[GitHub] [airflow] potiuk commented on a diff in pull request #27829: Improving the release process

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org