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

feat(chart/opensearch): add ability for serviceMonitor basicauth #594

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eyenx
Copy link
Contributor

@eyenx eyenx commented Sep 10, 2024

Description

Right now there is non possibility to set a basicAuth configuration for the serviceMonitor. This PR adds it

Check List

  • Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • Helm chart version bumped
  • Helm chart CHANGELOG.md updated to reflect change

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@peterzhuamazon
Copy link
Member

Note: We are in the review of an older PR now so will come back to this a little later.

@eyenx
Copy link
Contributor Author

eyenx commented Oct 18, 2024

@peterzhuamazon any update here? Thanks!

@eyenx
Copy link
Contributor Author

eyenx commented Nov 5, 2024

@peterzhuamazon any update here?

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Seems fine for me on this change.

cc: @prudhvigodithi @TheAlgo to take a look as well.

Thanks.

# password:
# secretName: credentials
# secretKey: password
basicAuth: {}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @eyenx can you please some details in the README ? also can basicAuth have one secret with username and password ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look at it tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now added a existingSecret functionality and a way to deliver the username & pw over the helm charts values

@peterzhuamazon
Copy link
Member

Hi @TheAlgo @prudhvigodithi please take another pass before I give an approval.

Thanks.

@peterzhuamazon
Copy link
Member

Pinging @prudhvigodithi @TheAlgo again to co-review.

Thanks!

@eyenx eyenx force-pushed the feat/serviceMonitor-with-basic-auth branch from f3a06ee to 9d239a8 Compare December 13, 2024 15:05
@eyenx
Copy link
Contributor Author

eyenx commented Dec 13, 2024

@prudhvigodithi @TheAlgo @peterzhuamazon Another attempt at getting this merged!

Please review! Thank you!

@eyenx
Copy link
Contributor Author

eyenx commented Dec 13, 2024

I would also be very happy to backport this to version 1.x

@peterzhuamazon
Copy link
Member

Adding @TheAlgo @prudhvigodithi @DandyDeveloper @gaiksaya to take a look.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📦 Backlog
Development

Successfully merging this pull request may close these issues.

3 participants