Skip to content

Conversation

@DMilmont
Copy link
Contributor

@DMilmont DMilmont commented Mar 3, 2023

This adds annotations to the following configmap objects:

Webserver
StatsD

Motivation:
We use spinnaker to deploy this helm chart, however spinnaker by default versions configmaps. This forces us to fork this helm chart and modify it to work with spinnaker, by adding annotations to disable spinnaker versioning. This would take us a step closer to not having to do that anymore.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Mar 3, 2023
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

You need to install pre-commit to fix static checks

@DMilmont
Copy link
Contributor Author

@hussein-awala I installed pre-commit to fix static checks and committed changes.

@DMilmont DMilmont requested review from hussein-awala and removed request for dstandish and jedcunningham March 15, 2023 02:07
@potiuk
Copy link
Member

potiuk commented Mar 15, 2023

@hussein-awala I installed pre-commit to fix static checks and committed changes.

There are few more suggestions from @hussein-awala

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

I think you had a conflict and you fixed it by overriding some changes from #29917 which improved the format of the templates.

Also you can improve the format of the new annotations by adding some indents as #29917 did.

Comment on lines +32 to +34
{{- with .Values.labels }}
{{ toYaml . | indent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this?

Suggested change
{{- with .Values.labels }}
{{ toYaml . | indent 4 }}
{{- end }}
{{- with .Values.labels }}
{{- toYaml . | nindent 4 }}
{{- end }}

Comment on lines +35 to +38
{{- with .Values.statsd.configMapAnnotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Add indent

Suggested change
{{- with .Values.statsd.configMapAnnotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- with .Values.statsd.configMapAnnotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}

Comment on lines +32 to +34
{{- with .Values.labels }}
{{ toYaml . | indent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

same here:

Suggested change
{{- with .Values.labels }}
{{ toYaml . | indent 4 }}
{{- end }}
{{- with .Values.labels }}
{{- toYaml . | nindent 4 }}
{{- end }

Comment on lines +35 to +38
{{- with .Values.webserver.configMapAnnotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Add indent

Suggested change
{{- with .Values.webserver.configMapAnnotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- with .Values.webserver.configMapAnnotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}

@github-actions
Copy link

github-actions bot commented May 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 7, 2023
@github-actions github-actions bot closed this May 13, 2023
@amoghrajesh
Copy link
Contributor

@hussein-awala I will take this up as we also need some custom labels/annotations onto our configmaps.
I will complete this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants