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

Reformat chart templates part 2 #29941

Merged
merged 1 commit into from
Mar 12, 2023

Conversation

dnskr
Copy link
Contributor

@dnskr dnskr commented Mar 6, 2023

The PR reformats multiple chart templates as the continuation of #29917

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Mar 6, 2023
@dnskr dnskr force-pushed the reformat_chart_templates_part_2 branch 2 times, most recently from 94e2941 to 607b5c3 Compare March 7, 2023 22:23
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Spotted some indents that might need to be nindents, but it may not matter?

Did you intentionally skip the pod_template_file template?

@dnskr
Copy link
Contributor Author

dnskr commented Mar 8, 2023

Hi @jedcunningham!

Overall LGTM. Spotted some indents that might need to be nindents, but it may not matter?

They should not be nindented because it will add one extra new line, i.e. the following

          envFrom: {{- include "custom_airflow_environment_from" . | default "\n  []" | nindent 10 }}
          env: {{- include "custom_airflow_environment" . | nindent 10 }}

produces

          envFrom:

            - secretRef:
                name: 'test-airflow-connections'
            - configMapRef:
                name: 'test-airflow-variables'

          env:

            # Dynamically created environment variables
            - name: qwe
              value: "rty"
            # Dynamically created secret envs
            # Extra env
            - name: AIRFLOW__CORE__LOAD_EXAMPLES
              value: 'True'

while current implementation

          envFrom: {{- include "custom_airflow_environment_from" . | default "\n  []" | indent 10 }}
          env: {{- include "custom_airflow_environment" . | indent 10 }}

produces

          envFrom:
            - secretRef:
                name: 'test-airflow-connections'
            - configMapRef:
                name: 'test-airflow-variables'

          env:
            # Dynamically created environment variables
            - name: qwe
              value: "rty"
            # Dynamically created secret envs
            # Extra env
            - name: AIRFLOW__CORE__LOAD_EXAMPLES
              value: 'True'

Did you intentionally skip the pod_template_file template?

I skipped pod-template-file.kubernetes-helm-yaml accidently and most of _helpers.yaml intentionally for now.

@dnskr dnskr force-pushed the reformat_chart_templates_part_2 branch from 607b5c3 to dc8df54 Compare March 8, 2023 20:23
@jedcunningham
Copy link
Member

I skipped pod-template-file.kubernetes-helm-yaml accidently and most of _helpers.yaml intentionally for now.

👍

it will add one extra new line

Ah. Can those helpers not emit an extra leading newline instead? If you want to defer to when you tackle helpers, that's cool too.

@dnskr
Copy link
Contributor Author

dnskr commented Mar 8, 2023

Ah. Can those helpers not emit an extra leading newline instead? If you want to defer to when you tackle helpers, that's cool too.

I don't know to be honest. I want to investigate it and refactor helpers separately.

@dnskr dnskr force-pushed the reformat_chart_templates_part_2 branch from dc8df54 to cb669d6 Compare March 10, 2023 16:39
@dnskr
Copy link
Contributor Author

dnskr commented Mar 10, 2023

Fixed merge conflict introduced by commit e60be9e

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants