New breeze command to clean up previous provider artifacts #35970

amoghrajesh · 2023-11-30T04:58:10Z

Currently during every release of a provider, cleanup of the older providers was done using a script. It would much better for the release manager if this were a breeze command.

Introducing a new breeze command for the same under release commands.

Command:

Help:

Running style:

(new-env) ➜  airflow git:(breezeCommandToRemoveOldReleases) ✗ breeze release-management clean-old-provider-artifacts --directory . --execute
Running provider cleanup for suffix: .tar.gz
Running provider cleanup for suffix: .tar.gz.sha512
Running provider cleanup for suffix: .tar.gz.asc
Running provider cleanup for suffix: -py3-none-any.whl
Running provider cleanup for suffix: -py3-none-any.whl.sha512
Running provider cleanup for suffix: -py3-none-any.whl.asc

^ 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

This looks cool.

Two things (besides static checks):

remove old script in the same PR
update the https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md (and check if there are any other references to the old script)

amoghrajesh · 2023-11-30T09:54:51Z

Woops.

remove old script in the same PR
I made the changes, forgot to push!

amoghrajesh · 2023-11-30T09:55:11Z

I have also updated the instructions in the required MD files. Should be good now :D

dev/README_RELEASE_PROVIDER_PACKAGES.md

amoghrajesh · 2023-11-30T09:55:39Z

@potiuk BTW, i do not see static checks failing in my env or here. Are they failing?

potiuk · 2023-11-30T09:58:42Z

@potiuk BTW, i do not see static checks failing in my env or here. Are they failing?

Could be intermittent - I see that recent docker has been failing more frequently in CI (likely 24.0.7 version has some stability issue - I still want to wait for 24.0.8 that should get out any time soon because 24.0.7 has important security fixes https://docs.docker.com/engine/release-notes/24.0/#security)

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

amoghrajesh · 2023-11-30T10:03:46Z

@potiuk BTW, i do not see static checks failing in my env or here. Are they failing?

Could be intermittent - I see that recent docker has been failing more frequently in CI (likely 24.0.7 version has some stability issue - I still want to wait for 24.0.8 that should get out any time soon because 24.0.7 has important security fixes https://docs.docker.com/engine/release-notes/24.0/#security)

Yeah looks like it. Sure, thanks for the update

potiuk · 2023-11-30T11:04:14Z

@potiuk BTW, i do not see static checks failing in my env or here. Are they failing?

Could be intermittent - I see that recent docker has been failing more frequently in CI (likely 24.0.7 version has some stability issue - I still want to wait for 24.0.8 that should get out any time soon because 24.0.7 has important security fixes https://docs.docker.com/engine/release-notes/24.0/#security)

Yeah looks like it. Sure, thanks for the update

Nope. It's a real issue:

- hook id: check-breeze-top-dependencies-limited
- exit code: 1

Traceback (most recent call last):
  File "dev/breeze/src/airflow_breeze/breeze.py", line 36, in <module>
    from airflow_breeze.commands.release_management_commands import release_management  # noqa
  File "/home/runner/actions-runner/_work/airflow/airflow/dev/breeze/src/airflow_breeze/commands/release_management_commands.py", line 37, in <module>
    from packaging.version import Version
ModuleNotFoundError: No module named 'packaging'
Breeze should only use limited dependencies when imported (see errors above).

This is the error:

Breeze should only use limited dependencies when imported (see errors above).

You have to make sure that packaging is locally imported not at the top-level. The problem is that when you do auto-completion, the auto-completion script will parse all the commands and click decorators. And in order to do that, it has to import the command modules. And this happens in a very "plain" python environment of python where no dependencies are installed (except rich and click) - the autocomplete script runs outside of the breeze venv (and this is not something we can or should change).

This means that you cannot add 3rd-party imports as top-level imports in the command files. This also allows auto-complete to be fast, because it will not import unnecessary a lot of packages.

This pre-commit failing checks that.

amoghrajesh · 2023-11-30T11:12:31Z

@potiuk BTW, i do not see static checks failing in my env or here. Are they failing?

Could be intermittent - I see that recent docker has been failing more frequently in CI (likely 24.0.7 version has some stability issue - I still want to wait for 24.0.8 that should get out any time soon because 24.0.7 has important security fixes https://docs.docker.com/engine/release-notes/24.0/#security)

Yeah looks like it. Sure, thanks for the update

Nope. It's a real issue:
- hook id: check-breeze-top-dependencies-limited
- exit code: 1

Traceback (most recent call last):
  File "dev/breeze/src/airflow_breeze/breeze.py", line 36, in <module>
    from airflow_breeze.commands.release_management_commands import release_management  # noqa
  File "/home/runner/actions-runner/_work/airflow/airflow/dev/breeze/src/airflow_breeze/commands/release_management_commands.py", line 37, in <module>
    from packaging.version import Version
ModuleNotFoundError: No module named 'packaging'
Breeze should only use limited dependencies when imported (see errors above).
This is the error:

Breeze should only use limited dependencies when imported (see errors above).

You have to make sure that packaging is locally imported not at the top-level. The problem is that when you do auto-completion, the auto-completion script will parse all the commands and click decorators. And in order to do that, it has to import the command modules. And this happens in a very "plain" python environment of python where no dependencies are installed (except rich and click) - the autocomplete script runs outside of the breeze venv (and this is not something we can or should change).

This means that you cannot add 3rd-party imports as top-level imports in the command files. This also allows auto-complete to be fast, because it will not import unnecessary a lot of packages.

This pre-commit failing checks that.

Sweet explanation. Yes, I remember hitting this earlier in the past.

I am pushing a fix for this.

amoghrajesh · 2023-11-30T11:13:11Z

Unable to catch it perhaps because I have some versioning issue in my breeze today..working on fixing it :)

potiuk · 2023-11-30T11:28:58Z

Unable to catch it perhaps because I have some versioning issue in my breeze today..working on fixing it :)

It could also be that you have all breeze packages also installed in default python installation. This might introduce variablity in that particular test, because it assumes, the python used from outside has no external dependencies installed. We have no influence on that one. That's why we also run the checks in CI in "controlled" environment (python is a bare python installation installed from the scratch) to avoid the "works for me" syndrome in this case.

dev/breeze/src/airflow_breeze/commands/release_management_commands.py

eladkal · 2023-12-01T09:31:26Z

dev/README_RELEASE_PROVIDER_PACKAGES.md

 # Check which old packages will be removed (you need Python 3.8+ and dev/requirements.txt installed)
-python ${AIRFLOW_REPO_ROOT}/dev/provider_packages/remove_old_releases.py --directory .
+breeze release-management clean-old-provider-artifacts --directory .


Now that we move to breeze command I believe the comment above of python 3.8 + dev/requirements.txt is no longer relevant ?

BTW I am not sure what is the output of this command?
For the script it was outputed a long list which was very hard to understand. If we can have friendlier output that is easy to read (maybe breakdown by provider name rather than a long mixed list) that can be very helpful!

Yep. Now when the script is part of breeze it could be improved with rich / color output.

It could print Provider Name, list of found artifacts and "command to run". Something like:

Package: apache-airflow-provider-amazon : 1 version found 8.11.0 [success]OK[/]

Package: apache-airflow-provider-google: 2 versions fondd: 9.10.0, 9.10.1. [warning]Removing 9.10.0[/]
Running: svn rm ......

It could even use the built-in --dry-run feature in run_command, this way we could get rid of the --execute flag

So rather than --execute command, the first entry in the RELEASE_PROVIDER_PACKAGES docs could be

breeze release-management clean-old-provider-artifacts --directory . --dry-run

And only then

breeze release-management clean-old-provider-artifacts --directory .

The run_command will automatically use --dry-run

Makes sense around removal of execute flag. I will make the changes. In terms of beautifying, shall we land the major change first and then follow up PR for it?

dev/breeze/src/airflow_breeze/commands/release_management_commands.py

amoghrajesh · 2023-12-01T16:26:11Z

Thank you for your reviews, I will accommodate it soon. Busy with some tasks at work :/

amoghrajesh · 2023-12-02T04:34:06Z

@potiuk I tried the suggestion and I keep hitting this:

dev/breeze/src/airflow_breeze/commands/release_management_commands.py:155:35: TCH004 Move import `packaging.version.Version` out of type-checking block. Import is used for more than type hinting.
Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).

ruff-format........................................................................Failed
- hook id: ruff-format
- files were modified by this hook

I am not so sure why this is happening. Any idea?

dev/breeze/src/airflow_breeze/commands/release_management_commands.py

potiuk · 2023-12-02T11:26:26Z

I am not so sure why this is happening. Any idea?

Becaasue we also need to import in the method where we use it (locally).

amoghrajesh · 2023-12-02T11:31:29Z

I am not so sure why this is happening. Any idea?

Becaasue we also need to import in the method where we use it (locally).

Thank you for the hint. Working on it :)

potiuk

the subprocess.run should be replaced with run_command from utils

amoghrajesh · 2023-12-02T14:23:20Z

the subprocess.run should be replaced with run_command from utils

Somehow I forgot to push the commit :/

Pushed the latest changes now. Should be ok

potiuk

NIce. That looks cool :)

dev/breeze/src/airflow_breeze/utils/common_options.py

dev/breeze/src/airflow_breeze/commands/release_management_commands.py

potiuk · 2023-12-02T14:41:57Z

Few nits regaring usability/parameters color-coding communication.

eladkal

Nice work @amoghrajesh

amoghrajesh · 2023-12-02T18:56:33Z

Thank you. I am making the usability improvements now

amoghrajesh · 2023-12-02T19:01:59Z

@potiuk just pushed a fix adding your comments. Compiles fine 👍🏽

potiuk · 2023-12-02T20:10:16Z

ALMOST :)

--------- Co-authored-by: Jarek Potiuk <jarek@potiuk.com> (cherry picked from commit 9c168b7)

New breeze command to clean up previous provider artifacts

3f5bb48

amoghrajesh requested review from potiuk, ashb and jedcunningham as code owners November 30, 2023 04:58

boring-cyborg bot added the area:dev-tools label Nov 30, 2023

