Fix edge cases of "migrate/create-default-connections"#33136
Merged
Conversation
boring-cyborg
Bot
added
area:CLI
area:dev-tools
area:providers
kind:documentation
provider:amazon
potiuk
force-pushed
the
support-pre-2-7-start-airflow
branch
from
August 5, 2023 08:12
e324782 to
d4b8f68
Compare
Member
Author
potiuk
force-pushed
the
support-pre-2-7-start-airflow
branch
5 times, most recently
from
August 5, 2023 17:02
e635085 to
377c0ef
Compare
Member
Author
|
OK. Changed the approach. Seems to be nicer and less surprising. Options can now be individually deprecated (not replaced by others but just deprecated) - and deprecation reason is displayed nicely in both - warning message and in the documentation. 
|
potiuk
force-pushed
the
support-pre-2-7-start-airflow
branch
3 times, most recently
from
August 5, 2023 17:13
219e8c0 to
6cd4737
Compare
jedcunningham
left a comment
jedcunningham
left a comment
Member
There was a problem hiding this comment.
The formatting is a little weird to me. Having the first line of the reason in the blue box feels unnecessary.
Member
Author
Yeah. Sphinx is doing things on its own sometimes. I had another verison of it that looked different. Let me try it. |
In apache#32810, "airflow db migrate" command has been added and it is used by `start-airflow` command. There were a few edge cases not covered and this PR completes it. * We can move the "database/load_default_connections" configuration to a new "deprecated" section. This is the first time we remove the option completely as it lost its meaning, but we likely still want to explain that the option was there and what it does when deprecated "db init" command is used. It has no meaning when you use "airflow db migrate" or when you run the new "airflow connections create-default-connections" commands. So we can now remove it completely from configuraiton. It will still work in the "airflow db init" which is deprecated, as long as we provide an explicit fallback. Also if someone had it defined in their config or env variable, it will continue to work even if it is not defined. * We need to explain the change in a significant newsfragment. * The ``start-airflow`` command supports creating default connections with ``--load-default-connections`` flag. This was lost after the change so this PR brings it back by running the new "airflow connections create-default-connections" command if the flag is used. * The `start-airflow` breeze command can be used to start older versions of airflow - with ``--use-airflow-version" - those that do not support `airflow db migrate` command. In this case the old behaviour is used with setting the "AIRFLOW__DATABASE__LOAD_DEFAULT_CONNECTIONS" based on the flag passed and running "airflow db init" instead.
potiuk
force-pushed
the
support-pre-2-7-start-airflow
branch
from
August 5, 2023 21:14
6cd4737 to
d0e1378
Compare
Member
Author
|
How about this? 
(other things except description -> detail done. I might want to change it in a separate PR though. |
jedcunningham
approved these changes
Aug 5, 2023
Member
Author
|
Same flaky as fixed (?) in main. |
ephraimbuddy
pushed a commit
that referenced
this pull request
Aug 8, 2023
In #32810, "airflow db migrate" command has been added and it is used by `start-airflow` command. There were a few edge cases not covered and this PR completes it. * We can move the "database/load_default_connections" configuration to a new "deprecated" section. This is the first time we remove the option completely as it lost its meaning, but we likely still want to explain that the option was there and what it does when deprecated "db init" command is used. It has no meaning when you use "airflow db migrate" or when you run the new "airflow connections create-default-connections" commands. So we can now remove it completely from configuraiton. It will still work in the "airflow db init" which is deprecated, as long as we provide an explicit fallback. Also if someone had it defined in their config or env variable, it will continue to work even if it is not defined. * We need to explain the change in a significant newsfragment. * The ``start-airflow`` command supports creating default connections with ``--load-default-connections`` flag. This was lost after the change so this PR brings it back by running the new "airflow connections create-default-connections" command if the flag is used. * The `start-airflow` breeze command can be used to start older versions of airflow - with ``--use-airflow-version" - those that do not support `airflow db migrate` command. In this case the old behaviour is used with setting the "AIRFLOW__DATABASE__LOAD_DEFAULT_CONNECTIONS" based on the flag passed and running "airflow db init" instead. (cherry picked from commit b672ba4)
ephraimbuddy
added
the
type:misc/internal
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #32810, "airflow db migrate" command has been added and it
is used by
start-airflowcommand. There were a few edge casesnot covered and this PR completes it.
to a new "deprecated" section. This is the first time we remove
the option completely as it lost its meaning, but we likely
still want to explain that the option was there and what it does
when deprecated "db init" command is used.
It has no meaning when you use "airflow db migrate" or when you run the
new "airflow connections create-default-connections" commands. So we can
now remove it completely from configuraiton. It will still work
in the "airflow db init" which is deprecated, as long as we provide an
explicit fallback. Also if someone had it defined in their config or
env variable, it will continue to work even if it is not defined.
We need to explain the change in a significant newsfragment.
The
start-airflowcommand supports creating default connectionswith
--load-default-connectionsflag. This was lost afterthe change so this PR brings it back by running the new
"airflow connections create-default-connections" command if the
flag is used.
The
start-airflowbreeze command can be used to start olderversions of airflow - with ``--use-airflow-version" - those that do not
support
airflow db migratecommand. In this case the old behaviour isused with setting the "AIRFLOW__DATABASE__LOAD_DEFAULT_CONNECTIONS"
based on the flag passed and running "airflow db init" instead.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.