Skip to content

Conversation

@wenbof-zz
Copy link

@wenbof-zz wenbof-zz commented Jul 31, 2019

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jul 31, 2019

In Testing, Please Ignore

[Logs] (Generated from 640aecc, Iteration 6)

Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Warning Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
  • Warning datafactory/resource-manager/v2017_09_01_preview [Logs]
  • Warning datafactory/resource-manager/v2018_06_01 [Logs]
Warning Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Warning JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
  • Warning @azure/arm-datafactory [Logs]

@wenbof-zz
Copy link
Author

the related SDK built and local tests which run well when testing locally: wenbof-zx/azure-sdk-for-net@a2f1622

@AutorestCI
Copy link

AutorestCI commented Jul 31, 2019

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#5747

@AutorestCI
Copy link

AutorestCI commented Jul 31, 2019

Automation for azure-sdk-for-go

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

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@michaeljqzq michaeljqzq added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 1, 2019
}
},
"AmazonRedshiftTableDataset": {
"x-ms-discriminator-value": "AmazonRedshiftTable",
Copy link
Member

@majastrz majastrz Aug 2, 2019

Choose a reason for hiding this comment

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

AmazonRedshiftTable [](start = 35, length = 19)

So you're adding a new discriminator value to an existing stable API. The SDK generated from the swagger before the change will be able to deal with the discriminator value of "AmazonRedshiftTable" because it's modeled as a string, but it will not be able to deserialize the payload specific to AmazonRedshiftTable. This unfortunately makes it an SDK breaking change. Can you add this in a new API version? #Closed

Copy link
Author

@wenbof-zz wenbof-zz Aug 4, 2019

Choose a reason for hiding this comment

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

Sure. it will be generated in a new API version when making changes to SDK. #Closed

Copy link
Author

@wenbof-zz wenbof-zz Aug 4, 2019

Choose a reason for hiding this comment

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

as similar as this SDK change where you can see that we change the API version Azure/azure-sdk-for-net#7044 #Closed

Copy link
Member

Choose a reason for hiding this comment

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

To generate a new API version when making changes to SDK, you need the swagger to exist for the new API version, no?


In reply to: 310406114 [](ancestors = 310406114)

Copy link
Author

@wenbof-zz wenbof-zz Aug 5, 2019

Choose a reason for hiding this comment

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

right. the order I understand is also the same as you : merge swagger change first, then generate SDK based on the latest swagger. #Closed

Copy link
Author

@wenbof-zz wenbof-zz Aug 5, 2019

Choose a reason for hiding this comment

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

the new SDK version is always built based on the latest swagger. which means the old SDK were built based on swagger at specific time and then kept intact there. #Closed

Copy link
Member

@majastrz majastrz Aug 5, 2019

Choose a reason for hiding this comment

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

Let's not assume. Can you verify the behavior? What I want to know if it will throw. If the SDK just accepts the payload even if the user can't really interact with it, I'd be ok with that.

@michaeljqzq, can you chime in here as well? What is the behavior with the generated SDK if a new discriminator value is being returned in the backend, but the SDK doesn't know about it yet? Will it throw an exception? #Closed

Copy link
Author

@wenbof-zz wenbof-zz Aug 5, 2019

Choose a reason for hiding this comment

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

I double checked our changes on SDK and can confirm that we manually changed the version and appending the new changes on existing one every time to the new version. @hvermis could you please also help confirm as well? thanks. #Closed

Copy link
Contributor

@hvermis hvermis Aug 6, 2019

Choose a reason for hiding this comment

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

We do not update the API version. This is a known behavior and we have been adding new types like this. Our customers are aware of this. #Closed

Copy link
Member

@majastrz majastrz Aug 6, 2019

Choose a reason for hiding this comment

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

@wenbof and @hvermis, you are not actually answering my question, which is delaying this review. I'm not asking whether you have done this before (you clearly have). Azure is a distributed system. There's no mechanism to ensure that all customers simultaneously upgrade to latest and greatest SDK that you have generated, which is why we have breaking changes guidance.

So I will ask the question again in a different way. Let's say you have SDK v2 that supports connectors a and b and SDK v3 that supports connectors a, b, and c. User A has been happily using SDK v2 to manage resources using connectors a and b. Suddenly user B creates resources using connector c in the same subscription or RG via SDK v3.

What will User A now see when he/she queries for all resources?

  1. Will the old SDK v2 throw an exception?
  2. Will the responses deserialize successfully but incompletely?

Please pick either answer 1 or 2. #Closed

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

I added some questions. Please take a look.

@wenbof-zz
Copy link
Author

I added some questions. Please take a look.
replied with my comments. could you please double review? thanks

@wenbof-zz
Copy link
Author

@majastrz could you please check whether you have any further concerns or comments? thanks.

@hvermis
Copy link
Contributor

hvermis commented Aug 6, 2019

@majastrz Answer to yourquestion is #2. This done by ClientRuntime library which is one of common libraries owned by the Azure SDK team. #Closed

@majastrz
Copy link
Member

majastrz commented Aug 6, 2019

@majastrz Answer to yourquestion is #2. This done by ClientRuntime library which is one of common libraries owned by the Azure SDK team.

Great! Thanks for confirming #Closed

@majastrz majastrz added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 6, 2019
Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

Signed off from ARM side.

@michaeljqzq michaeljqzq merged commit 0e1793c into Azure:master Aug 7, 2019
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.

8 participants