Skip to content

🐛 (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.

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
Closed
wants to merge 1 commit into from
Closed

🐛 (alpha update) fix make run logic in merge #4990

wants to merge 1 commit into from

Conversation

@vitorfloriano
Copy link
Contributor

@vitorfloriano vitorfloriano commented Aug 6, 2025 •
edited
Loading

alpha update will now run the make targets silently if the merge is completed with conflicts. The user will be warned about the conflicts and instructed to run the make targets again after resolving them:

INFO Preparing Merge branch and performing merge branch_name=tmp-merge-07-08-25-01-52
INFO Running make targets to generate, format, and lint code 
INFO Finished running make targets 
INFO Staging all changes for commit 
WARN Update completed with conflicts. Conflict markers were committed. 
WARN Resolve the conflicts that were committed, then run: 
WARN      make manifests generate fmt vet lint-fix 
WARN This ensures manifests and generated files are up to date, and the project layout remains consistent.

Fixes #4989

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 6, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 6, 2025

Hi @vitorfloriano. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 6, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 6, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vitorfloriano
Once this PR has been reviewed and has the lgtm label, please assign camilamacedo86 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86
camilamacedo86 reviewed Aug 6, 2025
View reviewed changes
pkg/cli/alpha/internal/update/update.go
}

// Best effort to run make targets to ensure the project is in a good state
runMakeTargets()
Copy link
Member

@camilamacedo86 camilamacedo86 Aug 6, 2025

Choose a reason for hiding this comment

The 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.
Why?
Example: If the conflict is the Makefile
We still able to re-generate the manifest, run the go vet, go lint and etc

If the conflict is in the manifests
Then, by regenerating we might solve it out.

Copy link
Contributor Author

@vitorfloriano vitorfloriano Aug 7, 2025

Choose a reason for hiding this comment

The 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?

@vitorfloriano
(alpha update) fix make run logic in merge
556ea8e 
Alpha update will now run make targets silently
if the merge completes with conflicts, and the logs
will warn the user to run the make targets again
after resolving the conflicts that were committed.
@vitorfloriano vitorfloriano force-pushed the fix-alpha-update-make-run branch from e725ae0 to 556ea8e Compare August 7, 2025 05:01
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 7, 2025
camilamacedo86
camilamacedo86 reviewed Aug 7, 2025
View reviewed changes
pkg/cli/alpha/internal/update/update.go
// 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() {
Copy link
Member

@camilamacedo86 camilamacedo86 Aug 7, 2025

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

Copy link
Member

@camilamacedo86 camilamacedo86 Aug 7, 2025

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.

@vitorfloriano vitorfloriano mentioned this pull request Aug 7, 2025
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 7, 2025

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Aug 8, 2025

Closing this one sorry.
I misleading it, I thought that we have an issue that we have not
Really thank you for checking that

@vitorfloriano
Copy link
Contributor Author

vitorfloriano commented Aug 19, 2025

Resolved in #5019.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@Kavinjsir Kavinjsir Awaiting requested review from Kavinjsir

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

alpha update runs make targets when there are conflicts

3 participants

@vitorfloriano @k8s-ci-robot @camilamacedo86