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

[prometheus-pushgateway] Align resource label handling #456

Merged

Conversation

skhalash
Copy link
Contributor

@skhalash skhalash commented Dec 6, 2020

Signed-off-by: Stanislav Khalash stanislav.khalash@sap.com

What this PR does / why we need it:

Most of the templated resources are using the prometheus-pushgateway.defaultLabels template to insert labels. The only difference is Ingress and ServiceMonitor, where the labels are hard-coded. This PR eliminates code duplication by aligning label handling for all resources.

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

@stale
Copy link

stale bot commented Jan 5, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale label Jan 5, 2021
@skhalash
Copy link
Contributor Author

skhalash commented Jan 5, 2021

Hi @gianrubio! Could you please review this one?

@stale stale bot removed the lifecycle/stale label Jan 5, 2021
@monotek
Copy link
Member

monotek commented Jan 9, 2021

@scottrigby
could you jump in with a review / merge please?

@scottrigby
Copy link
Member

scottrigby commented Jan 12, 2021

Ah, @cstaud is also a maintainer of this chart but is not invited to the @prometheus-community/helm-charts-maintainers team 😮 I see this was worked around here #195 (comment).

@prometheus-community/helm-charts-admins (@SuperQ or @bismarck @brancz) would you please invite him to the helm-charts-maintainers team? Thanks 😄

Let's wait just a bit to get that sorted or at least give @cstaud a chance to review. I was not aware this never happened, so he was never notified of this PR and hasn't had a chance to review yet.

@bismarck
Copy link
Contributor

@scottrigby I don't believe I have permission to add @cstaud to the team?

@scottrigby
Copy link
Member

@bismarck apologies, autocomplete snafu 🤦‍♂️ I meant @brancz 😄

I have also opened this issue prometheus-community/community#31

@skhalash
Copy link
Contributor Author

@scottrigby @brancz Hey folks, sorry for bugging you, but did you have a chance to review the PR?

@SuperQ
Copy link
Contributor

SuperQ commented Jan 25, 2021

@scottrigby Sorry, I didn't see the add member issue. Invite sent.

monotek
monotek previously approved these changes Jan 25, 2021
zanhsieh
zanhsieh previously approved these changes Jan 26, 2021
@skhalash skhalash dismissed stale reviews from zanhsieh and monotek via 3446125 January 26, 2021 07:42
Signed-off-by: Stanislav Khalash <stanislav.khalash@sap.com>
Signed-off-by: Stanislav Khalash <stanislav.khalash@sap.com>
monotek
monotek previously approved these changes Jan 26, 2021
@stale
Copy link

stale bot commented Feb 25, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

Signed-off-by: Stanislav Khalash <stanislav.khalash@gmail.com>
@zanhsieh
Copy link
Contributor

zanhsieh commented Mar 1, 2021

@scottrigby
could you jump in with a review / merge please?

@scottrigby
Copy link
Member

Hi yes, I believe we've waited long enough per our PROCESSES doc 👌

@scottrigby
Copy link
Member

Can not merge on my phone now - as settings need to be temporarily changed to do so in the GitHub app - can do on laptop tomorrow.

@skhalash
Copy link
Contributor Author

skhalash commented Mar 4, 2021

Hey @scottrigby, just a friendly reminder ;) Could you please merge it?

@scottrigby scottrigby merged commit 8ea5f7e into prometheus-community:main Mar 5, 2021
@scottrigby
Copy link
Member

Done! ✅

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

6 participants