cluster-api-provider-azure icon indicating copy to clipboard operation
cluster-api-provider-azure copied to clipboard

Update godoc of CapacityReservationGroupID of AzureMachineSpec

Open arkadeepsen opened this issue 1 year ago • 8 comments

What type of PR is this? /kind cleanup

What this PR does / why we need it: This PR updates the godoc of CapacityReservationGroupID of AzureMachineSpec. The updated godoc contains the explanation of what the requirements are for the CapacityReservationGroupID.

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

Special notes for your reviewer:

  • [ ] cherry-pick candidate

TODOs:

  • [ ] squashed commits
  • [ ] includes documentation
  • [ ] adds unit tests

Release note:

NONE

arkadeepsen avatar May 10 '24 12:05 arkadeepsen

Hi @arkadeepsen. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

k8s-ci-robot avatar May 10 '24 12:05 k8s-ci-robot

Thanks @arkadeepsen!

The azuremachine_types.go file is used in generating the CRDs. So you need to run make generate and commit those changes or this will fail the make verify-gen check.

mboersma avatar May 10 '24 16:05 mboersma

/ok-to-test

mboersma avatar May 10 '24 16:05 mboersma

Thanks @arkadeepsen!

The azuremachine_types.go file is used in generating the CRDs. So you need to run make generate and commit those changes or this will fail the make verify-gen check.

@mboersma my bad. Thanks for pointing that out. I have run make generate and pushed the changes.

arkadeepsen avatar May 10 '24 16:05 arkadeepsen

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 62.02%. Comparing base (43e72a9) to head (de135dc). Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4830      +/-   ##
==========================================
+ Coverage   62.01%   62.02%   +0.01%     
==========================================
  Files         201      201              
  Lines       16858    16858              
==========================================
+ Hits        10455    10457       +2     
+ Misses       5620     5618       -2     
  Partials      783      783              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 10 '24 17:05 codecov[bot]

I see the validation is already being done in ValidateCapacityReservationGroupID. I think the clarity helps but other fields that have similar requirements do not include this information in the godoc. @jackfrancis @mboersma what's your take on this? Was just wondering for the sake of consistency, otherwise lgtm.

Providing a context as to why the change in the godoc is introduced. In OpenShift specific APIs, we try to provide information in the godoc which will be beneficial for the user to create/update a CR. Though, as pointed out, the information is provided when the validation fails, it's good to have the information available beforehand. P.S. I made the change to the godoc specific to CapacityReservationGroupID as I had worked on the PR which adds the field.

arkadeepsen avatar May 10 '24 19:05 arkadeepsen

@willie-yao it's a good point–we could argue it should be documented similarly for many other fields. I just assumed this change in particular would be (would have been) helpful for the requestor and perhaps others, and wasn't bothered by the inconsistency. But we should hear from other maintainers.

mboersma avatar May 10 '24 19:05 mboersma

LGTM label has been added.

Git tree hash: f1440a9f88f5752096ad2940d2f4763c8c17d6ac

k8s-ci-robot avatar May 14 '24 15:05 k8s-ci-robot

/assign @willie-yao for approval

mboersma avatar May 14 '24 17:05 mboersma

@mboersma: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes-sigs members with read permissions, 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 @willie-yao for approval

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-sigs/prow repository.

k8s-ci-robot avatar May 14 '24 17:05 k8s-ci-robot

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: willie-yao

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 avatar May 14 '24 17:05 k8s-ci-robot