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

Avoid duplicate and useless checks #101

Merged
merged 7 commits into from
Dec 8, 2022
Merged

Avoid duplicate and useless checks #101

merged 7 commits into from
Dec 8, 2022

Conversation

btlogy
Copy link
Contributor

@btlogy btlogy commented Dec 6, 2022

As described in #94, we may want to avoid the integration test to run twice upon pull_request.

This PR introduce a pre-check to avoid duplicate test to be run on the same content.
In addition, this PR also provides:

  • a manual trigger to test any branch w/o having to create a PR.
  • a similar pre-check to skip code analysis based on the same criteria
  • new file, workflow and job names to improve clarity

Code Review Checklist (to be filled out by reviewer)

  • Description accurately reflects what changes are being made.
  • Description explains why the changes are being made (or references an issue containing one).
  • The PR appropriately sized.
  • New code has enough tests.
  • New code has enough documentation to answer "how do I use it?" and "what does it do?".
  • Existing documentation is up-to-date, if impacted.

@btlogy
Copy link
Contributor Author

btlogy commented Dec 6, 2022

I've not found any existing documentation of our use of GitHub actions in this repository.
Therefore, I'm not planning to update or create any in relation to this PR.

@btlogy
Copy link
Contributor Author

btlogy commented Dec 6, 2022

Looking at the check results attached to this PR, it seems like the test has been run only once as requested.

@btlogy btlogy mentioned this pull request Dec 6, 2022
3 tasks
@btlogy btlogy marked this pull request as ready for review December 6, 2022 10:47
@btlogy btlogy requested review from vu3rdd and donpui December 6, 2022 10:54
@btlogy btlogy changed the title Trigger test for PR and push on main only Trigger test for any pull_request and push on main branch only Dec 6, 2022
@btlogy btlogy mentioned this pull request Dec 6, 2022
6 tasks
@donpui donpui requested a review from JustusFT December 6, 2022 11:31
@donpui
Copy link
Contributor

donpui commented Dec 6, 2022

Please confirm if I correctly understand:

  • main.yml CI will run on any pull_request to any branch
  • main.yml CI will run only on push to main branch (in reality we are not directly pushing to main).

If so, then it is fine by me (we will need probably apply the same in few other repos: destiny, w-w)

@btlogy
Copy link
Contributor Author

btlogy commented Dec 6, 2022

* main.yml CI will run on any pull_request to any branch
* main.yml CI will run only on push to main branch (in reality we are not directly pushing to main).

That is my understanding indeed.
And also that merging a pull_request is triggering a push event on main branch.

What might no longer work after this PR is the automatic run of the test when somebody pushes more commits in a pull_request that is already open.
And for that matter, I hope the manual trigger could help.

If so, then it is fine by me (we will need probably apply the same in few other repos: destiny, w-w)

That might depends on who the actions are organized in those repositories. We do not seem to have a standard approach.
For instance, in some case there is one action file to cover many jobs (i.e.: build, test and deploy), and it might be more tricky to filter the events then.

Let see how it works for this case first.

@btlogy
Copy link
Contributor Author

btlogy commented Dec 6, 2022

@donpui, let's merge this PR once you'll have ticked (or not) the check-boxes in the description.
And see if the test is triggered in the main branch after the merge.
Though, I suspect we should check if this happens again for the next PR too.

@donpui
Copy link
Contributor

donpui commented Dec 6, 2022

What might no longer work after this PR is the automatic run of the test when somebody pushes more commits in a pull_request that is already open.
And for that matter, I hope the manual trigger could help.

That part might not be good, as we tend to push new commits/changes after review on PR. Manual actions depends too much depend on people and there is risk that we can merge something unverified.

May be this can help:
https://github.com/marketplace/actions/skip-duplicate-actions

@btlogy
Copy link
Contributor Author

btlogy commented Dec 8, 2022

What might no longer work after this PR is the automatic run of the test when somebody pushes more commits in a pull_request that is already open.
And for that matter, I hope the manual trigger could help.

That part might not be good, as we tend to push new commits/changes after review on PR. Manual actions depends too much depend on people and there is risk that we can merge something unverified.

May be this can help: https://github.com/marketplace/actions/skip-duplicate-actions

I'm testing different options in a sandbox repository (la-test/sbx-actions#27).
I'll propose some change here later...

@btlogy
Copy link
Contributor Author

btlogy commented Dec 8, 2022

As suggested by @donpui, the use of skip-duplicate-actions seems to achieve our goal here (and likely in other repositories too).
I've tested as much as I could (in sbx-actions), but I think this PR will have to be merged first before we can verify its full impact.

@btlogy btlogy changed the title Trigger test for any pull_request and push on main branch only Avoid duplicate and useless checks Dec 8, 2022
@btlogy
Copy link
Contributor Author

btlogy commented Dec 8, 2022

@donpui : can you have a second look here and tick the check-boxes in the description?

@donpui donpui merged commit 7eeba68 into main Dec 8, 2022
@donpui donpui deleted the avoid-test-twice branch December 8, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants