Skip to content

Add API Management Version Sets #2483

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

Merged
anuchandy merged 9 commits into from
Feb 21, 2018
Merged

Add API Management Version Sets #2483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b255d0
Added ApiVersionSets
darrelmiller Feb 9, 2018
84cbee3
Merge remote-tracking branch 'upstream/master' into dm/versionsets
darrelmiller Feb 9, 2018
5fec97d
Fixes based on CR
darrelmiller Feb 9, 2018
74fbf1f
Merged with master
darrelmiller Feb 10, 2018
0aed758
Refactored JSON Schemas for payloads
darrelmiller Feb 12, 2018
cc6f70e
Merge remote-tracking branch 'upstream/master' into dm/versionsets
darrelmiller Feb 12, 2018
8578f92
fixed names and reference
darrelmiller Feb 13, 2018
e1f8c06
Fixed reference to schema for ApiVersionSets
darrelmiller Feb 13, 2018
0c42317
Fixed ApiVersionSetCreate example
darrelmiller Feb 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 2 additions & 30 deletions ...on/apimanagement/resource-manager/Microsoft.ApiManagement/stable/2017-03-01/apimapis.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2203,7 +2203,7 @@
"description": "Describes on which protocols the operations in this API can be invoked."
},
"apiVersionSet" : {
"$ref" : "#/definitions/ApiVersionSetContract"
"$ref" : "./apimversionsets.json#/definitions/ApiVersionSetContract"
}
},
"allOf": [
Expand Down Expand Up @@ -2424,38 +2424,10 @@
},
"apiVersionSet" : {
"description": "Version set details",
"$ref" : "#/definitions/ApiVersionSetContract"
"$ref" : "./apimversionsets.json#/definitions/ApiVersionSetContractProperties"
Copy link
Copy Markdown
Member

@anuchandy anuchandy Feb 14, 2018

Choose a reason for hiding this comment

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

Note that changing the type of the property apiVersionSet in this model is breaking change.

If we take the diff of ApiVersionSetContract (old type) and ApiVersionSetContractProperties (new type), it looks like the read-only properties id, name and type won't be available for consumer any more.

Changing the type is a SDK breaking change. This requires major SDK version upgrade.

Removing properties in the response of a stable api call (without bumping api version) is service side breaking change, which requires ARM approval.

Copy link
Copy Markdown
Member Author

@darrelmiller darrelmiller Feb 14, 2018

Choose a reason for hiding this comment

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

@anuchandy This isn't breaking a change to the current API, this is fixing an incorrect description. Customer's who attempt to update those fields in the GA release will not be able to.

In the preview release of this API, the customers could create ApiVersionSet directly when creating the API, but it was decided that the ARM guidelines prevented this from being allowed. Therefore we had to remove this functionality for our customers in our GA release. The change to make the fields read-only was missed when doing the update. Our customers are now waiting for the documentation to be updated so they can use the new ARM compliant way of creating these things in a two step process.

If you could unblock this PR so that we can get that documentation to the customer, it would be most appreciated.

/cc @ravbhatnagar

Copy link
Copy Markdown
Member

@anuchandy anuchandy Feb 15, 2018

Choose a reason for hiding this comment

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

@darrelmiller -
One question, is it the case that service never used to return id, name and type hence swagger was wrong and we are fixing that issue in this PR?

Let me also explain the reason I am thinking this as SDK breaking change. All below SDK issues can be addressed by bumping up SDK version.

The ApiRevisionInfoContract in the current SDK

class ApiRevisionInfoContract {
  ....
  public ApiVersionSetContract apiVersionSet {get; set;}
}

The ApiRevisionInfoContract in the new SDK (based on this PR)

class ApiRevisionInfoContract {
  ....
  public ApiVersionSetContractProperties apiVersionSet {get; set;}
}

application code using current sdk

  ApiRevisionInfoContract revisionContract = ...;
  ApiVersionSetContract c = revisionContract.apiVersionSet;

application code using new sdk (based on this PR) [broken]

  ApiRevisionInfoContract revisionContract = ...;
  ApiVersionSetContract c = revisionContract.apiVersionSet; // Compiler error due to different return type

In this prespective this is a SDK breaking change. When an existing application based on current SDK switch to new SDK (generated from this PR), if their application is using ApiRevisionInfoContract.apiVersionSet property then they need to change the type.

This issue can be addressed by bumping up the major version of the SDK.

As i mentioned earlier, the read-only properties id, name and type won't be available when customer access ApiRevisionInfoContract.apiVersionSet, right?

application code using current sdk

 ApiRevisionInfoContract revisionContract = ...;
 System.out.println(revisionContract.id); // no compiler error

application code using new sdk (based on this PR) [broken]

 ApiRevisionInfoContract revisionContract = ...;
 System.out.println(revisionContract.id); // compiler error, id property does not exists.

again if we bump up SDK version then this is fine.

Copy link
Copy Markdown
Member

@anuchandy anuchandy Feb 15, 2018

Choose a reason for hiding this comment

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

Also we need ARM review because there is a new swagger apimversionsets.json, this is the first PR adding this swagger. So as per the established process, it is required to have this sign-ed off by ARM in this repo. I'm sending an email thread requesting ARM to take a look.

Copy link
Copy Markdown
Contributor

@ravbhatnagar ravbhatnagar Feb 16, 2018

Choose a reason for hiding this comment

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

@anuchandy - I think what @darrelmiller is saying is that at GA, customer could not update/assign the value of id, name, type since they were read only. Swagger didnt reflect it. And so now, swagger is updating the description correctly to reflect the service state. If true, this is not a breaking change, since it would have never worked for the customer even if he tried to update.

Copy link
Copy Markdown
Member

@anuchandy anuchandy Feb 16, 2018

Choose a reason for hiding this comment

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

thanks @ravbhatnagar got it. Still when customer compiles existing application (that was based current SDK) with the new sdk (generated from this PR), they will see compiler error. That should be fine given it's a side effect of correcting the swagger to reflect actual service state.

Copy link
Copy Markdown
Member Author

@darrelmiller darrelmiller Feb 16, 2018

Choose a reason for hiding this comment

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

@anuchandy The current SDK is from March 2017 and is a preview https://www.nuget.org/packages/Microsoft.Azure.Management.ApiManagement/3.4.0-preview

Copy link
Copy Markdown
Member

@anuchandy anuchandy Feb 16, 2018

Choose a reason for hiding this comment

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

@darrelmiller thanks for letting me know. Based on the process we established, i'll add potential SDK breaking changes tag for future reference.

Once we conclude the below discussion with ARM team, will merge this PR.

}
}
},
"ApiVersionSetContract" : {
"description": "An API Version Set contains the common configuration for a set of API Versions relating ",
"properties": {
"id" : {
"type": "string",
"description": "Identifier for existing API Version Set. Omit this value to create a new Version Set."
},
"description": {
"type": "string",
"description": "Description of API Version Set."
},
"versioningScheme": {
"type": "string",
"description": "An value that determines where the API Version identifer will be located in a HTTP request.",
"enum": [
"Segment", "Query", "Header"
]
},
"versionQueryName": {
"type": "string",
"description": "Name of query parameter that indicates the API Version if versioningScheme is set to `query`."
},
"versionHeaderName": {
"type": "string",
"description": "Name of HTTP header parameter that indicates the API Version if versioningScheme is set to `header`."
}
}
},
"ApiReleaseCollection": {
"properties": {
"value": {
Expand Down
Loading