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

Break workflows into separate 'E2E Test' and 'Build and Unit Test' workflows, use pull_request instead of pull_request_target across workflows #575

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

Break workflows into separate 'E2E Test' and 'Build and Unit Test' workflows, use pull_request instead of pull_request_target across workflows #575

wants to merge 4 commits into from

Conversation

squizzi
Copy link
Contributor

@squizzi squizzi commented Oct 29, 2024 •
edited
Loading

This PR makes the workflow changes discussed on Slack, moving away from pull_request_target and using pull_request across workflows. The decision to move away from pull_request_target entirely was made here: #575 (comment)

@squizzi
Copy link
Contributor Author

squizzi commented Oct 29, 2024 •
edited
Loading

The pull_request_target workflows are going to continue to run in this PR since they're based on main, once this is merged they'll only show up for E2E tests and be otherwise skipped -- so just ignore them. Build and Unit Tests is the workflow being added, which is green.

@squizzi
Add access checks prior to checking our PRs
5b352c5 
Ensure only users with write permissions can run CI

Signed-off-by: Kyle Squizzato <[email protected]>
@squizzi
Break e2e tests into own workflow, use pull_request for all workflows
916b428 
This patch breaks the e2e test and build workflows apart making it so that
all workflows return to using 'pull_request' to avoid any security
issues and other frustrations surrounding 'pull_request_target'

We now have 2 workflow files, one for build and unit tests which uses
'pull_request' and one for e2e tests.  The e2e tests require secret
population and must be created on branch to run, they also require the
'test e2e' label to prevent uneccessary execution.

Signed-off-by: Kyle Squizzato <[email protected]>
@squizzi squizzi changed the title Break workflows into separate 'E2E Test' and 'Build and Unit Test' workflows Break workflows into separate 'E2E Test' and 'Build and Unit Test' workflows, use pull_request instead of pull_request_target across workflows Oct 31, 2024
@squizzi
Copy link
Contributor Author

squizzi commented Oct 31, 2024 •
edited
Loading

I'm still seeing issues running tests on pull_request_target, I think it's time to just bite the bullet and call it what it is, with all of the discussion around pull_request_target and the unnecessary workarounds and additional checks we can now omit I believe the best step is to just run any work that requires running e2e tests on a repo branch and perhaps run them as nightly's to catch any outstanding issues from other PRs. The workflows have been updated to support this, I'll be reopening #516 as a repo branch PR based off of these commits and working to get the tests green today.

With other OSS projects following suite of running their e2e tests from repo branches I think it's best we do the same.

@squizzi squizzi mentioned this pull request Oct 31, 2024
Closed
slysunkin added a commit to slysunkin/hmc that referenced this pull request Nov 1, 2024
@slysunkin
Cherrypicked from Mirantis#575
0d7930e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kshatrix Kshatrix Awaiting requested review from Kshatrix Kshatrix is a code owner

@eromanova eromanova Awaiting requested review from eromanova eromanova is a code owner

@a13x5 a13x5 Awaiting requested review from a13x5 a13x5 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Project 2A
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@squizzi