Skip to content

Conversation

@aim-for-better
Copy link
Member

@aim-for-better aim-for-better commented Apr 2, 2021

Description

Use gettting default sku api to set workernode and headnode size if customer does not provide explictly

Testing Guide

History Notes

[HDInsight] BREAKING CHANGE: az hdinsight create: Use getting default sku api to set workernode and headnode size if customer does not provide.


This checklist is used to make sure that common guidelines for a pull request are followed.

dkmiller and others added 24 commits April 27, 2020 16:13
@aim-for-better aim-for-better changed the title {HDInsight}Using get default sku api to set workernode and headnode size if customer does not provide {HDInsight}Use getting default sku api to set workernode and headnode size if customer does not provide Apr 2, 2021
@aim-for-better aim-for-better added this to the S185 milestone Apr 2, 2021
@yonzhan yonzhan requested a review from msyyc April 2, 2021 08:09
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 2, 2021

HDInsight

@aim-for-better
Copy link
Member Author

Hi @Juliehzl @kairu-ms @msyyc Could you please help review this PR? We want to release in S185. Thank you~

Copy link
Contributor

@Juliehzl Juliehzl Apr 7, 2021

Choose a reason for hiding this comment

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

In this way, you will change default value for your CLI parameter, which is breaking. Is it by design?

Copy link
Member Author

@aim-for-better aim-for-better Apr 7, 2021

Choose a reason for hiding this comment

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

In this way, you will change default value for your CLI parameter, which is breaking. Is it by design?

Yes, we want to do this, but I forget it is a breaking change. This change will not have any impact on existing scripts, yes but it will change the parameter help info. Can we check in in this release cycle?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a good experience for customer to have such breaking change in stable azure cli. Is there strong business justification for such breaking change?

Copy link
Member Author

@aim-for-better aim-for-better Apr 8, 2021

Choose a reason for hiding this comment

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

It is not a good experience for customer to have such breaking change in stable azure cli. Is there strong business justification for such breaking change?

Because at before the default value is large, and it will be converted into some concrete value. As time goes, we will deprecate some vm size this will cause issue. If it is not acceptable for this release, it also makes sense for us. And when is the next release which accepts breaking change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

S187 - 05/25/2021 release cycle could accept breaking change. CLI only accepts breaking change twice a year.

Copy link
Member Author

@aim-for-better aim-for-better Apr 8, 2021

Choose a reason for hiding this comment

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

S187 - 05/25/2021 release cycle could accept breaking change. CLI only accepts breaking change twice a year.

Hi @yonzhan Thanks. Then this PR will target at S187, and I will announce there will be breaking change in S186. You can skip this PR in this S185. Thank you~

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.

@kairu-ms
Copy link
Contributor

kairu-ms commented May 7, 2021

Please fix conflicts and update History Notes in PR comments.

@Juliehzl
Copy link
Contributor

@aim-for-better could you make CI pass asap?

@aim-for-better
Copy link
Member Author

Hi @Juliehzl @kairu-ms The CI passed now. Thanks~

@Juliehzl Juliehzl merged commit 71a1226 into Azure:dev May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants