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 edge cases of "migrate/create-default-connections" #33136

Merged
merged 1 commit into from
Aug 5, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Aug 5, 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.


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Aug 5, 2023

cc: @Adaverse -> small follow up after #32810 - noticed a few edge-cases and I think we need to add release notes to explain the new behaviour.

@potiuk potiuk force-pushed the support-pre-2-7-start-airflow branch 5 times, most recently from e635085 to 377c0ef Compare August 5, 2023 17:02
@potiuk
Copy link
Member Author

potiuk commented Aug 5, 2023

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.

Screenshot 2023-08-05 at 19 11 13

@potiuk potiuk force-pushed the support-pre-2-7-start-airflow branch 3 times, most recently from 219e8c0 to 6cd4737 Compare August 5, 2023 17:13
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting is a little weird to me. Having the first line of the reason in the blue box feels unnecessary.

airflow/config_templates/config.yml Outdated Show resolved Hide resolved
airflow/configuration.py Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Aug 5, 2023

The formatting is a little weird to me. Having the first line of the reason in the blue box feels unnecessary.

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 potiuk force-pushed the support-pre-2-7-start-airflow branch from 6cd4737 to d0e1378 Compare August 5, 2023 21:14
@potiuk
Copy link
Member Author

potiuk commented Aug 5, 2023

How about this?

Screenshot 2023-08-05 at 23 14 02

(other things except description -> detail done. I might want to change it in a separate PR though.

@potiuk
Copy link
Member Author

potiuk commented Aug 5, 2023

Same flaky as fixed (?) in main.

@potiuk potiuk merged commit b672ba4 into apache:main Aug 5, 2023
60 of 61 checks passed
@potiuk potiuk deleted the support-pre-2-7-start-airflow branch August 5, 2023 22:19
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 ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:dev-tools area:providers kind:documentation provider:amazon-aws AWS/Amazon - related issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants