You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/20 10:26:16 UTC
[GitHub] [airflow] FloChehab opened a new pull request #11681: fix helm chart worker deployment without kerberos
FloChehab opened a new pull request #11681:
URL: https://github.com/apache/airflow/pull/11681
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)
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712790910
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712789182
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712789182
> @potiuk Should we add unit tests? Did your patch with kerberos have the unittest?
Yes it had.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] FloChehab commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-713081384
Helm tests should be fixed (at least this part of the CI is green).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] FloChehab edited a comment on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
FloChehab edited a comment on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712789987
I'll add a commit to fix the tests soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] FloChehab commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712789987
I'll add a commit to fix the test soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712770011
[The Workflow run](https://github.com/apache/airflow/actions/runs/317477969) 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*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712769628
[The Workflow run](https://github.com/apache/airflow/actions/runs/317477548) 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*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712790910
@flo076 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712760132
@potiuk Should we add unit tests? Did your patch with kerberos have the unittest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] FloChehab commented on pull request #11681: fix helm chart worker deployment without kerberos
Posted by GitBox <gi...@apache.org>.
FloChehab commented on pull request #11681:
URL: https://github.com/apache/airflow/pull/11681#issuecomment-712794283
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org