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

add a concurrency check to gh workflows #6652

Open
mahf708 opened this issue Sep 29, 2024 · 3 comments
Open

add a concurrency check to gh workflows #6652

mahf708 opened this issue Sep 29, 2024 · 3 comments
Labels
help wanted Testing Anything related to unit/system tests

Comments

@mahf708
Copy link
Contributor

mahf708 commented Sep 29, 2024

And maybe I would also add a concurrency check. If a commit is pushed while testing, we should kill any other existing PR. Something like

concurrency:
  group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.run_id }}
  cancel-in-progress: true

Btw, I'm not sure why we would want a workflow_dispath trigger here. And if you removed "workflow_dispatch", the group could simply be ${{ github.workflow }}-${{ github.event.pull_request.number}}. But I don't feel strongly about this, so I'm ok keeping workflow_dispatch.

Originally posted by @bartgol in #6637 (comment)

@mahf708 mahf708 added help wanted Testing Anything related to unit/system tests labels Sep 29, 2024
@mahf708
Copy link
Contributor Author

mahf708 commented Sep 29, 2024

If someone picks this up, pls keep the workflow_dispatch, and at any rate, pls request review from or at least tag both luca (@bartgol) and me (@mahf708) to review the PR

The workflow_dispatch part is useful because it allows us to trigger the workflow on any arbitrary branch (i.e., outside of PRs) and it also allows us to trigger the workflows at-will.

@bartgol
Copy link
Contributor

bartgol commented Sep 30, 2024

I thought that in your action, as it was written (or, maybe, as I read it) there was no real need for dispatch trigger. That said, there could be a use for dispatch trigger: to generate baselines. As such, we would need a check in the job section that, depending on the trigger, decides what flags to add to test-all. In my fork, I was experimenting with multiple dispatch types in the same action, each of which needs to handle different test-all flags:

  • if pr trigger: no "extra" flags
  • if schedule trigger (nightly): add -s (to submit to cdash)
  • if manual trigger (bless): add -g (to generate baselines)

This would however require to store baselines somewhere. I don't think we can store inside the container (unless we rebuild it). Perhaps we could store eamxx baselines for gh actions (or even all machines) in a separate repo, divided by machine? They are not that big anyways. Our whole mappy baselines folder (for eamxx standalone) is ~650M, and our v1 folders are prob ladding up to ~250M. We can make our nightly job copy the baselines from this helper repo into the container image, so that PR testing is always "fast" (just one download: the image).

@mahf708
Copy link
Contributor Author

mahf708 commented Sep 30, 2024

I thought that in your action, as it was written (or, maybe, as I read it) there was no real need for dispatch trigger.

The only reason for that line is that it allows us to trigger it manually. Without it, I don't think we can trigger the action manually... do you think I am mistaken? Is there a way to trigger the action manually without it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Testing Anything related to unit/system tests
Projects
None yet
Development

No branches or pull requests

2 participants