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

Removing default prometheus service annotations in Helm #547

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

airamare01
Copy link
Collaborator

Checklist

Test plan

Tested locally. In a nutshell:

This modification:

  1. Removes the Prometheus scrape annotations entirely.
  2. Keeps the ability to add custom service annotations through .Values.executor.serviceAnnotations.
  3. Only includes the annotations section if there are actually any custom annotations to add.
  4. This approach allows removal of the potentially problematic Prometheus scrape targets while maintaining flexibility for users to add their own custom annotations if needed.
  5. Keeps the YAML clean by not including an empty annotations section when it's not needed.

@airamare01 airamare01 requested a review from loujar September 9, 2024 13:39
@airamare01
Copy link
Collaborator Author

Duplicate issue here: #529

Copy link
Contributor

@loujar loujar left a comment

Choose a reason for hiding this comment

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

It looks like the Executor Prometheus scrape annotations are still present in this PR:

metadata:
  annotations:
    prometheus.io/port: "6060"
    sourcegraph.prometheus/scrape: "true"

@airamare01 airamare01 requested a review from loujar September 19, 2024 13:21
@airamare01
Copy link
Collaborator Author

Fixed it @loujar

@airamare01 airamare01 merged commit ac68bf6 into main Sep 20, 2024
5 checks passed
@airamare01 airamare01 deleted the helm-prometheus branch September 20, 2024 11:00
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.

2 participants