-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🐛 (alpha update) fix make run logic in merge #4990
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,6 +222,29 @@ func runMakeTargets() { | |
| } | ||
| } | ||
|
|
||
| // runMakeTargetsSilently is a helper function to run make with the targets necessary | ||
| // to ensure all the necessary components are generated, formatted and linted. | ||
| // This version runs silently without logging, suitable for best-effort execution when conflicts exist. | ||
| func runMakeTargetsSilently() { | ||
| log.Info("Running make targets to generate, format, and lint code") | ||
| targets := []string{"manifests", "generate", "fmt", "vet", "lint-fix"} | ||
| for _, target := range targets { | ||
| cmd := exec.Command("make", target) | ||
| // Run silently - discard output and errors | ||
| _ = cmd.Run() | ||
| } | ||
| log.Info("Finished running make targets") | ||
| } | ||
|
|
||
| // hasConflictMarkers checks if there are any Git conflict markers in the repository. | ||
| // It returns true if conflict markers are found, false otherwise. | ||
| func hasConflictMarkers() bool { | ||
| cmd := exec.Command("git", "grep", "-l", "-E", "^(<<<<<<<|=======|>>>>>>>)") | ||
| err := cmd.Run() | ||
|
|
||
| return err == nil | ||
| } | ||
|
|
||
| // runAlphaGenerate executes the old Kubebuilder version's 'alpha generate' command | ||
| // to create clean scaffolding in the ancestor branch. This uses the downloaded | ||
| // binary with the original PROJECT file to recreate the project's initial state. | ||
|
|
@@ -340,25 +363,27 @@ func (opts *Update) mergeOriginalToUpgrade() error { | |
| hasConflicts = true | ||
| if !opts.Force { | ||
| log.Warn("Merge stopped due to conflicts. Manual resolution is required.") | ||
| log.Warn("After resolving the conflicts, run the following command:") | ||
| log.Warn(" make manifests generate fmt vet lint-fix") | ||
| log.Warn("This ensures manifests and generated files are up to date, and the project layout remains consistent.") | ||
| return fmt.Errorf("merge stopped due to conflicts") | ||
| } | ||
| log.Warn("Merge completed with conflicts. Conflict markers will be committed.") | ||
| } else { | ||
| return fmt.Errorf("merge failed unexpectedly: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if !hasConflicts { | ||
| log.Info("Merge happened without conflicts.") | ||
| runMakeTargets() | ||
| } else { | ||
| runMakeTargetsSilently() | ||
| } | ||
|
|
||
| // Best effort to run make targets to ensure the project is in a good state | ||
| runMakeTargets() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we still need to try to run those, but without raising an error or a warning. If the conflict is in the manifests
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright. I changed the make targets to run silently when there are conflicts. I also added another check for conflict markers after running the make targets. Now, the conflicts warnings are only logged if there still are conflict markers after running the make targets. Is that what you were thinking of? |
||
| hasConflicts = false | ||
| if hasConflictMarkers() { | ||
| hasConflicts = true | ||
| } | ||
|
|
||
| // Step 4: Stage and commit | ||
| log.Info("Staging all changes for commit") | ||
| if err := exec.Command("git", "add", "--all").Run(); err != nil { | ||
| return fmt.Errorf("failed to stage merge results: %w", err) | ||
| } | ||
|
|
@@ -367,10 +392,20 @@ func (opts *Update) mergeOriginalToUpgrade() error { | |
| if hasConflicts { | ||
| message += " With conflicts - manual resolution required." | ||
| } else { | ||
| message += " Merge happened without conflicts." | ||
| message += " Merge completed without conflicts." | ||
| } | ||
|
|
||
| _ = exec.Command("git", "commit", "-m", message).Run() | ||
|
|
||
| if hasConflicts { | ||
| log.Warn("Update completed with conflicts. Conflict markers were committed.") | ||
| log.Warn("Resolve the conflicts that were committed, then run:") | ||
| log.Warn(" make manifests generate fmt vet lint-fix") | ||
| log.Warn("This ensures manifests and generated files are up to date, " + | ||
| "and the project layout remains consistent.") | ||
| } else { | ||
| log.Info("Update completed successfully. Please review the changes.") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
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.
I do not think that we should run silently
We need to suppress the error with force and raise a warning instead, see
https://github.com/kubernetes-sigs/kubebuilder/pull/4992/files
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.
I hope that you do mind but aiming help you out I create the above PR
I would love to get your help to review this one.