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

Use the same pod annotation formatting in syncresources cronjob #2439

Conversation

bstadlbauer
Copy link
Member

The pod annotations for flyteadmin and clusterresourcesync are inconsistent in the flyte-core helm chart.

Flyteadmin:

  template:
    metadata:
      annotations:
        configChecksum: {{ include (print .Template.BasePath "/admin/configmap.yaml") . | sha256sum | trunc 63 | quote }}
        {{- with .Values.flyteadmin.podAnnotations }}
        {{- toYaml . | nindent 8 }}
        {{- end }}

Clusterresourcesync:

      template:
        {{- with .Values.flyteadmin.podAnnotations }}
        metadata:
          annotations: {{ tpl (toYaml .) $ | nindent 12 }}
        {{- end }}

Note that one will try to complete the template, whereas the other will not.
So with annotations defined as:

  flyteadmin:
    podAnnotations:
      vault.hashicorp.com/agent-inject-template-flyte-admin-database: |
        {{- with secret "secret/flyte/admin/database" -}}
          {{- .Data.data.PASSWORD }}
        {{- end }}

The outcome would be different on the two pods.

This PR would align the formatting to be the same in both (just "print" the values as is)

The pod annotation formatting for flyteadmin was different to the one from the formatting in the syncresources cronjob. This leads to once interpolating templates whilst the other will not.

Signed-off-by: Bernhard Stadlbauer <[email protected]>
Signed-off-by: Bernhard Stadlbauer <[email protected]>
@bstadlbauer bstadlbauer requested a review from yindia May 5, 2022 14:00
annotations:
{{- with .Values.flyteadmin.podAnnotations }}
{{- toYaml . | nindent 12 }}
{{- end }}
spec:
Copy link
Contributor

Choose a reason for hiding this comment

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

small change, Add annotations if they are passed or else don't specify them in generated template.

        metadata:
          {{- with .Values.flyteadmin.podAnnotations }}
          annotations:
            {{- toYaml . | nindent 12 }}
          {{- end }}

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this 👍

yindia
yindia previously approved these changes May 5, 2022
@bstadlbauer
Copy link
Member Author

@evalsocket Thanks again for the quick review! I've updated the chart according to your review and have re-run make-helm in 0da6370

Copy link
Contributor

@yindia yindia left a comment

Choose a reason for hiding this comment

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

Awesome

@yindia yindia merged commit db3f229 into flyteorg:master May 6, 2022
@welcome
Copy link

welcome bot commented May 6, 2022

Congrats on merging your first pull request! 🎉

@bstadlbauer
Copy link
Member Author

@evalsocket - Would it be possible to release these changes? I've checked the v1.0.1 helm chart of flyte-core but it seems that these did not make it in yet

@yindia
Copy link
Contributor

yindia commented May 9, 2022

@bstadlbauer We only release chart with flyte release(In flyte App version is same as chart version).

@EngHabu @eapolinario Is it ok to create a beta release ?

@EngHabu
Copy link
Contributor

EngHabu commented May 16, 2022

Yes please go ahead!

@EngHabu
Copy link
Contributor

EngHabu commented May 16, 2022

1.0.2-b1

@agates4
Copy link

agates4 commented May 18, 2022

Hey @evalsocket, would love to see the beta release ~
Just confirming, would this change also be in that release? #2438

@yindia
Copy link
Contributor

yindia commented May 19, 2022

@agates4 yes

@bstadlbauer
Copy link
Member Author

@evalsocket Is there an ETA for the beta release?

@yindia
Copy link
Contributor

yindia commented May 25, 2022

@bstadlbauer We created a beta release https://github.com/flyteorg/flyte/releases/tag/v1.0.2-b1, Please check and let us know if anything missing

@bstadlbauer
Copy link
Member Author

@evalsocket Thank you for the release! We did run into an issue when adding podAnnotations to syncresources, I've opened up a PR here: #2542

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.

4 participants