sjyangkevin opened a new pull request, #76870:
URL: https://github.com/apache/airflow/pull/76870
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull
request.
Write a good git commit message following this guide:
https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the
documentation.
For user-facing UI changes, please attach before/after screenshots (or a
short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for
a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we
sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
### Why
While testing the Python Client 3.4.0rc1, I followed the "Non-interactive
smoke test" section in `dev/README_RELEASE_PYTHON_CLIENT.md`. Both documented
commands raised errors. The airflowctl release guide has the same section, with
the same problem.
There are two issues:
1. **The version never reaches the script.** The docs set `CLIENT_VERSION` /
`CLIENT_WHEEL` (`CTL_VERSION` / `CTL_WHEEL` for airflowctl) on the host, in
front of `breeze shell`. Breeze only forwards a fixed list of environment
variables into the container, so the script sees an empty value and exits with
`ERROR: set CLIENT_VERSION ... or CLIENT_WHEEL`.
2. **The wheel path does not exist.** The docs point the wheel to
`/opt/airflow/dist`, but Breeze mounts the host `./dist` folder at `/dist`
(`scripts/ci/docker-compose/files.yml`). `dist` is not one of the source
folders mounted under `/opt/airflow`. The interactive section of the same
README already uses `/dist`. This issue is hidden behind the first one, it only
shows up once the variable is passed in.
### What
Set the variable inside the quoted command, so it is set in the container,
and use `/dist` for the wheel. The same change is applied to both READMEs and
to the usage comment at the top of both scripts. No script logic is changed.
```shell
# Before
CLIENT_VERSION=${VERSION_RC} breeze shell --load-example-dags --backend
sqlite \
"bash dev/verify_python_client_rc.sh"
CLIENT_WHEEL=/opt/airflow/dist/apache_airflow_client-${VERSION}-py3-none-any.whl
\
breeze shell --load-example-dags --backend sqlite "bash
dev/verify_python_client_rc.sh"
# After
breeze shell --load-example-dags --backend sqlite \
"CLIENT_VERSION=${VERSION_RC} bash dev/verify_python_client_rc.sh"
breeze shell --load-example-dags --backend sqlite \
"CLIENT_WHEEL=/dist/apache_airflow_client-${VERSION}-py3-none-any.whl
bash dev/verify_python_client_rc.sh"
```
### Why not let Breeze forward the variables
From my understanding, Breeze keeps the variables it forwards in one place
(`ShellParams.env_variables_for_docker_commands`), and writes them into a
`_generated_docker_compose.env` file that concurrent Breeze runs share. A
variable that only some runs set can be dropped by another run, which is what
happened to `PYTHONWARNINGS` in #72226. Forwarding them through `base.yml` like
`PYTHONWARNINGS` would work, but it adds names that only two release helper
scripts use to every Breeze container, and it still doesn't fix the wheel path.
Setting the variable inside the quoted command is plain shell and needs no
Breeze change.
### Testing
I tested on `main` with Breeze, using the Python Client `3.4.0rc1` from
PyPI, the SVN wheel (SHA-512 checked) and airflowctl `0.1.5`. Following the
docs as written, the scripts exit 2 because the variable is unset in the
container, and once the variable is passed in, the wheel install fails because
`/opt/airflow/dist` does not exist. With the fixed commands, the wheel installs
from `/dist`, and the client and airflowctl smoke tests both print OK. On
`main`, the client test can still fail with a 404 right after the install,
because it starts before the example Dags are parsed; this is unrelated to this
change, and I will look at it separately.
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR,
please
change below checkbox to `[X]` followed by the name of the tool, uncomment
the "Generated-by".
-->
- [X] Yes — Claude Code (Opus 5.5)
Generated-by: Claude Code (Opus 5.5) following [the
guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information. Note: commit author/co-author name and email in commits
become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License
Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment:
`{pr_number}.significant.rst`, in
[airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
You can add this file in a follow-up commit after the PR is created so you
know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
