Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for HTTPS Pushgateway endpoints #2

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

andremv
Copy link
Contributor

@andremv andremv commented Oct 19, 2020

This solves the issue with the Pushgateway endpoint being hardcoded to a http endpoint.

1) Test\BlackBoxPushGatewayTest::pushGatewayShouldWork
GuzzleHttp\Exception\ClientException: Client error: `PUT http://super-secure-pushgateway:443/metrics/job/my_job/instance/foo` resulted in a `400 Bad Request` response:
(truncated...)

I also brought over the missing php-fpm directory from PromPHP/prometheus_client_php repo.

Copy link
Member

@LKaemmerling LKaemmerling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @andremv,

first of all, Thank you! Good idea and a good catch. I have two smaller "issues" that should be addressed. The biggest "issue" in my mind is the miss of unit tests for the current & new behavior. We need to make sure that we don't introduce a breaking change.

src/PrometheusPushGateway/PushGateway.php Show resolved Hide resolved
src/PrometheusPushGateway/PushGateway.php Show resolved Hide resolved
@LKaemmerling
Copy link
Member

LKaemmerling commented Oct 21, 2020

Hey @andremv,

thank you! I have just one small note (the doc string on the doRequest method), otherwise, it looks fine from my side. An please have a look at the DCO test: https://github.com/PromPHP/prometheus_push_gateway_php/pull/2/checks?check_run_id=1283429563

Maybe @rdohms, if you have time could you also have a fix look?

Copy link
Member

@LKaemmerling LKaemmerling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you!

@andremv
Copy link
Contributor Author

andremv commented Oct 21, 2020

It was my pleasure @LKaemmerling! And thank you for maintaining this library.

Copy link
Member

@rdohms rdohms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, minor nitpick on readability.

Consideration for @LKaemmerling since we have not talked about it. My team's usual approach is to rebase and make fixes directly on the commit that introduced the code instead of adding new fix commits. Thoughts?

src/PrometheusPushGateway/PushGateway.php Outdated Show resolved Hide resolved
@LKaemmerling
Copy link
Member

Looks great, minor nitpick on readability.

Consideration for @LKaemmerling since we have not talked about it. My team's usual approach is to rebase and make fixes directly on the commit that introduced the code instead of adding new fix commits. Thoughts?

@rdohms yep I like that too, in most MRs I use squashing so that in the end it doesn’t matter how many commit were in the branch.

@andremv your welcome!

Signed-off-by: Andrei Moldovan <andrei.moldovan@fourth.com>
@LKaemmerling LKaemmerling merged commit 09e7574 into PromPHP:master Oct 22, 2020
@LKaemmerling
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants