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

Update secrets backends to use get_conn_value instead of get_conn_uri #22348

Merged

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Mar 17, 2022

In #19857 we enabled storing connections as JSON instead of URI and renamed get_conn_uri to get_conn_value to be consistent with this change. The method get_conn_uri is now deprecated and should warn when used.

This PR updates secrets backends in providers to implement the new method, while retaining support for the old method for backcompat. We cannot remove the old methods until min airflow version is updated to 2.3 for the provider.

For users of Airflow versions older than 2.3 we suppress the deprecation warning since it's only from 2.3 onward that the warning makes sense (below 2.3, get_conn_uri is called; in 2.3 we call get_conn_value instead).

@boring-cyborg boring-cyborg bot added area:providers area:secrets kind:documentation provider:amazon AWS/Amazon - related issues provider:microsoft-azure Azure-related issues provider:google Google (including GCP) related issues labels Mar 17, 2022
@dstandish dstandish force-pushed the update-secrets-backends-with-get-conn-value branch from 79a5d57 to a39f54b Compare March 17, 2022 19:20
@potiuk
Copy link
Member

potiuk commented Mar 17, 2022

I REALLY ❤️ the attitude of thinking about the user experience and avoiding false positives when you have warnings when you can't disable them. I think having even a single of such warning is a major reason why people develop blind-spots that makes warnings useless.

@dstandish dstandish force-pushed the update-secrets-backends-with-get-conn-value branch from fe4d22a to 48fe315 Compare March 19, 2022 06:15
@raphaelauv
Copy link
Contributor

thanks @dstandish for that PR

could be great to add to the doc a short code example that convert an URI airflow connection to JSON , so users ( and even myself ) could easily convert all their current connections secrets

@dstandish
Copy link
Contributor Author

thanks @dstandish for that PR

could be great to add to the doc a short code example that convert an URI airflow connection to JSON , so users ( and even myself ) could easily convert all their current connections secrets

a reasonable suggestion but i think that will belong in a separate PR

@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 Mar 24, 2022
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@dstandish dstandish merged commit 7ab45d4 into apache:main Mar 24, 2022
@dstandish dstandish deleted the update-secrets-backends-with-get-conn-value branch March 24, 2022 21:21
potiuk added a commit to potiuk/airflow that referenced this pull request Mar 25, 2022
The apache#22348 introduced a change on how connections are retrieved
from secret backends, but one of the tests has not been
changed to follow.

This fixes failing main.
potiuk added a commit that referenced this pull request Mar 25, 2022
The #22348 introduced a change on how connections are retrieved
from secret backends, but one of the tests has not been
changed to follow.

This fixes failing main.
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers area:secrets changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation okay to merge It's ok to merge this PR as it does not require more tests provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues provider:microsoft-azure Azure-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants