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.

Sign up for GitHub

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

Jump to bottom

[OCPCLOUD-1377] Add "Azure Ultra Disks support in Machine API" enhancement #1021

Merged
merged 1 commit into from
Feb 11, 2022
Merged

[OCPCLOUD-1377] Add "Azure Ultra Disks support in Machine API" enhancement #1021

merged 1 commit into from
Feb 11, 2022

Conversation

damdo
Copy link
Member

@damdo damdo commented Jan 28, 2022

This PR introduces an enhancement for describing how we will introduce the Azure Ultra Disks feature into Machine API.

@damdo damdo force-pushed the azure-ultra-disks branch 2 times, most recently from ac275ad to 89d33ad Compare January 28, 2022 12:01
JoelSpeed
JoelSpeed reviewed Jan 28, 2022
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Got a couple of suggestions about kubebuilder tags on the API definitions, but other LGTM

enhancements/machine-api/azure-ultra-disks.md Show resolved Hide resolved
enhancements/machine-api/azure-ultra-disks.md Show resolved Hide resolved
enhancements/machine-api/azure-ultra-disks.md Show resolved Hide resolved
JoelSpeed
JoelSpeed reviewed Feb 1, 2022
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Changes look good to me and I'm happy to add my approve, this will need an API review before we do though

enhancements/machine-api/azure-ultra-disks.md Outdated Show resolved Hide resolved
enhancements/machine-api/azure-ultra-disks.md Outdated Show resolved Hide resolved
@damdo
Copy link
Member Author

damdo commented Feb 7, 2022

Any of the @openshift/api-reviewers folks available for an api review? Thanks.

JoelSpeed
JoelSpeed reviewed Feb 10, 2022
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Looks good, couple of suggestions on the API and a question about the upgrade/downgrade section, otherwise I'm happy for this to merge

enhancements/machine-api/azure-ultra-disks.md Show resolved Hide resolved
enhancements/machine-api/azure-ultra-disks.md Show resolved Hide resolved
enhancements/machine-api/azure-ultra-disks.md Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Contributor

Ok, I'm happy with this now, will need an api approver to give it a once over before we can merge though
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2022
@JoelSpeed
Copy link
Contributor

Just spoken with @deads2k on slack, he's happy to defer the API review for now given I've already given it a thorough review. I'd like a second pair of eyes on this from my team before we merge though

elmiko
elmiko reviewed Feb 10, 2022
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this is looking great @damdo , i found one minor spelling error but otherwise i'm
/lgtm

enhancements/machine-api/azure-ultra-disks.md Outdated Show resolved Hide resolved
JoelSpeed
JoelSpeed reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

@damdo If you get the typo and metadata fixed up, I'll add the labels to merge

enhancements/machine-api/azure-ultra-disks.md Outdated Show resolved Hide resolved
enhancements/machine-api/azure-ultra-disks.md Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2022
@JoelSpeed
Copy link
Contributor

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 11, 2022

@damdo: all tests passed!

Full PR test history. Your PR dashboard.

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.

@JoelSpeed
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2022
@openshift-merge-robot openshift-merge-robot merged commit 67bfbdf into openshift:master Feb 11, 2022
damdo added a commit to damdo/api that referenced this pull request Feb 11, 2022
@damdo
machine-api: azureprovider: add ultra disk support
9615017 
implements the API changes described in openshift/enhancements#1021
@damdo damdo mentioned this pull request Feb 11, 2022
Merged
damdo added a commit to damdo/api that referenced this pull request Feb 11, 2022
@damdo
machine-api: azureprovider: add ultra disk support
b7bafa5 
implements the API changes described in openshift/enhancements#1021
damdo added a commit to damdo/api that referenced this pull request Feb 11, 2022
@damdo
machine-api: azureprovider: add ultra disk support
17d1cba 
implements the API changes described in openshift/enhancements#1021
damdo added a commit to damdo/api that referenced this pull request Feb 11, 2022
@damdo
machine-api: azureprovider: add ultra disk support
029ed23 
implements the API changes described in openshift/enhancements#1021
@JoelSpeed JoelSpeed mentioned this pull request Mar 3, 2022
Closed
damdo added a commit to damdo/api that referenced this pull request Mar 8, 2022
@damdo
machine-api: azureprovider: add ultra disk support
e7eb24c 
implements the API changes described in openshift/enhancements#1021
openshift-merge-robot pushed a commit to openshift/api that referenced this pull request Mar 10, 2022
@damdo
[OCPCLOUD-1422] machine-api: azureprovider: add ultra disk support (#…
abf6417 
…1119)

* machine-api: azureprovider: add ultra disk support

implements the API changes described in openshift/enhancements#1021

* machine: split managed disk parameters

* machine: ultra disks: split ManagedDiskParameters and DataDiskManagedDiskParameters

* machine: ultra disks: specify default values in godoc

* machine: ultra disks: improve UltraSSDCapability godoc

* machine: ultra disks: drop LUN

* machine: ultra disks: rename ManagedDiskParameters to OSDiskManagedDiskParameters

* machine: ultra disks: add Enum and validation for DataDisk StorageAccountType

* Revert "machine: ultra disks: drop LUN"

This reverts commit 0f64a58.

* machine: ultra disks: update LUN api

* machine: ultra disks: Data Disk godoc
@damdo damdo mentioned this pull request Mar 14, 2022
Merged
@damdo damdo mentioned this pull request Mar 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-demicev alexander-demicev alexander-demicev left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@elmiko elmiko elmiko left review comments

@bparees bparees Awaiting requested review from bparees

@jerpeter1 jerpeter1 Awaiting requested review from jerpeter1

Assignees

@elmiko elmiko

@JoelSpeed JoelSpeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@damdo @JoelSpeed @elmiko @alexander-demicev @openshift-merge-robot