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.

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

[GEP-7] Adds check for seed provider type when changing the seed name of shoot #3254

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

plkokanov
Copy link
Contributor

@plkokanov plkokanov commented Dec 2, 2020

How to categorize this PR?

/area control-plane-migration
/kind enhancement
/priority normal

What this PR does / why we need it:
This PR forbids changing the Shoot.Spec.SeedName field of a Shoot cluster if the new Seed has different cloud provider from the old one. Currently control planes cannot be migrated between Seeds with different providers, although we could implement this in the future.

Which issue(s) this PR fixes:
Part of #1631

Special notes for your reviewer:

Release note:

Forbid control plane migration between `Seeds` with different cloud providers.

@plkokanov plkokanov requested a review from a team as a code owner December 2, 2020 09:11
@gardener-robot gardener-robot added area/control-plane-migration Control plane migration related kind/enhancement Enhancement, improvement, extension priority/normal size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 2, 2020
rfranzke
rfranzke previously approved these changes Dec 2, 2020
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you!

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion for the name of the old seed to make it more clear. It is strange to me that the old seed is named "new..."
Also, names cannot contain brackets, ref: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names.

@@ -1951,6 +1953,27 @@ var _ = Describe("validator", func() {

Expect(err).To(HaveOccurred())
})

It("should fail to change Seed name, because cloud provider for new Seed is not equal to cloud provider for old Seed", func() {
oldSeedName := fmt.Sprintf("new(%s)", seedName)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
oldSeedName := fmt.Sprintf("new(%s)", seedName)
oldSeedName := fmt.Sprintf("old-%s", seedName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops forgot to change this after I modified the variable name.

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

@vpnachev vpnachev merged commit 78d3e52 into gardener:master Dec 3, 2020
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
@plkokanov plkokanov deleted the cp-migration/check-provider branch December 9, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane-migration Control plane migration related kind/enhancement Enhancement, improvement, extension size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants