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

Add host platform to ByoHost CR info on registration #516

Merged
merged 4 commits into from
May 9, 2022

Conversation

shubham14bajpai
Copy link
Contributor

@shubham14bajpai shubham14bajpai commented May 2, 2022

Signed-off-by: Shubham Bajpai [email protected]

What this PR does / why we need it:

This PR adds the host platform information (arch/os/distribution) to the ByoHost CR when the agent registers the host. Tested the same locally and this is what the CR would look like.

$ kubectl get byohosts host1 -oyaml
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: ByoHost
metadata:
  creationTimestamp: "2022-04-25T05:52:54Z"
  generation: 1
  name: host1
  namespace: default
  resourceVersion: "169327"
  uid: 572acde9-2d80-446c-ab7a-260f71e8e8f4
spec: {}
status:
  conditions:
  - lastTransitionTime: "2022-04-25T05:52:55Z"
    reason: WaitingForMachineRefToBeAssigned
    severity: Info
    status: "False"
    type: K8sNodeBootstrapSucceeded
  hostinfo:
    architecture: arm64
    osimage: Ubuntu 20.04.4 LTS
    osname: linux
  network:
  ...

This PR also adds additional printers for the Byohost CRD and with those the output would look like this:

$ kubectl get byohost                                                  
NAME    OSNAME   OSIMAGE              ARCH
host1   linux    Ubuntu 20.04.4 LTS   amd64
host2   linux    Ubuntu 20.04.4 LTS   amd64

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

Additional information
The function added in this PR fetches information directly from files from the underneath operating system. Hence not sure how to add a test for the same.

Special notes for your reviewer

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2022

Codecov Report

Merging #516 (8c81d12) into main (bdcbb70) will decrease coverage by 0.22%.
The diff coverage is 47.05%.

❗ Current head 8c81d12 differs from pull request most recent head cec97de. Consider uploading reports for the commit cec97de to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   63.88%   63.65%   -0.23%     
==========================================
  Files          25       25              
  Lines        1902     1934      +32     
==========================================
+ Hits         1215     1231      +16     
- Misses        608      624      +16     
  Partials       79       79              
Impacted Files Coverage Δ
agent/registration/host_registrar.go 15.09% <47.05%> (+15.09%) ⬆️

agent/registration/host_registrar.go Outdated Show resolved Hide resolved
agent/registration/host_registrar.go Outdated Show resolved Hide resolved
@shubham14bajpai shubham14bajpai force-pushed the host branch 3 times, most recently from 4dce41d to e3308b1 Compare May 5, 2022 06:48
Copy link
Contributor

@sachinkumarsingh092 sachinkumarsingh092 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

@sachinkumarsingh092 sachinkumarsingh092 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

@sachinkumarsingh092 sachinkumarsingh092 left a comment

Choose a reason for hiding this comment

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

LGTM

@dharmjit dharmjit merged commit c6d2137 into vmware-tanzu:main May 9, 2022
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 ByoHost CR with the Host platform information
5 participants