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

Remove optional param CtlplaneNetmask #241

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove optional param CtlplaneNetmask #241

wants to merge 2 commits into from
+2 −30

Conversation

bshephar
Copy link
Contributor

@bshephar bshephar commented Dec 9, 2024 •
edited
Loading

@openshift-ci openshift-ci bot requested review from dprince and viroel December 9, 2024 07:19
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/aede651eb3d449eaafd46ba7dd3c6692

❌ openstack-baremetal-operator-content-provider FAILURE in 7m 30s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

@bshephar
Copy link
Contributor Author

/test openstack-baremetal-operator-build-deploy

rabi
rabi reviewed Dec 12, 2024
View reviewed changes
api/v1beta1/openstackbaremetalset_types.go Outdated
@@ -88,7 +88,6 @@ type OpenStackBaremetalSetSpec struct {
// +kubebuilder:validation:Optional
CtlplaneGateway string `json:"ctlplaneGateway,omitempty"`
// +kubebuilder:validation:Optional
// +kubebuilder:default="255.255.255.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this from API as it's not used anymore. Was left behind[1] to not break any usage. I think we can safely remove this.
[1] https://github.com/openstack-k8s-operators/openstack-baremetal-operator/blob/main/pkg/openstackbaremetalset/baremetalhost.go#L102

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, let's remove it

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 16, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bshephar
Once this PR has been reviewed and has the lgtm label, please assign dprince 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

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/3425573c7335423d83e6ba6a4eaa0523

❌ openstack-baremetal-operator-content-provider FAILURE in 6m 55s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

@bshephar
Copy link
Contributor Author

/retest

rabi
rabi reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@rabi rabi left a comment

Choose a reason for hiding this comment

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

You also have to remove https://github.com/openstack-k8s-operators/openstack-baremetal-operator/blob/main/pkg/openstackbaremetalset/baremetalhost.go#L102 where instance.Spec.CtlplaneNetmask is used.

api/bases/baremetal.openstack.org_openstackbaremetalsets.yaml Outdated
@@ -139,7 +139,6 @@ spec:
to use for ctlplane network
type: string
ctlplaneNetmask:
Copy link
Contributor

Choose a reason for hiding this comment

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

You've to regenerate these, else pre-commit and other jobs would fail.

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f97fde84bff542feb904c45d0d8c6db9

❌ openstack-baremetal-operator-content-provider FAILURE in 12m 18s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

@bshephar
Copy link
Contributor Author

/test openstack-baremetal-operator-build-deploy

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/bbdae48fe81d4606ba491a9c4c86e9ee

❌ openstack-baremetal-operator-content-provider FAILURE in 12m 13s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

bshephar added a commit to bshephar/openstack-operator that referenced this pull request Dec 23, 2024
@bshephar
Remove use of CtlplaneNetmask
f39669a 
This change removes use of CtlplaneNetmask in line with
the changes proposed in:
openstack-k8s-operators/openstack-baremetal-operator#241

Signed-off-by: Brendan Shephard <[email protected]>
@bshephar bshephar mentioned this pull request Dec 23, 2024
Merged
@bshephar
Copy link
Contributor Author

I think we probably need to go with the original plan of making this param optional. Then we can remove it from openstack-operator, then we can remove it from here.

@bshephar bshephar changed the title Don't set default for optional CtlplaneNetmask Remove optional param CtlplaneNetmask Jan 3, 2025
@bshephar
Copy link
Contributor Author

bshephar commented Jan 3, 2025

If we merge this one first:
#251

Then we can delete the param from the functional tests in openstack-operator, which will allow us to remove it entirely in this PR.

@rabi
Copy link
Contributor

rabi commented Jan 3, 2025

If we merge this one first:

OK, can we test the three PRs with depends-on, though I think it would work? I guess you've to also remove that from kuttl tests in openstack-operator.

@rabi rabi mentioned this pull request Jan 3, 2025
Merged
@bshephar
Don't set default for optional CtlplaneNetmask
3cbcdde 
This change removes the CtlplaneNetmask parameter since it is not
required since we're using CIDR's.

Signed-off-by: Brendan Shephard <[email protected]>
@bshephar
Don't set default for optional CtlplaneNetmask
42d9151 
Signed-off-by: Brendan Shephard <[email protected]>
@bshephar bshephar force-pushed the no-default-netmask branch from ff31cd9 to 42d9151 Compare January 3, 2025 04:32
bshephar added a commit to bshephar/openstack-operator that referenced this pull request Jan 3, 2025
@bshephar
Remove use of CtlplaneNetmask
f0a222c 
This change removes use of CtlplaneNetmask in line with
the changes proposed in:
openstack-k8s-operators/openstack-baremetal-operator#241

Signed-off-by: Brendan Shephard <[email protected]>
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/dee1eef1b12e4b5b892a85273422758d

❌ openstack-baremetal-operator-content-provider FAILURE in 12m 37s
⚠️ openstack-baremetal-operator-crc-podified-edpm-baremetal SKIPPED Skipped due to failed job openstack-baremetal-operator-content-provider

@bshephar
Copy link
Contributor Author

bshephar commented Jan 3, 2025

/test functional

@bshephar
Copy link
Contributor Author

bshephar commented Jan 3, 2025

If we merge this one first:

OK, can we test the three PRs with depends-on, though I think it would work? I guess you've to also remove that from kuttl tests in openstack-operator.

I'm not sure if depends-on is going to work here actually. Because this change will depend on the references being removed from the openstack-operator now, but the openstack-operator will need to use a go.mod replaces to use this code. So there's a circular dependency there. I think I'll now need to bump the version of the openstack-baremetal-operator in the openstack-operator repo, then remove the param from the functional tests, merge that change. Then we can loop back to this one.

@rabi
Copy link
Contributor

rabi commented Jan 3, 2025

I'm not sure if depends-on is going to work here actually. Because this change will depend on the references being removed from the openstack-operator now, but the openstack-operator will need to use a go.mod replaces to use this code. So there's a circular dependency there. I think I'll now need to bump the version of the openstack-baremetal-operator in the openstack-operator repo, then remove the param from the functional tests, merge that change. Then we can loop back to this one.

I've changed the openstack-operator functional tests to not use baremetal-operator API directly in openstack-k8s-operators/openstack-operator#1248. I think that would allow us not to break dataplane functional tests when we make certain changes in baremetal-operator API.

@bshephar
Copy link
Contributor Author

bshephar commented Jan 3, 2025

/test openstack-baremetal-operator-build-deploy

@bshephar
Copy link
Contributor Author

bshephar commented Jan 3, 2025

recheck

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

@rabi rabi rabi left review comments

@dprince dprince Awaiting requested review from dprince

@viroel viroel Awaiting requested review from viroel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bshephar @rabi