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.

Sign up for GitHub

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

Jump to bottom

add tls support #133

Merged
merged 1 commit into from
Dec 2, 2020
Merged

add tls support #133

merged 1 commit into from
Dec 2, 2020

Conversation

marcosdotps
Copy link
Contributor

Proposed changes

As some high secured environments should have tls comms even for metrics, this PR address to close that gap and makes serve metrics using https an option.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING guide
  • I have proven my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have ensured the README is up to date
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch on my own fork

@marcosdotps marcosdotps force-pushed the feature/95526-added_tls_support branch 3 times, most recently from b25fb5d to 1c77442 Compare November 9, 2020 09:16
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Nov 10, 2020
@pleshakov
Copy link
Contributor

Hi @MPenate

Thanks for the PR! We'll review it and get back to you shortly

pleshakov
pleshakov reviewed Nov 17, 2020
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @MPenate

Apologies for the delay.

The PR looks good! Thanks for updating the docs as well.

Just a few consistency-related issues. Could you possibly address them?

Additionally, we prefer present tense for commit messages. For example, Add feature instead of Added feature. Could you also update the commit message/PR title?

exporter.go Outdated Show resolved Hide resolved
exporter.go Outdated Show resolved Hide resolved
exporter.go Outdated Show resolved Hide resolved
exporter.go Outdated Show resolved Hide resolved
exporter.go Outdated Show resolved Hide resolved
@marcosdotps marcosdotps force-pushed the feature/95526-added_tls_support branch 3 times, most recently from 83f10fc to 90aba19 Compare November 23, 2020 23:53
@marcosdotps marcosdotps changed the title added tls support add tls support Nov 23, 2020
@marcosdotps marcosdotps force-pushed the feature/95526-added_tls_support branch from 90aba19 to ba00a9e Compare November 23, 2020 23:55
@marcosdotps
Copy link
Contributor Author

Hi @MPenate

Apologies for the delay.

The PR looks good! Thanks for updating the docs as well.

Just a few consistency-related issues. Could you possibly address them?

Additionally, we prefer present tense for commit messages. For example, Add feature instead of Added feature. Could you also update the commit message/PR title?

Resolved and also renamed PR/commit message. Thank you for the review!

pleshakov
pleshakov approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @MPenate

Looks good to me! Before we could merge, could you possibly rebase the PR against the latest master?

I'll request one more approval from my coworker, as we have 2 approvals policy.

lucacome
lucacome approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

LGTM

@marcosdotps marcosdotps force-pushed the feature/95526-added_tls_support branch from ba00a9e to 3782ae8 Compare December 2, 2020 21:19
@marcosdotps
Copy link
Contributor Author

Thanks for your reviews, rebased from upstream master.

@marcosdotps
add tls support
3629671 
Author:    mpenate <mpsanchez.desarrollo@gmail.com>
Date:      Fri Nov 6 17:23:10 2020 +0100
@marcosdotps marcosdotps force-pushed the feature/95526-added_tls_support branch from 3782ae8 to 3629671 Compare December 2, 2020 21:22
@pleshakov pleshakov merged commit d7cb179 into nginxinc:master Dec 2, 2020
@pleshakov
Copy link
Contributor

@MPenate thanks!

@lucacome lucacome added the minor label Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucacome lucacome lucacome approved these changes

@pleshakov pleshakov pleshakov approved these changes

Assignees
No one assigned
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcosdotps @pleshakov @lucacome