-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support automatically merging PRs that don't affect target clusters #6
Conversation
Co-authored-by: Sunil Aggarwal <[email protected]>
Co-authored-by: Sunil Aggarwal <[email protected]>
ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) | ||
err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) | ||
if err != nil { | ||
prHandleError = err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought but non blocking for this, I see this is following convention below but would be shadowed if errors happens there. Could potentially use errors.Join
or refactor to avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.Join
== mind blown :)
I'll merge as is because I do log the errors so they are not completely lost but yeah, keeping them all is definitely better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:0 I didn't even know about errors.Join
TechTime on error wrapping would definitely be killer. ErrorWrapping isn't something I've really picked up. I do love being able to use errors.Is
for error pathing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Support automaticallyh merging PRs that don't affect target clusters - those usually happen when you change some component configuration that affect only some of the environments.
We still want to merge these to avoid environment drift.
Type of Change
Checklist