8000 Split all testing from `publish_draft_release` workflow and filter execution by labels on the PRs by Garandor · Pull Request #743 · Manta-Network/Manta · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Split all testing from publish_draft_release workflow and filter execution by labels on the PRs #743

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Aug 30, 2022
Merged

Split all testing from publish_draft_release workflow and filter execution by labels on the PRs #743

merged 22 commits into from
Aug 30, 2022

Conversation

Garandor
Copy link
Contributor
@Garandor Garandor commented Aug 8, 2022 •
edited
Loading

Signed-off-by: Adam Reif Garandor@manta.network

Description

Refactors the publish_draft_release workflow so it does only what it's meant to do - publish a draft release.

Changes in functionality:

  1. Only run dolphin integration test if the PR touches the dolphin RT ( A-dolphin on PR )
  2. Only run calamari with its long integration test if the PR touches calamari ( A-calamari on PR )
  3. Only run docker_image_test with calamari_integration_test
  4. only run runtime_upgrade_test on release branches and merge to manta
  5. we run everything and the kitchen sink when it's merged to manta

All testing happens in a runtime-specific file and is triggered only when the PR touches the corresponding runtime as defined by the PR label.

I think this saves

  • developer sanity maintaining the thing
  • aws compute usage
  • calamari and dolphin test workflows can be easily diffed against each other

Please discuss and contribute!

Also removes the manta runtime docker build from releases as these images are not currently used anywhere

closes #608

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Added one label out of the L- group to this PR
  • Added one or more labels out of A- and C- groups to this PR
  • Re-reviewed Files changed in the Github PR explorer.

Situational Notes:

  • If adding functionality, write unit tests!
  • If importing a new pallet, choose a proper module index for it, and allow it in BaseFilter. Ensure every extrinsic works from front-end. If there's corresponding tool, ensure both work for each other.
  • If needed, update our Javascript/Typescript APIs. These APIs are officially used by exchanges or community developers.
  • If modifying existing runtime storage items, make sure to implement storage migrations for the runtime and test them with try-runtime. This includes migrations inherited from upstream changes, and you can search the diffs for modifications of #[pallet::storage] items to check for any.
  • If runtime changes, need to update the version numbers properly:
    • authoring_version: The version of the authorship interface. An authoring node will not attempt to author blocks unless this is equal to its native runtime.
    • spec_version: The version of the runtime specification. A full node will not attempt to use its native runtime in substitute for the on-chain Wasm runtime unless all of spec_name, spec_version, and authoring_version are the same between Wasm and native.
    • impl_version: The version of the implementation of the specification. Nodes are free to ignore this; it serves only as an indication that the code is different; as long as the other two versions are the same then while the actual code may be different, it is nonetheless required to do the same thing. Non-consensus-breaking optimizations are about the only changes that could be made which would result in only the impl_version changing.
    • transaction_version: The version of the extrinsics interface. This number must be updated in the following circumstances: extrinsic parameters (number, order, or types) have been changed; extrinsics or pallets have been removed; or the pallet order in the construct_runtime! macro or extrinsic order in a pallet has been changed. You can run the metadata_diff.yml workflow for help. If this number is updated, then the spec_version must also be updated
  • Verify benchmarks & weights have been updated for any modified runtime logics

split up the workflow and filter runs by PR labels
6714202 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor added A-calamari Area: Issues and PRs related to the Calamari Runtime A-ci Area: Continuous Integration A-dolphin Area: Issues and PRs related to the Dolphin Runtime C-cleanup Category: Issues documenting cleanup or PRs that clean code up L-changed Log: Issues and PRs related to changes labels Aug 8, 2022
@Garandor Garandor self-assigned this Aug 8, 2022
@Garandor
Copy link
Contributor Author
Garandor commented Aug 8, 2022

It looks like having if: on each job is spammy in the list, anyone know if if: is allowed above the job: level in gh actions?

@Garandor Garandor removed their assignment Aug 8, 2022
Fix linter, test if before jobs
588ad93 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor
Copy link
Contributor Author
Garandor commented Aug 8, 2022 •
edited
Loading

There are likely duplicated tests in this PR. I'm opening this as a first attempt for discussion. please feel free to pick this up and push more commits

Most likely the runtime_upgrade_test can be refactored out of these as well

fix trigger
1e6965d 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor changed the title Split all testing from publish_draft_relese workflow and filter execution by labels on the PRs Split all testing from publish_draft_release workflow and filter execution by labels on the PRs Aug 8, 2022
fix lint
5a5b848 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor added this to the v3.4.0 milestone Aug 8, 2022
@Garandor
Copy link
Contributor Author
Garandor commented Aug 8, 2022

One more optimization could be to use https://github.com/rust-lang/docker-rust-nightly

as in runs_on: rustlang/rust:nightly instead of running rustup at our compute cost on every run

add missed if
024db0c 
Signed-off-by: Adam Reif <Garandor@manta.network>
ghzlatarev
ghzlatarev previously approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor
@ghzlatarev ghzlatarev left a comment

Choose a reason for hiding this comment

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

lgtm. the only reason it was like this was to not dupe the code, but we've ended up doing that in a lot of other places and we can take care of that problem separately. Probably less painful than maintaining this monstrosity.

Apokalip
Apokalip previously approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor
@Apokalip Apokalip left a comment

Choose a reason for hiding this comment

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

lgmt! I don't know what part of this will be moved to the type script tests, but as seeing how the signer is now on my plate too, that will take some time.

@Garandor Garandor added the DO-NOT-MERGE Labels a PR that should not be merged label Aug 9, 2022
@ghzlatarev ghzlatarev self-requested a review August 23, 2022 15:07
manually re-apply v3.3.0 changes to pdr workflow
469b02d 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor dismissed stale reviews from Apokalip and ghzlatarev via 469b02d August 26, 2022 00:11
@Garandor Garandor requested a review from Apokalip August 26, 2022 02:36
Adam Reif added 2 commits August 25, 2022 21:56
fix lint
18cc4d8 
Signed-off-by: Adam Reif <Garandor@manta.network>
Run yamllint before doing all the rust steps for faster failure
76870ed 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor added A-dolphin Area: Issues and PRs related to the Dolphin Runtime A-calamari Area: Issues and PRs related to the Calamari Runtime labels Aug 26, 2022
@ghzlatarev
Copy link
Contributor

Not sure if this does something for the build times, it's always building the same binary but not it needs to build it twice sometimes.
Overall 50% - 50% on the core idea of doing less CI and addon with labels, instead of doing all CI and limiting based on some conditions. I guess it's more compute efficient to do the former, but then we also have more human overhead of making sure labeling is correct. And i'm really not sure this is a bottleneck.
I think optimally you want a workflow that just builds the binary and then the split (for easier maintenance) test-workflows can wait for the first workflow and download the binary and then do their thing.

@Garandor
Copy link
Contributor Author
Garandor commented Aug 26, 2022 •
edited
Loading

Not sure if this does something for the build times, it's always building the same binary but not it needs to build it twice sometimes.

That's why i'm saying the last big optimization is to make a reusable workflow ( #320 ) and that runs once and all others use the common build output from that.
I think it's worth the extra compute for now given that it

  • saves time if the integration tests and RT upgrades are not run on every commit
  • is significantly more maintainable

@Garandor Garandor removed the E-hours few hours of effort, small change label Aug 26, 2022
@github-actions

This comment was marked as resolved.

Sign in to view
remove unneeded deps on parse_runtime and build_runtime
2605197 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor force-pushed the garandor/split_draft_release_ci branch from d387c95 to 2605197 Compare August 26, 2022 12:08
@Garandor
Copy link
Contributor Author

Plus I for one can't reason about the dependencies of the CI jobs in a >1k spaghetti code workflow.
Did you know that calamari-integration-test depends on buiild-runtimes when it doesn't actually use it?

There seems to be a problem with aws scheduling tho
https://github.com/Manta-Network/Manta/runs/8036038006?check_suite_focus=true#step:2:29

Error: aws scheduled instance starting error
Error: VcpuLimitExceeded: You have requested more vCPU capacity than your current vCPU limit of 2064 allows for the instance bucket that the specified instance type belongs to. Please visit http://aws.amazon.com/contact-us/ec2-request to request an adjustment to this limit.

@ghzlatarev
Copy link
Contributor
ghzlatarev commented Aug 26, 2022 •
edited
Loading

Not sure if this does something for the build times, it's always building the same binary but not it needs to build it twice sometimes.

That's why i'm saying the last big optimization is to make a reusable workflow and that runs once and all others use the common build output from that. I think it's worth the extra compute for now given that it

  • saves time if the integration tests and RT upgrades are not run on every commit
  • is significantly more maintainable

I agree it would make things easier to maintain, just saying it won't do much about build times, which you included as a reason in the description.
My only concern here is the extra human effort of doing the labeling correctly, so the right workflows are triggered, and then the reviewers reviewing it correctly. So I'd keep the split of the workflows but no the labeling.

@Garandor
Copy link
Contributor Author
Garandor commented Aug 26, 2022 •
edited
Loading

I agree it would make things easier to maintain, just saying it won't do much about build times, which you included as a reason in the description.

Looking at the build times, I agree. The check_tests workflow that always runs takes ~90mins which is the same or more as all the integration tests which run in parallel, so skipping those won't speed up the build unfortunately :(
It will however save ~1 hour of pointlessly wasted cloud compute per skipped integration test

My only concern here is the extra human effort of doing the labeling correctly, so the right workflows are triggered, and then the reviewers reviewing it correctly. So I'd keep the split of the workflows but no the labeling.

We're labelling these anyway because our changelog depends on them being set correctly and

  • laziness is not an excuse ( we need to add it to the PR template tho )
  • the workflows will run on merge at the latest ( whether labelled or not )

Adam Reif added 3 commits August 26, 2022 07:42
always run the stop jobs
d104f08 
Signed-off-by: Adam Reif <Garandor@manta.network>
Run stop jobs only if start job succeded and wasn't skipped
3b5ccf4 
Signed-off-by: Adam Reif <Garandor@manta.network>
bump
4dd2fa8 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor force-pushed the garandor/split_draft_release_ci branch from 6ddc38f to a85e1ee Compare August 26, 2022 13:23
Revert "Run stop jobs only if start job succeded and wasn't skipped"
de59c15 
This reverts commit 3b5ccf4.

Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor Garandor force-pushed the garandor/split_draft_release_ci branch from a85e1ee to de59c15 Compare August 26, 2022 20:41
@Dengjianping
Copy link
Contributor
Dengjianping commented Aug 29, 2022 •
edited
Loading

I saw we're building docker image for manta(actually it's the same image as calamari's). It doesn't make much sense to me.
https://hub.docker.com/r/mantanetwork/manta/tags

We should not push manta image to dockerhub.
We can enable it when manta wins the auction.

publish calamari docker only
aa5a630 
Signed-off-by: Adam Reif <Garandor@manta.network>
@Garandor
Copy link
Contributor Author
Garandor commented Aug 29, 2022 •
edited
Loading

I saw we're building docker image for manta(actually it's the same image as calamari's). It doesn't make much sense to me. https://hub.docker.com/r/mantanetwork/manta/tags

We should not push manta image to dockerhub. We can enable it when manta wins the auction.

Thx for the suggestion, i added it

in the future, pls add new stuff that isn't related with the current PR as a new issue, I'd rather not have a PR that's meant to be about one thing become larger in scope ( and delay being merged )

@ghzlatarev
Copy link
Contributor
ghzlatarev commented Aug 29, 2022 •
edited
Loading

I agree it would make things easier to maintain, just saying it won't do much about build times, which you included as a reason in the description.

Looking at the build times, I agree. The check_tests workflow that always runs takes ~90mins which is the same or more as all the integration tests which run in parallel, so skipping those won't speed up the build unfortunately :( It will however save ~1 hour of pointlessly wasted cloud compute per skipped integration test

My only concern here is the extra human effort of doing the labeling correctly, so the right workflows are triggered, and then the reviewers reviewing it correctly. So I'd keep the split of the workflows but no the labeling.

We're labelling these anyway because our changelog depends on them being set correctly and

  • laziness is not an excuse ( we need to add it to the PR template tho )
  • the workflows will run on merge at the latest ( whether labelled or not )

I'm 50/50 so cool to try it, but as you mention let's add a line to the PR template as a reminder that labeling needs to be done correctly, otherwise CI won't run. As well as a reminder that just because the code diff doesn't include some runtime, it doesn't mean that CI shouldn't be run for it (eg: changing the CI integration test workflows themselves)

Adam Reif added 2 commits August 29, 2022 07:54
Merge branch 'manta' into garandor/split_draft_release_ci
bb3e7b4 
Signed-off-by: Adam Reif <Garandor@manta.network>
Mention runtime label in PR template
60be5a9 
Signed-off-by: Adam Reif <Garandor@manta.network>
@ghzlatarev ghzlatarev merged commit 07daf89 into manta Aug 30, 2022
@ghzlatarev ghzlatarev deleted the garandor/split_draft_release_ci branch August 30, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dengjianping Dengjianping Dengjianping approved these changes

@ghzlatarev ghzlatarev ghzlatarev approved these changes

@flame4 flame4 Awaiting requested review from flame4

@Apokalip Apokalip Awaiting requested review from Apokalip

Assignees

@Garandor Garandor

Labels
A-calamari Area: Issues and PRs related to the Calamari Runtime A-ci Area: Continuous Integration A-dolphin Area: Issues and PRs related to the Dolphin Runtime C-cleanup Category: Issues documenting cleanup or PRs that clean code up L-changed Log: Issues and PRs related to changes
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

Investigate splitting up the publish_draft_releases workflow
4 participants
@Garandor @ghzlatarev @Dengjianping @Apokalip
0