✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070

tobiasgiese · 2021-12-04T14:04:20Z

What this PR does / why we need it:

This PR allows OpenStackCluster.Spec.Bastion to be updated (i.e., to be mutable).

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 #1054

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

_{Tobias Giese tobias.giese@daimler.com, Daimler TSS GmbH, legal info/Impressum}

netlify · 2021-12-04T14:04:26Z

✔️ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

🔨 Explore the source changes: ab690e0

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/61af34d309b6db0007ad5eb2

😎 Browse the preview: https://deploy-preview-1070--kubernetes-sigs-cluster-api-openstack.netlify.app/print

Signed-off-by: Tobias Giese <tobias.giese@daimler.com>

api/v1alpha4/openstackcluster_webhook.go

mdbooth

Thanks for the unit tests! Couple of discussion points.

api/v1alpha4/openstackcluster_webhook_test.go

api/v1beta1/openstackcluster_webhook_test.go

api/v1alpha4/openstackcluster_webhook_test.go

Signed-off-by: Tobias Giese <tobias.giese@daimler.com>

mdbooth

/lgtm

Thanks for the extra test. As discussed in Slack, I was worried that allowing changes to the spec before the bastion had actually been removed could potentially introduce a race which somebody might hit if, e.g. they scripted it. This approach feels simple and correct.

mdbooth · 2021-12-07T10:37:32Z

api/v1alpha4/openstackcluster_webhook_test.go

+			wantErr: true,
+		},
+		{
+			name: "Disabling the OpenStackCluster.Spec.Bastion while it's running is allowed",


mdbooth · 2021-12-07T13:14:52Z

@tobiasgiese Tests are all passing. Do you want to remove the hold?

tobiasgiese · 2021-12-07T13:18:05Z

@tobiasgiese Tests are all passing. Do you want to remove the hold?

I‘m still waiting for the approve of a CAPO maintainer. Prow won‘t merge until it‘s approved.

tobiasgiese · 2021-12-07T13:23:59Z

ptal @hidekazuna @jichenjc 🙂 thanks

mdbooth · 2021-12-07T13:47:10Z

@tobiasgiese Tests are all passing. Do you want to remove the hold?

I‘m still waiting for the approve of a CAPO maintainer. Prow won‘t merge until it‘s approved.

That's ok. If you remove the /hold it will merge immediately after it's approved.

tobiasgiese · 2021-12-07T13:49:57Z

That's ok. If you remove the /hold it will merge immediately after it's approved.

I know that 🙂 we're using Prow at work.
I like to know when my PR is going to be merged. Also, I don't like to handover the responsibility (as it's sometimes a approve-blocker for someone).

Edit: and it removes the chance to get reviews from more people. If someone approves and lgtm this PR without my hold it's going to be merged, no matter if other people have findings/concerns or not :)

seanschneeweiss · 2021-12-08T00:10:38Z

/LGTM
Agree very much that the tests make it easy to review 👍

hidekazuna · 2021-12-08T00:27:47Z

/approve

k8s-ci-robot · 2021-12-08T00:28:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, mdbooth, tobiasgiese

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:

~~OWNERS~~ [hidekazuna]

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

hidekazuna · 2021-12-08T00:30:40Z

@tobiasgiese You can remove /hold if you want 😃

tobiasgiese · 2021-12-08T06:47:39Z

thx @hidekazuna :)

/unhold

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 4, 2021

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 4, 2021

k8s-ci-robot requested review from prankul88 and seanschneeweiss December 4, 2021 14:04

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch from ad684bc to 301d7b8 Compare December 4, 2021 14:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 4, 2021

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch 4 times, most recently from 9a56fa2 to aea3643 Compare December 4, 2021 15:11

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 4, 2021

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch 5 times, most recently from e6d66f4 to 8810b36 Compare December 5, 2021 11:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 5, 2021

tobiasgiese changed the title ~~✨ Allow webhook changes to the bastion enabled flag~~ ✨ Allow webhook changes to OpenStackCluster.Spec.Bastion Dec 5, 2021

Refactor API webhooks

0a04a08

Signed-off-by: Tobias Giese <tobias.giese@daimler.com>

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch 3 times, most recently from c68b89e to 5be9181 Compare December 5, 2021 22:02

seanschneeweiss reviewed Dec 5, 2021

View reviewed changes

api/v1alpha4/openstackcluster_webhook.go Show resolved Hide resolved

tobiasgiese mentioned this pull request Dec 6, 2021

Bastion host is immutable on 0.5.0 while it wasnt on 0.4.0 #1054

Closed

mdbooth requested changes Dec 6, 2021

View reviewed changes

k8s-ci-robot assigned mdbooth Dec 7, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2021

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch from 6c2b7f4 to a32fe32 Compare December 7, 2021 10:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2021

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch from a32fe32 to f81ecbf Compare December 7, 2021 10:10

Allow webhook changes to OpenStackCluster.Spec.Bastion

ab690e0

Signed-off-by: Tobias Giese <tobias.giese@daimler.com>

tobiasgiese force-pushed the tobiasgiese/allow-bastion-host-changes branch from f81ecbf to ab690e0 Compare December 7, 2021 10:17

mdbooth approved these changes Dec 7, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2021

k8s-ci-robot assigned seanschneeweiss Dec 8, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2021

k8s-ci-robot merged commit 172c23f into kubernetes-sigs:main Dec 8, 2021

This was referenced Dec 8, 2021

✨ [CP] Allow webhook changes to OpenStackCluster.Spec.Bastion #1075

Closed

✨ [v0.5.0] Allow webhook changes to OpenStackCluster.Spec.Bastion #1076

Merged

✨ [v0.4.0] Allow webhook changes to OpenStackCluster.Spec.Bastion #1077

Merged

tobiasgiese deleted the tobiasgiese/allow-bastion-host-changes branch December 8, 2021 09:18

mdbooth mentioned this pull request Feb 7, 2022

Release v0.5.1 #1129

Closed

apricote mentioned this pull request Mar 22, 2022

docs: Explain OpenStackCluster.Spec.Bastion mutability in CRD #1180

Closed

bavarianbidi mentioned this pull request Jul 15, 2022

✨ bastion should be mutable #1298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070

✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070

tobiasgiese commented Dec 4, 2021 •

edited

Loading

netlify bot commented Dec 4, 2021 •

edited

Loading

mdbooth left a comment

mdbooth left a comment

mdbooth Dec 7, 2021

mdbooth commented Dec 7, 2021

tobiasgiese commented Dec 7, 2021

tobiasgiese commented Dec 7, 2021

mdbooth commented Dec 7, 2021

tobiasgiese commented Dec 7, 2021 •

edited

Loading

seanschneeweiss commented Dec 8, 2021

hidekazuna commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

hidekazuna commented Dec 8, 2021

tobiasgiese commented Dec 8, 2021

✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070

✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070

Conversation

tobiasgiese commented Dec 4, 2021 • edited Loading

netlify bot commented Dec 4, 2021 • edited Loading

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth Dec 7, 2021

Choose a reason for hiding this comment

mdbooth commented Dec 7, 2021

tobiasgiese commented Dec 7, 2021

tobiasgiese commented Dec 7, 2021

mdbooth commented Dec 7, 2021

tobiasgiese commented Dec 7, 2021 • edited Loading

seanschneeweiss commented Dec 8, 2021

hidekazuna commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

hidekazuna commented Dec 8, 2021

tobiasgiese commented Dec 8, 2021

tobiasgiese commented Dec 4, 2021 •

edited

Loading

netlify bot commented Dec 4, 2021 •

edited

Loading

tobiasgiese commented Dec 7, 2021 •

edited

Loading