-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Added Compute Instance OS Patching Properties #20284
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3719,6 +3719,11 @@ | |
"name" | ||
] | ||
}, | ||
"osImageMetadata": { | ||
"readOnly": true, | ||
"description": "Returns the metadata about the current and latest version for this ComputeInstance", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Returns metadata about the operating system image for this compute instance." < Let's use lower casing for compute instance in alignment with marketing/cela naming requirements. |
||
"$ref": "#/definitions/ImageMetadata" | ||
}, | ||
"connectivityEndpoints": { | ||
"readOnly": true, | ||
"description": "Describes all connectivity endpoints available for this ComputeInstance.", | ||
|
@@ -4851,6 +4856,24 @@ | |
], | ||
"type": "object" | ||
}, | ||
"ImageMetadata": { | ||
"type": "object", | ||
"description": "Specifies the Image Metadata about current ComputeInstance", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Returns metadata about the operating system image for this compute instance." < Let's use lower casing for compute instance in alignment with marketing/cela naming requirements. |
||
"properties": { | ||
"currentImageVersion": { | ||
"type": "string", | ||
"description": "Indicates the current version of OS Base Image, Compute Instance is running on." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifies the current operating system image version this compute instance is running on. |
||
}, | ||
"latestImageVersion": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this and currentImageVersion be readOnly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've got a partner request from Defender for Cloud to make this one patchable. ReadOnly should be good for now, but in case we want to make it patchable in the future, would that be possible later? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. top level property "osImageMetadata" referring to ImageMetadata is itself a read only property.. Shouldn't that suffice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep it read-only per discussion. |
||
"type": "string", | ||
"description": "Indicates the latest OS Base Image available for the Compute Instance." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifies the latest available operating system image version. |
||
}, | ||
"isLatestOsImageVersion": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's drop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't be possible as contract already changed in backend layer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typically, backend implementation shouldn't be a reason to worsen our customer-facing contract definitions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also... is this just basically displaying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is correct. One of the goals it that we will be able to track compliance of compute instance states using Azure Policy. Azure Policies can be defined over boolean properties, but comparing and looking up two values for current==latest seems not an option syntactically. |
||
"type": "boolean", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ARM guidelines (https://armwiki.azurewebsites.net/rp_onboarding/process/api_review_best_practices.html) strongly suggest AGAINST booleans, saying:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case this variable is set as per currentVersion==latestVersion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per discussion, we can make this an Enum with [True, False] to represent minimum. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this require extensive backend change and we don't see changing this variable in future. In future we might consider adding another variable that give more extensive information. We therefore plan to keep it as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do consider this when possible, I'm going to sign off for now. |
||
"description": "Indicates whether the Compute Instance is running latest OS Base Image or not." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifies whether this compute instance is running on the latest operating system image. |
||
} | ||
} | ||
}, | ||
"CustomService": { | ||
"type": "object", | ||
"description": "Specifies the custom service configuration", | ||
|
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.
Let's drop
Os
for consistency with other properties fromisLatestOsImageVersion
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.
Won't be possible as contract already changed in backend layer.