-
Notifications
You must be signed in to change notification settings - Fork 430
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
Make OSDisk Storage Type optional #1321
Make OSDisk Storage Type optional #1321
Conversation
3f64ba3
to
0542dbd
Compare
:sigh: |
0542dbd
to
a74b25e
Compare
a74b25e
to
097afd0
Compare
/retest |
/assign @nader-ziada @shysank |
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.
great PR, just wondering about the naming
097afd0
to
cabc185
Compare
cabc185
to
a58e6a2
Compare
@CecileRobertMichon: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
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.
With all of the individual validate${TYPE}
funcs, it feels like we should have a validate method added to the types via the code generator and then just have it wired up in the webhooks. Then the webhooks can call an empty validate on the top level type and have it cascade to all of the field types.
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.
Overall looks great. Just curious about the name change.
StorageAccountType string `json:"storageAccountType"` | ||
DiskEncryptionSet *DiskEncryptionSetParameters `json:"diskEncryptionSet,omitempty"` | ||
// ManagedDiskParameters defines the parameters of a managed disk. | ||
type ManagedDiskParameters struct { |
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.
I'm curious about the name change. Why ManagedDiskParameters
rather than ManagedDisk
?
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.
Aligns with the Azure types & makes it clear this isn't about optionally enabling managed disks but rather about how the managed disk is configured.
thanks for addressing all the comments Cecile lgtm |
/lgtm |
/test pull-cluster-api-provider-azure-e2e-windows |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
What type of PR is this?
What this PR does / why we need it: Making OS disk ManagedDisk.StorageAccountType optional as it is not actually a required field for Azure APIs.
VM sizes without "s" support both Standard HDD and Standard SSD, default is Standard HDD. VM sizes with an "s" support Premium SSD, Standard HDD and Standard SSD, defaults to Premium SSD. Some select VM sizes support Ultra SSD for data disks.
Also fixed the ephemeral OS disk flavor to use ReadOnly caching and added validation for caching types.
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 partially #618
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: