[stable/airflow] (ISSUE 20568) implemented feature to remove default connections #21018
Conversation
Signed-off-by: Shaun Elliott <javamonkey79@gmail.com>
helm-bot
added
Contribution Allowed
|
Hi @javamonkey79. Thanks for your PR. I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: javamonkey79 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign @maver1ck |
|
I would accept this one. @nbartelot, what do you think ? |
|
After thinking about it a little bit more I now doubt that it is the chart's job to manage the default connections. Here, it hardcodes the list of default connections in a script that's not maintained at the same level they are added/removed from the Airflow project. I see several ways to fix this:
Pro : this is the nicest way to do things.
Pro : this is the least effort compared to the current commit.
Pro : that would help for other tasks, like creating users, and the user could choose exactly what to do with the default connections. I'd go with 1 for being the nicest and cleanest. But 3 is also very appealing, and I'm in need of such a feature so I could make the PR anyway. |
|
Why not keeping this hard coded list and when 1) is implemented we can remove it ? Can you open a jira ticket on airflow project about this point? |
|
@gsemet it creates a hard link between the chart and the Airflow docker image version. As-is, the script would break if an attempt is made to remove the default connection while it does not exist. Imagine you use Airflow 1.10.4 that uses a default connection named mock_default. And then in Airflow 1.11.0 this connection does not exist anymone. One should not be forced to wait for the Helm chart to be upgraded in order to use the new Airflow version (i.e. the new docker images for that version), instead of just changing the image's version in the chart's values. Please see my PR #21047 for an implementation of idea number 3. Note : If you want to keep the idea of the current PR, I think the script should be reworked in order to make it resilient to errors (like the connection not existing anymore). In fact, it will need to be idempotent in any case because it will be executed when the pod restarts. |
|
I agree with the script rework, in case upstream change we should be resilient. |
|
@gsemet @NBardelot I explored option 2; it's not promising since the output of the airflow connections list is difficult to parse and in some cases, the data is not available (for example, see If we feel strongly enough that the hardcoding of default values approach will not work, I can close my PR and just opt to use @NBardelot (and my former) approach of extra startup scripts. wdyt? |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
and removed
needs-ok-to-test
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
stale
bot
added
the
lifecycle/stale
|
This issue is being automatically closed due to inactivity. |
Signed-off-by: Shaun Elliott javamonkey79@gmail.com
What this PR does / why we need it:
Which issue this PR fixes
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
[stable/mychartname])