Keep consistency with the previous PR

[henry3260]

Keep consistency with the previous PR like #60238

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[henry3260]

@Miretpl To ensure the fallback logic works properly with the standard has check, I believe we should remove the default enabled: false in values.yaml (leaving it unset/nil). Otherwise, the default value overrides the old configuration in tests. WDYT?"

[Miretpl]

@henry3260, the change in chart behaviour is by intent (there was a discussion in #59730), so currently, we want to make sure that the behaviour will be the same for every workers.celery/kubernetes work and will be properly communicated.

Behaviour same as in already moved configs means:

1. If there is a default value specified, the user will need to manually unset the proper field in celery/kubernetes sections (for this, we also want to have a dedicated newsfragment file to make sure that this will be more visible to users in release notes),
2. If there is no default value specified, the behaviour remains unchanged.

For simple examples for the above points, you can take a look at (also in terms of what is changed, how and where):

1. Add workers.celery.replicas field #59730
2. Add workers.celery.revisionHistoryLimit field #60056

If you have further questions, feel free to ask!

[Miretpl]

As I was here, I've made a quick review of the current state to not be late like last time :D

[Miretpl]

Suggested change

	{{- $kerberosInitContainerEnabled := or (.Values.workers.celery.kerberosInitContainer).enabled (and (not (has (.Values.workers.celery.kerberosInitContainer).enabled (list true false))) (.Values.workers.kerberosInitContainer).enabled) }}
	{{- $kerberosInitContainerEnabled := or .Values.workers.celery.kerberosInitContainer.enabled (and (not (has .Values.workers.celery.kerberosInitContainer.enabled (list true false))) .Values.workers.kerberosInitContainer.enabled) }}

[Miretpl]

We will need to add additional test cases for these options

[henry3260]

As I was here, I've made a quick review of the current state to not be late like last time :D

Many thanks for your kind guidance!!!

[henry3260]

@henry3260, the change in chart behaviour is by intent (there was a discussion in #59730), so currently, we want to make sure that the behaviour will be the same for every workers.celery/kubernetes work and will be properly communicated.

Behaviour same as in already moved configs means:

1. If there is a default value specified, the user will need to manually unset the proper field in celery/kubernetes sections (for this, we also want to have a dedicated newsfragment file to make sure that this will be more visible to users in release notes),
2. If there is no default value specified, the behaviour remains unchanged.

For simple examples for the above points, you can take a look at (also in terms of what is changed, how and where):

1. Add workers.celery.replicas field #59730
2. Add workers.celery.revisionHistoryLimit field #60056

If you have further questions, feel free to ask!

Okay! So we should update the tests by explicitly setting workers.celery.kerberosInitContainer to nil if we want to verify workers.kerberosInitContainer, assuming I understand correctly.

[Miretpl]

Okay! So we should update the tests by explicitly setting workers.celery.kerberosInitContainer to nil if we want to verify workers.kerberosInitContainer, assuming I understand correctly.

Depends where. If you are testing the enabled flag behaviour - yes (not always, of course :D), if it is required to test something else, then no

[Miretpl]

Hi @henry3260, can I finish this section? I need it for #60420, which I would like to finish and properly tested today

[henry3260]

Hi @henry3260, can I finish this section? I need it for #60420, which I would like to finish and properly tested today

Feel free

[henry3260]

Hi @henry3260, can I finish this section? I need it for #60420, which I would like to finish and properly tested today

Do you need my help to add additional test cases for securityContext and lifecycle?
I can add them in this pr

[Miretpl]

In my PR, I will leave securityContexts and lifecycle as they are (as I believe that not using those fields anywhere is a different issue that current implementation in terms of separation and worker sets). I noticed that the kerberosInitContainer is also in pod-template-file, which changes the logic a little, and I'm not sure now in which direction we should go with it. If you want to take a look - it is here #60751

[Miretpl]

I think that we came to conclustion in the mentioned PR, so for this one, you would need only add missing support for:

1. securityContexts
2. containerLifecycleHooks

under kerberosInitContainer section of workers, workers.kubernetes and workers.celery.

+ of course test cases for each. Will that be good?

[henry3260]

I think that we came to conclustion in the mentioned PR, so for this one, you would need only add missing support for:

1. securityContexts
2. containerLifecycleHooks

under kerberosInitContainer section of workers, workers.kubernetes and workers.celery.

• of course test cases for each. Will that be good?

looks good!