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

fix the location of annotations for pods #31

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

rabunkosar-dd
Copy link
Contributor

Fixing the incorrect location for the pod annotation in #24 and also bumped the version to avoid the version issues.

@Pokom I noticed that issue while deploying, so apologies for the back and forth, can you take a look please?

Copy link
Collaborator

@Pokom Pokom left a comment

Choose a reason for hiding this comment

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

👍 From me. Again this is a bit harder for me to test out manually, so let me know once this has been deployed out.

charts/opencost/templates/deployment.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@Pokom Pokom left a comment

Choose a reason for hiding this comment

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

Left one comment. @rabunkosar-dd if you have an opportunity, can you provide a snippet of running a helm template with this value set?

metadata:
labels:
{{- include "opencost.selectorLabels" . | nindent 8 }}
{{- with .Values.podAnnotations }}
annotations:
{{- toYaml . | nindent 8 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it okay to not be explicit here with what we're writing out? Most other places in the codebase we're being explicit. IE, I believe it is a bit clearer if we say:

annotations:
{{- toYaml .Values.podAnnotations | nindent 8 }}

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've made the change, here is the test:

value:

podAnnotations:
  "iam.amazonaws.com/role": arn:aws:iam::12345:role/a

output:

    metadata:
      labels:
        app.kubernetes.io/name: opencost
        app.kubernetes.io/instance: opencost
      annotations:
        iam.amazonaws.com/role: arn:aws:iam::12345:role/a

the default case:

    metadata:
      labels:
        app.kubernetes.io/name: opencost
        app.kubernetes.io/instance: opencost

Hopefully the YAML is properly formatted and the objects are now in the right place 🤞

Copy link
Collaborator

Choose a reason for hiding this comment

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

Double checking against other deployments I've got with annotations and that looks like it's in the correct spot! Thanks for making the change

Copy link
Collaborator

@Pokom Pokom left a comment

Choose a reason for hiding this comment

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

👍 Tested personally with

helm template --set opencost.prometheus.internal.enabled=false --set opencost.prometheus.external.enabled=true --set opencost.metrics.serviceMonitor.enabled=true --set podAnnotations="test.this=foo" .

And noted that the annotation was created in the correct location with test.this=foo

@Pokom Pokom merged commit 67530df into opencost:main Mar 13, 2023
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