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 helm chart worker deployment without kerberos #11681

Merged
merged 1 commit into from
Oct 31, 2020

Conversation

FloChehab
Copy link
Contributor

Hello,

A small follow up to #11130 after some tests on the new chart : we shouldn't mount the kerberos-keytab volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Oct 20, 2020
@mik-laj mik-laj requested a review from potiuk October 20, 2020 10:38
@mik-laj
Copy link
Member

mik-laj commented Oct 20, 2020

@potiuk Should we add unit tests? Did your patch with kerberos have the unittest?

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk
Copy link
Member

potiuk commented Oct 20, 2020

@potiuk Should we add unit tests? Did your patch with kerberos have the unittest?

Yes it had. Those are the very tests that are failing now :). So having tests did not help in this case.

@FloChehab
Copy link
Contributor Author

FloChehab commented Oct 20, 2020

I'll add a commit to fix the tests soon.

@potiuk
Copy link
Member

potiuk commented Oct 20, 2020

@FloChehab The unit tests need updating. Also We have a discussion in #11657 about better unit tests for the chart. I would really love to hear your opinion as a "newcomer" whether the current unit tests are easy to understand and maintain and how do they compare to the proposal by @mik-laj in this issue.

@FloChehab
Copy link
Contributor Author

@FloChehab The unit tests need updating. Also We have a discussion in #11657 about better unit tests for the chart. I would really love to hear your opinion as a "newcomer" whether the current unit tests are easy to understand and maintain and how do they compare to the proposal by @mik-laj in this issue.

I don't have experience with chart testing in general ; I'll play with the current setup for this PR (and the other ones that I have opened / will open) and try to give feedback.

@FloChehab
Copy link
Contributor Author

Helm tests should be fixed (at least this part of the CI is green).

@mik-laj
Copy link
Member

mik-laj commented Oct 30, 2020

Hello.

We have a new test framework for Helm Chart. Can you add unit tests?

Best regards,
Kamil Breguła

Follow up to apache#11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)
@FloChehab FloChehab force-pushed the fix-helm-without-kerberos branch from a3e3266 to 0369f63 Compare October 30, 2020 20:33
@FloChehab
Copy link
Contributor Author

Hello,

I've rebased on master and added a test to prevent kerberos from being mentioned in the render if it's not enabled (except in airflow's configmap because it would be a bit hard to change this behavior.

I'll be happy to adapt if necessary.

@mik-laj mik-laj changed the title fix helm chart worker deployment without kerberos Fix helm chart worker deployment without kerberos Oct 31, 2020
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Oct 31, 2020
@github-actions
Copy link

The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!

@potiuk potiuk merged commit 4c54718 into apache:master Oct 31, 2020
@FloChehab FloChehab deleted the fix-helm-without-kerberos branch October 31, 2020 20:26
@FloChehab
Copy link
Contributor Author

Thanks for the merge !

szn pushed a commit to szn/airflow that referenced this pull request Nov 1, 2020
Follow up to apache#11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)
potiuk pushed a commit that referenced this pull request Nov 15, 2020
Follow up to #11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)

(cherry picked from commit 4c54718)
@potiuk potiuk added the type:bug-fix Changelog: Bug Fixes label Nov 15, 2020
@potiuk potiuk added this to the Airflow 1.10.13 milestone Nov 15, 2020
potiuk pushed a commit that referenced this pull request Nov 16, 2020
Follow up to #11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)

(cherry picked from commit 4c54718)
potiuk pushed a commit that referenced this pull request Nov 16, 2020
Follow up to #11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)

(cherry picked from commit 4c54718)
kaxil pushed a commit that referenced this pull request Nov 18, 2020
Follow up to #11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)

(cherry picked from commit 4c54718)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Follow up to apache#11130 : we shouldn't mount the `kerberos-keytab` volume
in the worker deployment if we are not using
kerberos in the first place.
(the previous behavior is breaking the chart)

(cherry picked from commit 4c54718)
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 okay to merge It's ok to merge this PR as it does not require more tests type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants