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

⚠️ Move InstanceID from Spec to Status #1988

Merged
merged 1 commit into from
Apr 2, 2024
Merged

⚠️ Move InstanceID from Spec to Status #1988

merged 1 commit into from
Apr 2, 2024

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Apr 1, 2024 •
edited
Loading

This is a status field and should not be included in the spec. ProviderID is the same, but cannot be removed as it is required by Cluster API.

This PR replaces #1961 with a smaller scope.

TODO:

/hold

@k8s-ci-robot 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 Apr 1, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 1, 2024 •
edited
Loading

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

Name Link
🔨 Latest commit 5c1ad44
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/660ad176a95ee60008ecace4
😎 Deploy Preview https://deploy-preview-1988--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 configuration.

@mdbooth mdbooth mentioned this pull request Apr 1, 2024
Closed
@mdbooth mdbooth force-pushed the instanceidstatus2 branch 2 times, most recently from 75e2bb0 to a6b46de Compare April 1, 2024 12:36
@mdbooth mdbooth mentioned this pull request Apr 1, 2024
Merged
@mdbooth mdbooth force-pushed the instanceidstatus2 branch from a6b46de to 5c1ad44 Compare April 1, 2024 15:23
@mdbooth
Copy link
Contributor Author

mdbooth commented Apr 1, 2024

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

MaysaMacedo
MaysaMacedo reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@MaysaMacedo MaysaMacedo left a comment

Choose a reason for hiding this comment

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

It lgtm, I just have one question:

api/v1beta1/openstackmachine_types.go
@@ -119,6 +118,10 @@ type OpenStackMachineStatus struct {
// +optional
Ready bool `json:"ready"`

// InstanceID is the OpenStack instance ID for this machine.
// +optional
InstanceID optional.String `json:"instanceID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we considered adding it under the Resolved field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first PR put it in Resources. This made sense, but I've decided against that for now. The reason is I think InstanceID is fundamental to the OpenStackMachine object and something an end user might be interested in (e.g. in printcolumns), but the other properties are not.

I have an idea that I might move actual machine creation into a separate controller with its own CRD, in which case Resources would move to the new controller. In that case I think we'd still want InstanceID to be on this object, so I decided to put it here instead.

None of which precludes adding it to Resources in the future too if it turns out that still makes sense.

@jichenjc
Copy link
Contributor

jichenjc commented Apr 2, 2024

/lgtm

looks good

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

mdbooth commented Apr 2, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2024
@k8s-ci-robot k8s-ci-robot merged commit 4c162b6 into kubernetes-sigs:main Apr 2, 2024
10 checks passed
@mdbooth mdbooth deleted the instanceidstatus2 branch April 2, 2024 09:25
@pierreprinetti pierreprinetti mentioned this pull request Apr 5, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaysaMacedo MaysaMacedo MaysaMacedo left review comments

@jichenjc jichenjc Awaiting requested review from jichenjc

@lentzi90 lentzi90 Awaiting requested review from lentzi90

Assignees

@jichenjc jichenjc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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
CAPO Roadmap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mdbooth @k8s-ci-robot @jichenjc @MaysaMacedo