amoghrajesh requested a review from eladkal November 30, 2023 04:58

potiuk reviewed Nov 30, 2023

View reviewed changes

updating instructions and removing old script

2ea51b4

amoghrajesh requested review from kaxil and pierrejeambrun as code owners November 30, 2023 09:54

potiuk reviewed Nov 30, 2023

View reviewed changes

dev/README_RELEASE_PROVIDER_PACKAGES.md Outdated Show resolved Hide resolved

Update dev/README_RELEASE_PROVIDER_PACKAGES.md

66ee003

Co-authored-by: Jarek Potiuk <jarek@potiuk.com>

amoghrajesh added 2 commits November 30, 2023 16:44

moving top level import down

4937408

moving top level import down

ca6d3a4

amoghrajesh requested a review from potiuk November 30, 2023 11:16

potiuk approved these changes Nov 30, 2023

View reviewed changes

potiuk reviewed Dec 1, 2023

View reviewed changes

dev/breeze/src/airflow_breeze/commands/release_management_commands.py Show resolved Hide resolved

eladkal reviewed Dec 1, 2023

View reviewed changes

Lee-W approved these changes Dec 1, 2023

View reviewed changes

dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated Show resolved Hide resolved

fixing checks and review comments

85fa3d6

potiuk reviewed Dec 2, 2023

View reviewed changes

dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated Show resolved Hide resolved

Fixing static checks

8321748

amoghrajesh requested a review from potiuk December 2, 2023 11:31

potiuk requested changes Dec 2, 2023

View reviewed changes

removing subprocess

22d3d9d

amoghrajesh requested review from potiuk and eladkal December 2, 2023 14:23

potiuk approved these changes Dec 2, 2023

View reviewed changes

potiuk reviewed Dec 2, 2023

View reviewed changes

dev/breeze/src/airflow_breeze/utils/common_options.py Outdated Show resolved Hide resolved

potiuk reviewed Dec 2, 2023

View reviewed changes

dev/breeze/src/airflow_breeze/commands/release_management_commands.py Outdated Show resolved Hide resolved

potiuk reviewed Dec 2, 2023

View reviewed changes

dev/breeze/src/airflow_breeze/commands/release_management_commands.py Show resolved Hide resolved

eladkal approved these changes Dec 2, 2023

View reviewed changes

usability improvements from potiuk

0b206e2

amoghrajesh requested a review from potiuk December 2, 2023 19:02

potiuk approved these changes Dec 2, 2023

View reviewed changes

static chekc fix

a2c3f2b

potiuk merged commit 9c168b7 into apache:main Dec 3, 2023

potiuk pushed a commit that referenced this pull request Dec 15, 2023

New breeze command to clean up previous provider artifacts (#35970)

478e87e

--------- Co-authored-by: Jarek Potiuk <jarek@potiuk.com> (cherry picked from commit 9c168b7)

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New breeze command to clean up previous provider artifacts #35970

New breeze command to clean up previous provider artifacts #35970

amoghrajesh commented Nov 30, 2023

potiuk left a comment

amoghrajesh commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

potiuk commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

potiuk commented Nov 30, 2023 •

edited

Loading

amoghrajesh commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

potiuk commented Nov 30, 2023 •

edited

Loading

eladkal Dec 1, 2023 •

edited

Loading

potiuk Dec 1, 2023

amoghrajesh Dec 2, 2023

amoghrajesh commented Dec 1, 2023

amoghrajesh commented Dec 2, 2023

potiuk commented Dec 2, 2023

amoghrajesh commented Dec 2, 2023

potiuk left a comment

amoghrajesh commented Dec 2, 2023

potiuk left a comment

potiuk commented Dec 2, 2023

eladkal left a comment

amoghrajesh commented Dec 2, 2023

amoghrajesh commented Dec 2, 2023

potiuk commented Dec 2, 2023

New breeze command to clean up previous provider artifacts #35970

New breeze command to clean up previous provider artifacts #35970

Conversation

amoghrajesh commented Nov 30, 2023

potiuk left a comment

Choose a reason for hiding this comment

amoghrajesh commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

potiuk commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

potiuk commented Nov 30, 2023 • edited Loading

amoghrajesh commented Nov 30, 2023

amoghrajesh commented Nov 30, 2023

potiuk commented Nov 30, 2023 • edited Loading

eladkal Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

potiuk Dec 1, 2023

Choose a reason for hiding this comment

amoghrajesh Dec 2, 2023

Choose a reason for hiding this comment

amoghrajesh commented Dec 1, 2023

amoghrajesh commented Dec 2, 2023

potiuk commented Dec 2, 2023

amoghrajesh commented Dec 2, 2023

potiuk left a comment

Choose a reason for hiding this comment

amoghrajesh commented Dec 2, 2023

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Dec 2, 2023

eladkal left a comment

Choose a reason for hiding this comment

amoghrajesh commented Dec 2, 2023

amoghrajesh commented Dec 2, 2023

potiuk commented Dec 2, 2023

potiuk commented Nov 30, 2023 •

edited

Loading

potiuk commented Nov 30, 2023 •

edited

Loading

eladkal Dec 1, 2023 •

edited

Loading