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

remove duplicate annotation in nats statefulset #2176

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

sudarshan-uc
Copy link
Contributor

combined config from two metadata.annotations

Description

There are two annotation sections in the NATS statefulsets,

Related Issues

fixes: https://github.com/astronomer/issues/issues/6284

Testing

Do not merge this PR until this text is replaced with details about how these changes were tested.

Merging

Do not merge this PR until it lists which release branches this PR should be merged / cherry-picked into.

combined config from two metadata.annotations
@sudarshan-uc sudarshan-uc requested a review from a team as a code owner April 8, 2024 06:49
@pgvishnuram pgvishnuram changed the title Update statefulset.yaml remove duplicate annotation in nats statefulset Apr 8, 2024
Comment on lines 24 to +29
metadata:
{{- if or .Values.podAnnotations .Values.exporter.enabled }}
annotations:
checksum/nats-config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
{{- if and .Values.global.nats.jetStream.enabled .Values.global.nats.jetStream.tls }}
checksum/nats-tls: {{ include (print $.Template.BasePath "/nats-jetstream-tls-secret.yaml") . | sha256sum }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@danielhoherd this is a new change on top of sudharsan change

@rishkarajgi
Copy link
Contributor

Tests are failing

@pgvishnuram
Copy link
Contributor

Let me re-run them , adding basic test case for annotation validation

@rob-1126
Copy link
Contributor

rob-1126 commented Apr 8, 2024

Can we explicitly test the upgrade path here I am expecting it to result in failures and that variable being completely unset for a period of time between the two due to helm issue 10741. What would the impact to the platform be during that time?

@pgvishnuram
Copy link
Contributor

i dont think this will cause an issue for the customers

@pgvishnuram pgvishnuram merged commit 1b60a57 into master Apr 8, 2024
7 of 8 checks passed
@pgvishnuram pgvishnuram deleted the sudarshan-uc-fix-annotations-in-NATS branch April 8, 2024 12:20
rishkarajgi added a commit that referenced this pull request Apr 8, 2024
* Update statefulset.yaml

combined config from two metadata.annotations

* update nats statefulset annotation work

* add basic annotation tests

* add key assertion

* fix pre-commit

* fix test failures

---------

Co-authored-by: pgvishnuram <vishnu@astronomer.io>
Co-authored-by: Rishabh Karajgi <rishabh.karajgi@gmail.com>
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

4 participants