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.

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

Add new OsType parameters for Microsoft.Web/availablestacks #3808

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

balag0
Copy link
Contributor

@balag0 balag0 commented Sep 5, 2018

No description provided.

@AutorestCI
Copy link

AutorestCI commented Sep 5, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Sep 5, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3312

@AutorestCI
Copy link

AutorestCI commented Sep 5, 2018

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@AutorestCI
Copy link

AutorestCI commented Sep 5, 2018

Automation for azure-sdk-for-java

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-java#2218

@AutorestCI
Copy link

AutorestCI commented Sep 5, 2018

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#3560

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

Copy link
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

I get nervous about extending enum values inside of API Versions for the following reasons:

  1. API Versions expose a mechanism for reasoning about availability between regions.
  2. While this type already looks to be modeled as a string, applications will need to be retooled to expose/reason about the new options. Without changing API Version, it is hard to build tooling to identify places where the enum list has been updated.

That said, I don't think those grounds are enough to block this or any other individual PR. However, I want to make sure that this conversation is included when considering what changes warrant waiting for an API Version boundary. Better yet, trying to change the culture to one where deploying new API Versions is a lighter-weight, more common operation.

@marstr marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 6, 2018
@ravbhatnagar
Copy link
Contributor

Looks good. Signing off from ARM side. Just one minor note - Currently in the response body, i couldnt find a way to know which stack is for Linux or Windows. How do you differentiate that looking at the response. Specially when the filter query parameter is optional so by default in the response both stacks can be present.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Sep 6, 2018
@balag0
Copy link
Contributor Author

balag0 commented Sep 6, 2018

The response body will include the type based on the ostype query parameter.
Ex:
Microsoft.Web/availableStacks?osTypeSelected=Linux
Microsoft.Web/availableStacks?osTypeSelected=Windows

if the query parameter is skipped it is implicitly assumed to be Windows and only Windows stacks will be returned.

@naveedaz
Copy link
Contributor

naveedaz commented Sep 7, 2018

Are there any more concerns? Can we merge this PR?

@marstr
Copy link
Member

marstr commented Sep 10, 2018

Talked offline with @naveedaz, merging now.

@marstr marstr merged commit ca4d82a into Azure:master Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants