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.

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

GitHub Actions: smart run strategy #937

Merged
merged 4 commits into from
Mar 10, 2021
Merged

GitHub Actions: smart run strategy #937

merged 4 commits into from
Mar 10, 2021

Conversation

amureki
Copy link
Member

@amureki amureki commented Mar 9, 2021 •
edited

Description of the Change

Previously, every pull request was running actions twice, since two signals were fired - push commit in the PR and new PR itself.

https://docs.github.com/en/actions/reference/events-that-trigger-workflows#webhook-events

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

Rust Saiargaliev added 2 commits March 9, 2021 20:53
GHA: run CI on PRs and pushes to master only
fc98c6f 
Previously, every pull request was running actions twice, since two signas were fired - push commit in the PR and new PR itself.
Update CHANGELOG
6de9e34
@amureki amureki self-assigned this Mar 9, 2021
@amureki amureki changed the title Gha smart run strategy GitHub Actions: smart run strategy Mar 9, 2021
@codecov
Copy link

codecov bot commented Mar 9, 2021 •
edited

Codecov Report

Merging #937 (8ab52f5) into master (e09e7d4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #937   +/-   ##
=======================================
  Coverage   94.94%   94.94%           
=======================================
  Files          30       30           
  Lines        1423     1423           
=======================================
  Hits         1351     1351           
  Misses         72       72

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e09e7d4...8ab52f5. Read the comment docs.

@amureki amureki mentioned this pull request Mar 9, 2021
Merged
5 tasks
n2ygk
n2ygk requested changes Mar 9, 2021
View reviewed changes
CHANGELOG.md Outdated
@@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [unreleased]

### Fixed
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your contribution!

I don't believe this goes in the CHANGELOG though as it's not a user-visible change.

Copy link
Member

Choose a reason for hiding this comment

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

Oh and please add yourself to AUTHORS

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, Alan, for a fast review!
I've added requested changes.

@amureki amureki requested a review from n2ygk March 9, 2021 21:33
n2ygk
n2ygk approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

Thanks for this speed up

@n2ygk n2ygk merged commit 062408a into master Mar 10, 2021
@auvipy auvipy deleted the gha_smart_run_strategy branch March 10, 2021 07:06
@jezdez
Copy link
Member

jezdez commented Mar 10, 2021 •
edited

@amureki Please be so kind and revert this change, the use of both events is deliberate to account for all kinds of state changes a pull request can have, such as pushes happening to branches in forked repos that a pull request has been opened against.

@n2ygk Please let me know if other pull requests come in about the GitHub Actions configuration so I can do a review. Thank you!

@amureki
Copy link
Member Author

amureki commented Mar 10, 2021

@jezdez hey Jannis! I am sorry for bringing some confusion here. ATM trying to understand which states are not covered by this configuration.
I forked the repository and created a test PR here:
#938

image

After seeing green actions build, I dropped another commit to see if it will retrigger actions event, and it did:
image

I am creating a revert PR in any case, to not possibly block anyone.

amureki pushed a commit that referenced this pull request Mar 10, 2021
Revert "GitHub Actions: smart run strategy (#937)"
47a0748 
This reverts commit 062408a.
This was referenced Mar 10, 2021
Merged
Closed
@jezdez
Copy link
Member

jezdez commented Mar 10, 2021 •
edited

@jezdez hey Jannis! I am sorry for bringing some confusion here. ATM trying to understand which states are not covered by this configuration.
I forked the repository and created a test PR here:
#938

image

After seeing green actions build, I dropped another commit to see if it will retrigger actions event, and it did:
image

I am creating a revert PR in any case, to not possibly block anyone.

Thanks for willing to dive into this, and you're right this seems to work. Sadly I don't have a working setup anymore where I was able to reproduce the edge cases with pull requests when I prepared the move from Travis to GitHub Actions. I appreciate your interest in figuring this out though!

IIRC it was a combination of pull requests first being closed and reopened again and then not receiving push events anymore for changes in a fork branch. GitHub may have fixed it, or not, I can't really say without digging in again, their changelogs are not very detailed I'm afraid and last time the GitHub forum had a few people with similar issues. Personally I'm mostly concernd about consistency in running GitHub Actions for all Jazzband projects.

Thank you for resurfacing this since if it turns out to be not reproducable anymore I can update all the other Jazzband projects as well, which obviously would be a really nice thing to do, ecologically speaking. I'd appreciate a roll-back in the meantime though.

jezdez pushed a commit that referenced this pull request Mar 10, 2021
Revert "GitHub Actions: smart run strategy (#937)" (#939)
d6d1f99 
This reverts commit 062408a.
amureki pushed a commit that referenced this pull request Mar 10, 2021
Merge remote-tracking branch 'origin/master' into drop_python_35
46d250d 
* origin/master:
  Revert "GitHub Actions: smart run strategy (#937)" (#939)
@amureki
Copy link
Member Author

amureki commented Mar 10, 2021 •
edited

@jezdez thank you for the detailed explanation! ✨

Whenever you have some time and will digging into this, feel free to drop me a line here or on amureki @ hey.com with some links to GitHub issues or forum topics that are related to the issue.
I'd be happy then to follow up on that and try to make some test sandbox to try to reproduce different use cases, so we can move forward and possibly optimize CI runs (also do to this across all Jazzband projects).

Update: from what I see in GitHub Actions webhooks documentation, adding pull request event covers all PR actions, including:

  • opened
  • edited
  • closed
  • reopened
  • synchronize
  • ready_for_review

So, it might be that they fixed/added some time after the issues were appearing, but we'd still need to validate that.

This was referenced Mar 15, 2021
Closed
Closed
Closed
Closed
Closed
Closed
@obulat obulat mentioned this pull request Apr 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n2ygk n2ygk n2ygk approved these changes

@MattBlack85 MattBlack85 Awaiting requested review from MattBlack85

Assignees

@amureki amureki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@amureki @jezdez @n2ygk