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

Add TLS and basic authentication #101

Merged
merged 1 commit into from
Jan 14, 2021
Merged

Conversation

roidelapluie
Copy link
Member

Signed-off-by: Julien Pivotto [email protected]

Copy link
Member

@grobie grobie left a comment

Choose a reason for hiding this comment

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

Thanks for that! IMHO it would have been great to follow existing naming patterns in the toolkit interface, but I guess I missed it to comment on that in the repository itself and the ship has sailed now.

@@ -82,7 +87,7 @@ func main() {
})

level.Info(logger).Log("msg", "Listening on address", "address", *listenAddress)
if err := http.ListenAndServe(*listenAddress, nil); err != nil {
if err := https.Listen(srv, *httpsConfig, logger); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

As the server is not necessarily serving HTTPS, I'd have chosen a more neutral package name to express this better, e.g. server. Following the naming of the golang stdlib and calling the function ListenAndServe would have also helped to make it easier for developers to understand what's going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am willing to take a PR for renaming Listen to ListenAndServe and add a backwards compatible Listen function.

Copy link
Member

Choose a reason for hiding this comment

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

Let's change this here @roidelapluie?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want you fully happy: prometheus/exporter-toolkit#29

@roidelapluie roidelapluie force-pushed the bauth branch 2 times, most recently from b126105 to 05a48ef Compare January 13, 2021 20:41
@roidelapluie
Copy link
Member Author

Please approve again

Signed-off-by: Julien Pivotto <[email protected]>
Copy link
Member

@grobie grobie left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Please feel totally free to ignore my nitpicking! This is great already.

cmd/memcached_exporter/main.go Show resolved Hide resolved
@roidelapluie roidelapluie merged commit 5c34df5 into prometheus:master Jan 14, 2021
@roidelapluie
Copy link
Member Author

roidelapluie commented Jan 14, 2021

To stay consistent with other exporters and because you have resolved the conversation, I have merged this. We could revisit later, if we feel this is an issue.

@sangeetg
Copy link

Do we know when will be next stable release of prometheus/memcached_exporter which will include this TLS code?

@SuperQ
Copy link
Member

SuperQ commented Mar 25, 2021

@sangeetg I can get that started.

@sangeetg
Copy link

Thank you @SuperQ ... once you build a release, will it be tested to ensure it is stable or the tip is already tested and is stable?

SuperQ added a commit that referenced this pull request Mar 25, 2021
* [FEATURE] Add TLS and basic authentication #101

Signed-off-by: Ben Kochie <[email protected]>
@SuperQ SuperQ mentioned this pull request Mar 25, 2021
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.

4 participants