-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
📖 Update Azure spot types to use string instead of float #3063
📖 Update Azure spot types to use string instead of float #3063
Conversation
@@ -214,7 +214,7 @@ when the instance is created: | |||
(Delete is supported for VMs as part of VMSS only). | |||
|
|||
- BillingProfile (default: -1) : This is a struct containing a single field, `MaxPrice`. | |||
This is a float representation of the maximum price the user wishes to pay for their VM. | |||
This is a string representation of the maximum price the user wishes to pay for their VM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a small comment about why we're using a string for the record? It might not be obvious to future readers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/assign @CecileRobertMichon
/milestone v0.3.6
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, vincepri 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 |
b06ce0a
to
dcfd137
Compare
@CecileRobertMichon I've added a link to a comment that I think will be helpful for future readers, do you think that's a good link to use? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
What this PR does / why we need it:
Update the Spot instances proposal to clarify that the Azure types should use string instead of float as originally noted.
It was noticed during implementation (https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/559/files#r424579944) that floats are not allowed by the controller-tools crd generator tool
Ref: kubernetes-sigs/controller-tools#245
The alternative to this is to use a
resource.Quantity
which is allowed, but I'm not sure if that adds much benefit over using a stringCC @CecileRobertMichon