potiuk commented on code in PR #27829:
URL: https://github.com/apache/airflow/pull/27829#discussion_r1063217565
##########
.github/workflows/ci.yml:
##########
@@ -747,6 +747,41 @@ jobs:
run: breeze ci fix-ownership
if: always()
+ test-airflow-release-commands:
Review Comment:
I am not so sure about it. The breeze/tests are really "unit tests" and this
is cool for some internal logic an behaviour (for example "selective-checks"
logic is very thoroughly tested by unit tests).
What running the commands directly as "users" would use them, we are testing
more - those are equivalent of "system tests" we have in AIP-47, and for
breeze, just running the commands is much better way of doing it. It tests much
more than any kind of unit tests can do:
* they test parsing of the commands passed
* they produce the colorful output that the user would see so that we can
manually verify them in CI output
* finally - they provide examples of how you should run those commands when
manually releasing, so that you can always refer to the commands we have in CI
(I've used it multiple times in the past - to be sure about it
Most of the breeze command functionality is tested this way (and this is one
of the main reasons breeze Python refactoring was specifically aimed to be able
to run the same commands manually AND in CI - this was one of the decisions we
made when rewriting breeze in Python - to be able to use exactly the same
commands in the CI as we use manually, to avoid regressions. Previously we had
a different approach - where we had "common" libraries in Bash and they were
run by `breeze` bash CLI manually but the same functionality was run via
separate "ci" scripts in CI and this caused multiple, multiple regressions for
the users.
Right now pretty much ALL breeze commands are run in the CI precisely for
that reason as part of the CI build even if they are not `strictly` needed - to
prevent regressions.
A very good recent example of similar regression when we did NOT run breeze
command has been recently fixed in https://github.com/apache/airflow/pull/28683
(@elradkal might confirm it added up to the pains of his release process). We
did not have `breeze release-management generate-issue-content
--only-available-in-dist` command run in CI and when Elad attempted to generate
it at release time, it crashed (because an earlier refactor of mine introduced
bugs).
My philosophy on Breeze and release process is that it shoudl **always**
work when release manager attempts to do it and it should not add friction
(that is the goal at least). And the only way to do be sure about it, is to run
the command that release-manager uses as part of our CI. There is no other sane
way to achieve it.
--
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]
