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

Actions: allow manual triggering #597

Merged
merged 1 commit into from
Oct 21, 2023
Merged

Actions: allow manual triggering #597

merged 1 commit into from
Oct 21, 2023

Conversation

gperciva
Copy link
Member

No description provided.

@gperciva
Copy link
Member Author

Lastest attempt to get this working.

... nope, I can already see that it's not doing what I want. :|

- '!coverity-scan'
pull_request:
branches:
- '**'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only my guess. I think this is kind of the correct way. I am guessing in this PR the actions ran twice because of both 'push' and 'pull_request' events. For example, I looked at curl/.github/workflows/linux.yml. There I see 'on push branch main' and 'on pull_request branch main'. I think that means pushing to main branch of the repository runs the actions and on pull request against the main branch. That would prevent running them twice if you open a pull request. To actually test if this fixes the outsiders' issue, someone would need to submit a PR with these yml changes in their PR (assuming tarsnap's master branch doesn't have these yet).

Copy link
Contributor

Choose a reason for hiding this comment

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

Edit: by main I meant master. If you do the same in this PR i.e., 'on push branch master' and 'on pull_request branch master' like the curl example, the actions should not run twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I found the part of the docs:

If multiple triggering events for your workflow occur at the same time, multiple workflow runs will be triggered.
https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#using-multiple-events

I was hopeful when I read that, because I kind-of expected the next sentence to be "... if you only want one workflow to be trigger, then XYZ".

I have the nagging feeling that I'm missing something obvious. I mean, what I was isn't complicated:

  • if there's any push to the Tarsnap/tarsnap respository, run the tests.
  • if there's a PR but no workflow yet (because it came from a third party), run the tests.

I'm baffled that the default assumption seems to be that running the same workflow multiple times is desired. I mean, unless the tests are non-deterministic (which would be a mistake!), there's no point testing a certain git hash more than once. If git hash abce1234 fails, one workflow is enough. If it succeeds, then running more than one workflow is just a waste of time & energy & resources.

Hmm, maybe workflow_dispatch is the best option. At least that lets us run a workflow manually.

@gperciva gperciva marked this pull request as draft October 20, 2023 23:15
I tried to get Actions to run on contributor's PRs back in:
    2023-09-19 Actions: don't ignore if '/' in branch name
    748cb93
but it didn't work.

After reading the docs some more, my theory is that the since the
previous yaml ran "on a push to any branch", that meant "on a push to
any branch in the Tarsnap/tarsnap repository", which didn't apply to
third parties, since they were pushing to their own fork.

Adding "workflow_dispatch" should allow us to manualy trigger a check
for third-party PRs.
@gperciva gperciva marked this pull request as ready for review October 21, 2023 21:15
@gperciva
Copy link
Member Author

Latest attempt. I think it's ready to merge?

There's two caveats:

So I think we might as well merge this.

@gperciva gperciva changed the title Actions: run on PR as well as push Actions: allow manual triggering Oct 21, 2023
@cperciva cperciva merged commit 6a053ba into master Oct 21, 2023
2 checks passed
@gperciva gperciva deleted the actions branch October 21, 2023 21:43
@gperciva gperciva mentioned this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants