Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Jul 4, 2020

  • we come back to idea of having one CI workflow
  • cancel and openapi are incorporated into that CI workflow
  • cancel retrieves workflow id automatically (works for forks)
  • static checks are now merged into one job
  • less dependencies between jobs so that waiting is minimised
  • better name for check if tests should be run
  • separated out script for tests should be run check

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@potiuk potiuk force-pushed the spllit-ci-workflow branch 3 times, most recently from 77ba8a6 to 109dd00 Compare July 4, 2020 19:05
@potiuk
Copy link
Member Author

potiuk commented Jul 4, 2020

cc: @kaxil @dimberman @turbaszek @mik-laj @ashb - > I experimented a bit with splitting the CI workflow. I possibly split it a bit too much but I wanted to see if this makes sense. I think it does. We will be able to re-run one group of tests only rather than all of it + I think we can remove the "static-check" -> test dependency now that we have Github Actions - this way tests will start running faster and we can run static checks as a single check as it runs independently.

Let me know what you think.

@potiuk potiuk force-pushed the spllit-ci-workflow branch 2 times, most recently from 5d9cb18 to e147418 Compare July 5, 2020 07:02
@kaxil
Copy link
Member

kaxil commented Jul 5, 2020

cc: @kaxil @dimberman @turbaszek @mik-laj @ashb - > I experimented a bit with splitting the CI workflow. I possibly split it a bit too much but I wanted to see if this makes sense. I think it does. We will be able to re-run one group of tests only rather than all of it + I think we can remove the "static-check" -> test dependency now that we have Github Actions - this way tests will start running faster and we can run static checks as a single check as it runs independently.

Let me know what you think.

Yeah I think we can combine the 2 static checks to one.

@dimberman
Copy link
Contributor

I completely agree with removing the static check dependency. That was a necessary workaround in Travis, (though maybe we can fail-fast the rest of tests if static tests fail)

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2020

Ah yeah... @dimberman Failing fast is a good idea. I think we can do it similarly to the Cancel workflow. And @ashb - in another beam-infra project (where I helped to introduce Github Actions we found a way to get the workflow id using Github API so we can reuse it here :). Synergies.

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2020

Yeah I think we can combine the 2 static checks to one.

@kaxil -> yeah. This is only good if we remove the static -> test dependency otherwise one long static check will take too much time. And also once we complete pylint migration (Soon! We are 13 files left!) we might turn off "serialize" in pylint checks (it was needed because it was detecting cyclic dependencies somewhat randomly. This should speed -up pylint checks considerably (at least 2x or as many processors we will have available in Github Actions.

@potiuk potiuk force-pushed the spllit-ci-workflow branch 6 times, most recently from e93f369 to 00081e2 Compare July 16, 2020 18:58
@potiuk potiuk marked this pull request as ready for review July 16, 2020 20:06
@potiuk
Copy link
Member Author

potiuk commented Jul 16, 2020

@kaxil @ashb @turbaszek @mik-laj . Here it is - I split the Ci workflow in smaller ones. Each workflow is independent and can be rerun separately as needed. cancel steps are incorporated in each workflow. It has some benefits in terms of independence and rerun ability, but I am not sure if we should split in general - it introduces more entities in action history (but not more jobs)

* we come back to idea of having one CI workflow
* cancel and openapi are incorporated into that CI workflow
* cancel retrieves workflow id automatically (works for forks)
* static checks are now merged into one job
* less dependencies between jobs so that waiting is minimised
* better name for check if tests should be run
* separated out script for tests should be run check
@potiuk potiuk force-pushed the spllit-ci-workflow branch from 00081e2 to 50dc40b Compare July 17, 2020 07:36
@potiuk
Copy link
Member Author

potiuk commented Jul 17, 2020

I did some experimenting and spliiting to separate workflows brings far more problems than benefits. I reorganized the CI tests and put them back in ci.yml (including cancel workflow).

  • we come back to idea of having one CI workflow
  • cancel and openapi are incorporated into that CI workflow
  • cancel retrieves workflow id automatically (works for forks)
  • static checks are now merged into one job
  • less dependencies between jobs so that waiting is minimised
  • better name for check if tests should be run
  • separated out script for tests should be run check

@potiuk potiuk changed the title Spllit ci workflow Reorganizing of CI tests Jul 17, 2020
@potiuk potiuk requested review from ashb, kaxil, mik-laj and turbaszek July 17, 2020 07:44
Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me 👌

@potiuk potiuk merged commit 496ed6f into apache:master Jul 17, 2020
@potiuk potiuk deleted the spllit-ci-workflow branch July 17, 2020 08:30
scrambldchannel pushed a commit to scrambldchannel/airflow that referenced this pull request Jul 17, 2020
* we come back to idea of having one CI workflow
* cancel and openapi are incorporated into that CI workflow
* cancel retrieves workflow id automatically (works for forks)
* static checks are now merged into one job
* less dependencies between jobs so that waiting is minimised
* better name for check if tests should be run
* separated out script for tests should be run check
@potiuk potiuk added this to the Airflow 1.10.12 milestone Jul 22, 2020
@potiuk potiuk added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 22, 2020
potiuk added a commit that referenced this pull request Jul 22, 2020
* we come back to idea of having one CI workflow
* cancel and openapi are incorporated into that CI workflow
* cancel retrieves workflow id automatically (works for forks)
* static checks are now merged into one job
* less dependencies between jobs so that waiting is minimised
* better name for check if tests should be run
* separated out script for tests should be run check

(cherry picked from commit 496ed6f)
kaxil pushed a commit that referenced this pull request Aug 11, 2020
* we come back to idea of having one CI workflow
* cancel and openapi are incorporated into that CI workflow
* cancel retrieves workflow id automatically (works for forks)
* static checks are now merged into one job
* less dependencies between jobs so that waiting is minimised
* better name for check if tests should be run
* separated out script for tests should be run check

(cherry picked from commit 496ed6f)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
* we come back to idea of having one CI workflow
* cancel and openapi are incorporated into that CI workflow
* cancel retrieves workflow id automatically (works for forks)
* static checks are now merged into one job
* less dependencies between jobs so that waiting is minimised
* better name for check if tests should be run
* separated out script for tests should be run check

(cherry picked from commit 496ed6f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants