Skip to content

Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate #1257

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

Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate #1257

Xenwar wants to merge 1 commit into from

Conversation

@Xenwar
Copy link

@Xenwar Xenwar commented Jun 6, 2022

Deprecating and replacing PortOpts.SecurityGroups of []string format field from OpenStackMachineTemplate ports and replace is by []SecurityGroupParam format.

fixes: #1251

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2022
@netlify
Copy link

netlify bot commented Jun 6, 2022 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 6b88bd1
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/62b017f15767ef0008108ead
😎 Deploy Preview https://deploy-preview-1257--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Xenwar Xenwar changed the title ✨ Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate ✨ [WIP] Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate Jun 6, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 6, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Xenwar
To complete the pull request process, please assign tobiasgiese after the PR has been reviewed.
You can assign the PR to them by writing /assign @tobiasgiese in a comment when ready.

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

@k8s-ci-robot k8s-ci-robot requested review from jichenjc and mdbooth June 6, 2022 11:44
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2022
@Xenwar
Copy link
Author

Xenwar commented Jun 6, 2022

/pull-cluster-api-provider-openstack-e2e-test

1 similar comment
@Xenwar
Copy link
Author

Xenwar commented Jun 6, 2022

/pull-cluster-api-provider-openstack-e2e-test

@Xenwar Xenwar force-pushed the deprecate-security-field-at-port-level branch from f699622 to 33d243f Compare June 6, 2022 12:15
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 6, 2022
@Xenwar
Copy link
Author

Xenwar commented Jun 6, 2022

/pull-cluster-api-provider-openstack-e2e-test

@Xenwar
Copy link
Author

Xenwar commented Jun 6, 2022

/retest-required

@Xenwar
Copy link
Author

Xenwar commented Jun 7, 2022

/pull-cluster-api-provider-openstack-e2e-test

@Xenwar
Copy link
Author

Xenwar commented Jun 7, 2022

for the e2e, the "...(multiple attached networks) ..." test is failing. could not see the relationship with my changes. Any idea ?

@jichenjc
Copy link
Contributor

jichenjc commented Jun 8, 2022

looks like the the error occurs at Delete stage and 300 sec timeout
I checked https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/1257/pull-cluster-api-provider-openstack-e2e-test/1533819008657133568/artifacts/clusters/bootstrap/controllers/capo-controller-manager/capo-controller-manager-579b86d9c6-8tczt/manager.log

but see nothing special to cluster e2e-z02d16/cluster-e2e-z02d16 compare to other clusters..
I am guessing some sec group changes leads to the delete process failed and blocked the final deletion, but no clue and evidence yet, need more insight ...

@Xenwar
Copy link
Author

Xenwar commented Jun 8, 2022

@jichenjc Thanks, will try focusing on that.

@Xenwar Xenwar force-pushed the deprecate-security-field-at-port-level branch 2 times, most recently from f368a63 to 6691aee Compare June 8, 2022 09:08
@Xenwar
Copy link
Author

Xenwar commented Jun 8, 2022

@jichenjc thanks for hint.
the issue was the cleanup did not remove a security group I added for testing the new format.
Now fixed,

@jichenjc
Copy link
Contributor

jichenjc commented Jun 8, 2022

ok, please fix the UT and others lgtm

@Xenwar Xenwar force-pushed the deprecate-security-field-at-port-level branch 3 times, most recently from a97f98e to e7cd974 Compare June 9, 2022 06:38
@Xenwar
Copy link
Author

Xenwar commented Jun 9, 2022

I am having a problem with the conversion to/from v1alpha4. Could this be related to the discussion here ?#1246 (comment)

@Xenwar Xenwar mentioned this pull request Jun 13, 2022
Closed
@Xenwar Xenwar force-pushed the deprecate-security-field-at-port-level branch 2 times, most recently from e7bda91 to 516fbc4 Compare June 13, 2022 11:45
@Xenwar Xenwar changed the title ✨ [WIP] Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate Deprecating and/or replacing securityGroups field from OpenStackMachineTemplate Jun 13, 2022
@jichenjc
Copy link
Contributor

jichenjc commented Jun 15, 2022

/test pull-cluster-api-provider-openstack-test

@Xenwar Xenwar force-pushed the deprecate-security-field-at-port-level branch from 516fbc4 to 11da8cb Compare June 17, 2022 10:19
@Xenwar Xenwar force-pushed the deprecate-security-field-at-port-level branch from 11da8cb to 6b88bd1 Compare June 20, 2022 06:47
@Xenwar
Copy link
Author

Xenwar commented Jun 20, 2022

/hold cancel

@apricote
Copy link
Member

apricote commented Jun 20, 2022

Sorry for my delayed response.

/hold

This PR modifies our v1alpha5 CRDs in a backwards-incompatible way. As this version is already released in v0.6, such changes are not allowed. Backwards-incompatible changes are only allowed on unreleased CRD versions. We discussed this during last weeks office hours and decided to create a new CRD version v1alpha6 to allow making these changes, I will create an issue for this today/tomorrow.

I would like to see this cleanup implemented in v1alpha6, but until the necessary PR for that has been merged, and this PR changed to make the changes to the new version, this PR should be left on hold.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 20, 2022
@Xenwar
Copy link
Author

Xenwar commented Jun 20, 2022

@apricote
Thanks for the response. Adding the hold again.
/hold

@Xenwar
Copy link
Author

Xenwar commented Jun 20, 2022

/assign @lentzi90
This might require a rebase and I may not able to do that during the summer.
would you please handle this. Thanks.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 20, 2022

@Xenwar: GitHub didn't allow me to assign the following users: lentzi90.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @lentzi90
This might require a rebase and I may not able to do that during the summer.
would you please handle this. Thanks.

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-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 27, 2022

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

@apricote
Copy link
Member

apricote commented Jun 27, 2022

@lentzi90 @Xenwar The new API Version v1alpha6 is available in main now, you can rebase your PR and remove the field from the new API.

@lentzi90 lentzi90 mentioned this pull request Jul 4, 2022
Closed
3 tasks
@mdbooth
Copy link
Contributor

mdbooth commented Jul 5, 2022

Closed for #1291

@mdbooth mdbooth closed this Jul 5, 2022
@lentzi90 lentzi90 deleted the deprecate-security-field-at-port-level branch April 17, 2025 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jichenjc jichenjc Awaiting requested review from jichenjc

@mdbooth mdbooth Awaiting requested review from mdbooth

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deprecating/Replacing securityGroups field from OpenStackMachineTemplate ports

5 participants

@Xenwar @k8s-ci-robot @jichenjc @apricote @mdbooth