Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

github: partition the github action workflows #8489

Merged
merged 1 commit into from Apr 15, 2020

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Apr 14, 2020

This commit partitions the spread systems into multiple actions
that are each unit tested with a different go version. This
allows independent restarts.

@mvo5 mvo5 force-pushed the split-actions branch 6 times, most recently from 77bb57e to e13db81 Compare April 14, 2020 09:58
@mvo5 mvo5 added ⛔ Blocked Skip spread Indicate that spread job should not run and removed ⛔ Blocked labels Apr 14, 2020
@mvo5 mvo5 force-pushed the split-actions branch 2 times, most recently from 211895d to d313dd1 Compare April 14, 2020 11:04
@mvo5 mvo5 removed Skip spread Indicate that spread job should not run ⛔ Blocked labels Apr 14, 2020
This commit partitions the spread systems into multiple actions
that are each unit tested with a different go version. This
allows independent restarts and also earlier failures if a
specific go version does break the tests.

The common code in unit-tests-* should be shared somehow,
maybe via a custom action. But that can be done in a followup.
@mvo5 mvo5 marked this pull request as ready for review April 14, 2020 11:21
@anonymouse64 anonymouse64 self-requested a review April 14, 2020 19:40
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Partial review.

I'd like to use the opportunity to restart this a few more times to see the new workers operate successfully.

on:
pull_request:
branches: [ "master", "release/**" ]
jobs:
unit-tests:
unit-tests-1_10:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each workflow has private identifiers, you can just keep this at unit-tests without ill effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did initially but iirc it looked not super nice in the web UI, it was a bit confusing what was running where etc.

@@ -30,15 +30,21 @@ jobs:
with:
path: /var/cache/apt
key: var-cache-apt-{{ hashFiles('**/debian/control') }}
- name: Run "apt update"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this moved out to a separate step? On a cache hit you avoid this entire section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the archive/ppa changes all the time so "apt install foo" will not work after a couple of days because the cache has v1.22 but the archive has moved on to 1.23 but the cache was not updated because nothing in debian/control has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think this is needed because of a hash key collision somehow

# golang 1.10 is used on 14.04/16.04/18.04
- name: Install the go snap
run: |
sudo snap install --classic --channel=1.10 go
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is excellent :) Feels like we are really doing something right :)

@bboozzoo
Copy link
Collaborator

Does this need some repo level switches to be changed? There is a bunch of spread-stable jobs that are marked as required.

@mvo5
Copy link
Contributor Author

mvo5 commented Apr 15, 2020

@bboozzoo Yes, it needs an update to the required actions, I will do that once this is ready to land.

@@ -1,18 +1,18 @@
name: Tests
name: Tests go1.10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should name the workflows go1.xx unit and spread[-unstable] native?

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Let's merge this.

Mvo: remember to adjust required status checks.

@mvo5 mvo5 merged commit 6b00f7b into snapcore:master Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants