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

Build pool on merge to main #131

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Build pool on merge to main #131

wants to merge 3 commits into from

Conversation

zsusswein
Copy link
Collaborator

Currently we're only building pools as part of PRs.

Copy link

github-actions bot commented Dec 17, 2024

Thank you for your contribution @zsusswein 🚀! Your pkgdown-site is ready for download 👉 here 👈!
(The artifact expires on 2024-12-30T16:03:02Z. You can re-generate it by re-running the workflow here.)

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!

@zsusswein zsusswein marked this pull request as ready for review December 18, 2024 22:25
@athowes athowes self-requested a review December 19, 2024 12:40
Copy link
Collaborator

@athowes athowes left a comment

Choose a reason for hiding this comment

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

Does this mean that tests are run on merge to main as well as in PRs?

And the point of that is that one can verify that main is stable, not only that the PRs that went into main are stable?

@zsusswein
Copy link
Collaborator Author

zsusswein commented Dec 19, 2024

No, many checks run on merge to main. See their history here. What this PR does is at adds a trigger for the workflow to set up an Azure Batch pool to run as after each push to main. Currently, the build Batch pool deployment only runs on PRs, not after merging so we don't have a "production" Batch pool.

Currently we're only building pools as part of PRs.
@athowes
Copy link
Collaborator

athowes commented Dec 19, 2024

Currently, the build Batch pool deployment only runs on PRs, not after merging so we don't have a "production" Batch pool.

What's the benefit of a "production" Batch pool?

@amondal2
Copy link

Will this error out on a push to main event? Since it would technically run after the PR has closed (I think?)

ref: ${{ github.event.pull_request.head.sha }}

@zsusswein
Copy link
Collaborator Author

That's a great point.

I don't see an easy solution here. Perhaps the simplest would be to drop the functionality that parses the commit message. It was a clever idea from George, but it feels optional and building a pool off main is critical.

Thoughts?

@amondal2
Copy link

I'm ok with removing as long as it doesn't break the PR pool setup workflow

@gvegayon
Copy link
Member

I agree with removing that bit. @amondal2, you seem like a GHA wiz! It'd be great if you could contribute to the cfa-actions project. I would love to replace what we currently have here with the action (https://github.com/CDCgov/cfa-actions/blob/3e3f6ddbd1c4742c9cc830c4fd4e06e131dce587/twostep-container-build/action.yml#L84-L87). We are starting to use this in a few places already, so it would be a nice contribution across projects.

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.

4 participants