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

🌱 Improve cert-manager shouldUpgrade #10407

Merged
merged 1 commit into from
Apr 12, 2024
Merged

🌱 Improve cert-manager shouldUpgrade #10407

merged 1 commit into from
Apr 12, 2024

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Apr 9, 2024 •
edited

What this PR does / why we need it:

Make shouldUpgrade() accept the list of objects to be installed too.
Compare the installed and to-be-installed objects in case the
installed and to-be-installed versions are the same. If their length is
different, assume that an upgrade is required.

Update unit tests.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #10389
(the linked issue was a but unclear what's going on, but we established that shouldUpgrade can check len of objects too)

/area clusterctl
/kind feature

@k8s-ci-robot k8s-ci-robot added area/clusterctl Issues or PRs related to clusterctl kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 9, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 9, 2024
@neolit123
Copy link
Member Author

/cc @chrischdi @sbueringer
PTAL

@neolit123 neolit123 changed the title clusterctl/client/cert_manager: improve shouldUpgrade 🌱 clusterctl/client/cert_manager: improve shouldUpgrade Apr 9, 2024
sbueringer
sbueringer reviewed Apr 9, 2024
View reviewed changes
cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/cert_manager.go Outdated Show resolved Hide resolved
cmd/clusterctl/client/cluster/cert_manager_test.go Outdated Show resolved Hide resolved
@neolit123 neolit123 force-pushed the 1.7-improve-clusterctl-cert-manager-install branch from 3563bfc to 2784b54 Compare April 9, 2024 13:53
@sbueringer
Copy link
Member

Thx!

/lgtm

/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 53e62f6f8b22f5dc8244862969b5c8712f335122

@neolit123
clusterctl/client/cert_manager: improve shouldUpgrade
47dcdda 
Make shouldUpgrade() accept the list of objects to be installed too.
Compare the installed and to-be-installed objects in case the
installed and to-be-installed versions are the same. If their length is
different, assume that an upgrade is required.

Update unit tests.
@neolit123 neolit123 force-pushed the 1.7-improve-clusterctl-cert-manager-install branch from 2784b54 to 47dcdda Compare April 11, 2024 20:10
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2024
@sbueringer
Copy link
Member

/lgtm

/assign @fabriziopandini @chrischdi

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: beadb3144fc0f5dad31328345d7d9ea29af7a2c1

chrischdi
chrischdi reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/approve

Thanks for working on this!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2024
@k8s-ci-robot k8s-ci-robot merged commit fb30a6f into kubernetes-sigs:main Apr 12, 2024
25 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.8 milestone Apr 12, 2024
fabriziopandini
fabriziopandini reviewed Apr 12, 2024
View reviewed changes
cmd/clusterctl/client/cluster/cert_manager.go
c, err := cm.proxy.NewClient(ctx)
if err != nil {
return err
}

return NewCRDMigrator(c).Run(ctx, objs)
return NewCRDMigrator(c).Run(ctx, installObj)
Copy link
Member

Choose a reason for hiding this comment

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

q: why are passing installed objects here?
If I remember well CRD migrator expect to get in input the objects that are going to be installed to determine what is going to change

Copy link
Member

Choose a reason for hiding this comment

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

never mind, those are the objects to be installed

Copy link
Member

Choose a reason for hiding this comment

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

installObj are the objects to-be installed, not the one which exist :-)

It's the same as before, the output of: cm.getManifestObjs(ctx, config)

@sbueringer
Copy link
Member

This was actually a bug fix. So it qualifies for cherry-pick

/cherry-pick release-1.7

@sbueringer
Copy link
Member

/cherry-pick release-1.6

@k8s-infra-cherrypick-robot
Copy link

@sbueringer: #10407 failed to apply on top of branch "release-1.6":

Applying: clusterctl/client/cert_manager: improve shouldUpgrade
Using index info to reconstruct a base tree...
M	cmd/clusterctl/client/cluster/cert_manager.go
M	cmd/clusterctl/client/cluster/cert_manager_test.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/clusterctl/client/cluster/cert_manager_test.go
Auto-merging cmd/clusterctl/client/cluster/cert_manager.go
CONFLICT (content): Merge conflict in cmd/clusterctl/client/cluster/cert_manager.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 clusterctl/client/cert_manager: improve shouldUpgrade
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.6

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/test-infra repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Apr 23, 2024
Merged
@k8s-infra-cherrypick-robot
Copy link

@sbueringer: new pull request created: #10497

In response to this:

This was actually a bug fix. So it qualifies for cherry-pick

/cherry-pick release-1.7

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/test-infra repository.

@sbueringer sbueringer changed the title 🌱 clusterctl/client/cert_manager: improve shouldUpgrade 🌱 improve cert-manager shouldUpgrade Jul 19, 2024
@sbueringer sbueringer changed the title 🌱 improve cert-manager shouldUpgrade 🌱 Improve cert-manager shouldUpgrade Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrischdi chrischdi chrischdi left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@Jont828 Jont828 Awaiting requested review from Jont828

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

@chrischdi chrischdi

@sbueringer sbueringer

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

clusterctl can fail idempotency when installing cert-manager during upgrade
6 participants
@neolit123 @sbueringer @k8s-ci-robot @k8s-infra-cherrypick-robot @chrischdi @fabriziopandini