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

Platform specific fields in BYOHost CRD #479

Merged

Conversation

sachinkumarsingh092
Copy link
Contributor

@sachinkumarsingh092 sachinkumarsingh092 commented Apr 8, 2022

What this PR does / why we need it:
We need platform-specific info regarding the host in BYOHost CRDs.

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

Additional information

Changes in brief:

  • /apis/infrastructure/v1beta1/byohost_types.go: We introduce new type HostStatus which has all the platformspecific details like OS name, Kernel version, architecture etc. We introduce a new field in ByoHostStatus with this type.
  • /config/crd/bases/infrastructure.cluster.x-k8s.io_byohosts.yaml: Using make generate, we let controller-gen introduce these new fields in the CRDs automatically.
  • /apis/infrastructure/v1beta1/zz_generated.deepcopy.go: Autogenerated implementation of the deepcopy methods for type HostStatus.

Special notes for your reviewer

<none>

@mayur-tolexo
Copy link
Contributor

Just curious, why host OS and Arch details are not a part of Spec and added in Status of ByoHost?

@sachinkumarsingh092
Copy link
Contributor Author

sachinkumarsingh092 commented Apr 11, 2022

why host OS and Arch details are not a part of Spec and added in Status of ByoHost

@mayur-tolexo according to kubebuilder docs, Spec holds desired state, so any inputs to our controller goes here while Status holds observed state and contains any information we want users or other controllers to be able to easily obtain.
By these definitions, OS and Arch details should be in Status, right?

@mayur-tolexo
Copy link
Contributor

@sachinkumarsingh092, thanks for clarifying. Ya make sense to provide only status subresource access to other controllers for HostInfo.

Copy link
Contributor

@anusha94 anusha94 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@dharmjit dharmjit left a comment

Choose a reason for hiding this comment

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

lgtm

@anusha94 anusha94 merged commit 337e90b into vmware-tanzu:main Apr 11, 2022
@sachinkumarsingh092 sachinkumarsingh092 deleted the platform-fields-byohosts branch April 11, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Host platform related fields to ByoHost CRD
5 participants