Skip to content

Check python version before starting triggerer - #18926

Merged
potiuk merged 3 commits into
apache:mainfrom
astronomer:triggerer_check_version
Oct 13, 2021
Merged

Check python version before starting triggerer#18926
potiuk merged 3 commits into
apache:mainfrom
astronomer:triggerer_check_version

Conversation

@jedcunningham

@jedcunningham jedcunningham commented Oct 12, 2021

Copy link
Copy Markdown
Member

The triggerer requires Python 3.7+.

cc: @andrewgodwin

@jedcunningham
Check python version before starting triggerer
c114a15 
The triggerer requires Python 3.7+.
@jedcunningham jedcunningham added this to the Airflow 2.2.1 milestone Oct 12, 2021
@andrewgodwin

andrewgodwin commented Oct 12, 2021

Copy link
Copy Markdown
Contributor

I thought our import error in utils.asyncio errored out early enough, but this is nicer :)

mik-laj
mik-laj reviewed Oct 12, 2021
View reviewed changes
Comment thread airflow/cli/cli_parser.py Resolved
@github-actions

github-actions Bot commented Oct 13, 2021

Copy link
Copy Markdown
Contributor

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.

@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 Oct 13, 2021
@potiuk

potiuk commented Oct 13, 2021

Copy link
Copy Markdown
Member

Skipif needed for triggerer tests

ashb
ashb reviewed Oct 13, 2021
View reviewed changes

@ashb ashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason you put the check here, rather than inside trigger_command.py?

@mik-laj

mik-laj commented Oct 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@ashb Loading modules that we know are incompatible may have unexpected results. At this point, we are sure the module has not yet been loaded.

Additionally, this situation is handled by CLI framework, so we have consistent UX experience

@jedcunningham

jedcunningham commented Oct 13, 2021

Copy link
Copy Markdown
Member Author

@ashb, yeah consistency was my reasoning.

@potiuk
potiuk merged commit f0e2143 into apache:main Oct 13, 2021
@ephraimbuddy
ephraimbuddy deleted the triggerer_check_version branch October 13, 2021 21:21
jedcunningham added a commit that referenced this pull request Oct 14, 2021
@jedcunningham
Check python version before starting triggerer (#18926)
7183ad0 
(cherry picked from commit f0e2143)
@potiuk potiuk mentioned this pull request Oct 27, 2021
Closed
24 of 37 tasks
@mik-laj mik-laj mentioned this pull request Oct 28, 2021
Closed
1 of 2 tasks
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashb ashb ashb left review comments
@potiuk potiuk potiuk approved these changes
@kaxil kaxil kaxil approved these changes
@mik-laj mik-laj mik-laj approved these changes
+1 more reviewer
@andrewgodwin andrewgodwin andrewgodwin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:CLI okay to merge It's ok to merge this PR as it does not require more tests type:bug-fix Changelog: Bug Fixes

Projects

None yet

Milestone

Airflow 2.2.1

Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham @andrewgodwin @potiuk @mik-laj @ashb @kaxil