-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Standardize AWS Athena naming #20305
Conversation
Missed renaming the hooks in the sensor tests, update coming. |
setup.cfg
Outdated
@@ -45,6 +45,7 @@ license_files = | |||
licenses/LICENSE-moment-strftime.txt | |||
licenses/LICENSE-moment.txt | |||
licenses/LICENSE-normalize.txt | |||
licenses/LICENSES-ui.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the pre-commits keep adding that??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that doesn’t make sense, there’s an identical line just below and pre-commit should catch that. Something is probably broken somewhere. Could you temporarily disable pre-commit to remove that line and push to see if CI would let it pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep this change should be removed.
this PR should not change setup.cfg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change reverted and pushed.
My pre-commit hooks also started failing a whole bunch of mypy tests today when I pulled the latest main down, so I may need to unbork something on my environment. Sorry I missed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, I just now rebased main and ran the pre-commits locally using pre-commit run --all-files
on it without any of my code changes and it did the same:
Update setup.cfg file with all licenses..................................................Failed
- hook id: update-setup-cfg-file
- files were modified by this hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@uranusjr any further comments?
Looks like I'll have to rebase to clear some merge conflicts. I'll take care of that tomorrow/Thursday. |
93f3dba
to
a4c96d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If there will be more comments we'll address them in a followup PR.
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. |
Part of #20296
Also adjusted some tests to use
poke({})
andexecute({})
instead ofpoke(None)
andexecute(None)
as the IDE was throwing type hint warnings on those.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.