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

Merge queue workflow config #1138

Merged
merged 3 commits into from
Jan 30, 2025
Merged

Merge queue workflow config #1138

merged 3 commits into from
Jan 30, 2025

Conversation

laurafitzgerald
Copy link
Contributor

@laurafitzgerald laurafitzgerald commented Jan 24, 2025 •
edited
Loading

Removing master from branch list as we don't use it.
Updating CODEOWNERS to valid team.
Adding configuration as per Kuadrant/kuadrant#7 in preparation to enable the merge queue in the settings

@laurafitzgerald laurafitzgerald mentioned this pull request Jan 24, 2025
Open
30 tasks
@Boomatang
Copy link
Contributor

This is looking good. I want to approve it but first I want to ask one question.

What triggers the auto merge?
There is two case that I can see myself wanting to disable the auto-merge. First, I am expecting more people to review the PR and I don't want one person triggering the auto merge before everyone had a change to look at it. The second is based around, I know the PR has more changes coming. This could be a simple as the commit history clean up, or the adding in of some related work like tests. I know for adding the related work that can be in a follow up PR.

@laurafitzgerald
Copy link
Contributor Author

This is looking good. I want to approve it but first I want to ask one question.

What triggers the auto merge? There is two case that I can see myself wanting to disable the auto-merge. First, I am expecting more people to review the PR and I don't want one person triggering the auto merge before everyone had a change to look at it. The second is based around, I know the PR has more changes coming. This could be a simple as the commit history clean up, or the adding in of some related work like tests. I know for adding the related work that can be in a follow up PR.

Hi @Boomatang thanks for the review, there is one further change we'll need here so please hold off on approval.
The author is free to click the Merge when Ready button on their own prs at any time.
The pr will then be added to the merge pool when all checks are met (this includes approval from one approver)
Once in the merge pool retesting may happen against other prs or an up to date main.

@Boomatang
Copy link
Contributor

Okay, so there is still some control for the author. That seems fair.

@laurafitzgerald laurafitzgerald force-pushed the merge-queue-workflow-config branch 2 times, most recently from b191e87 to 8754071 Compare January 30, 2025 15:25
laurafitzgerald
laurafitzgerald commented Jan 30, 2025
View reviewed changes
.github/workflows/test.yaml
# This check adds a list of checks to one job to simplify adding settings to the repo.
# If a new check is added in this file, and it should be retested on entry to the merge queue,
# it needs to be added to the list below aka needs: [ existing check 1, existing check 2, new check ].
needs: [ unit-tests, controllers-integration-tests, bare-k8s-integration-tests, gatewayapi-integration-tests, gatewayapi-provider-integration-tests, verify-manifests, verify-bundle, verify-fmt, test-scripts, verify-generate, verify-go-mod, verify-helm-charts ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a big list. I need someone more familiar with the code base to say whether all of these checks are required to be retested once the pr is added to the merge-queue

Copy link
Member

Choose a reason for hiding this comment

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

This list should just be all the jobs in this workflow for now, that will probably be the case for most if not all of them.

@laurafitzgerald laurafitzgerald force-pushed the merge-queue-workflow-config branch 2 times, most recently from 6969624 to dd8663d Compare January 30, 2025 15:50
@laurafitzgerald laurafitzgerald force-pushed the merge-queue-workflow-config branch from dd8663d to 0192ac2 Compare January 30, 2025 15:58
mikenairn
mikenairn approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@mikenairn mikenairn left a comment

Choose a reason for hiding this comment

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

The changes here look good. We have applied the same configuration to the dns-operator workflows and things are working as expected so far. See no reason not to go ahead with the same setup here.

@laurafitzgerald laurafitzgerald added this pull request to the merge queue Jan 30, 2025
Merged via the queue into Kuadrant:main with commit de4bc11 Jan 30, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikenairn mikenairn mikenairn approved these changes

@Boomatang Boomatang Awaiting requested review from Boomatang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@laurafitzgerald @Boomatang @mikenairn