You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/04/27 21:50:53 UTC
[GitHub] [airflow] ashb opened a new pull request #8594: Reduce duplication of functions called in ci scripts
ashb opened a new pull request #8594:
URL: https://github.com/apache/airflow/pull/8594
Every place we called rebuild_ci_image_if_needed we were also calling
preprepare_ci_build -- there's no need.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #8594:
URL: https://github.com/apache/airflow/pull/8594#issuecomment-620638052
My main goal in here was trying to remove source of mistakes by developers when writing CI scripts -- i.e. from what I can work out it never makes sense to call `rebuild_ci_image_if_needed` without first calling `preprepare_ci_build` -- since that is possible to get wrong, I thought to move it inside the function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #8594:
URL: https://github.com/apache/airflow/pull/8594#issuecomment-620650779
> My problem when doing this was that there are no comments or docs for these functions, so I have to work out what it does, and what state they require to be set up in advance to work.
I can fix that easily. I have already started to add unit tests for those functions - but wanted to complete the prod image. But I absolutely want to add more meaningful documentation and unittests to make sure that the scripts continue to work - they are fairly complex and having unit tests will help to make sure they are still working.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #8594:
URL: https://github.com/apache/airflow/pull/8594#issuecomment-620649534
> My main goal in here was trying to remove source of mistakes by developers when writing CI scripts -- i.e. from what I can work out it never makes sense to call `rebuild_ci_image_if_needed` without first calling `preprepare_ci_build` -- since that is possible to get wrong, I thought to move it inside the function.
Yeah. but sometimes there is a need to do something between those two tasks - because the first one sets variables that are needed to be used before the build image is run. This is the main reason why those two are split.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #8594:
URL: https://github.com/apache/airflow/pull/8594#issuecomment-620615942
I can fix up the breeze issue, somehow missed that in my grep.
The purpose of this PR was that I was experimenting creating a new CI script (for my encrypted secrets work) and I noticed a lot of duplication in the scripts - when the same sequence of calls is repeated 13. If I get the rest of the uses in breeze too, do you think this would be worth it to make the `ci_*` scripts less duplicated?
My problem when doing this was that there are no comments or docs for these functions, so I have to work out what it does, and what state they require to be set up in advance to work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #8594:
URL: https://github.com/apache/airflow/pull/8594#issuecomment-620545404
The change is not compleete and I do not think it's worth it. I am not sure what it would bring.
This method run separately and explicitly is there to prepare variables for CI build - not run the build itself. and it is deliberately named like that to make sure that it is "preparation" rather than "build". I prefer single-responsibility principle as it makes it cleaner where things have to be changed.
I am already using that single-responsibility principle - you probably missed all the usage of those methods in the "breeze" script as it does not end with .sh
The methods and builds of images can be triggered in various places:
* ci_scripts (used in Github Actions/Travis)
* pre-commit scripts
* breeze
There are a number of places in `breeze` script that the method is used separately - just to set the variables and act on those variables before the build happens. There are things happening between that method and running the build itself - for example cleaning the build cache between prepare and build or separation of the prepare/build in breeze when we choose which command should be executed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #8594:
URL: https://github.com/apache/airflow/pull/8594#issuecomment-642568389
Code changed, not needed anymore
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb closed pull request #8594: Reduce duplication of functions called in ci scripts
Posted by GitBox <gi...@apache.org>.
ashb closed pull request #8594:
URL: https://github.com/apache/airflow/pull/8594
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org