Skip to content

Adding appRole property to ApplicationCreateParameters and ApplicationUpdateParameters #3165

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
Closed
wants to merge 3 commits into from
Closed

Adding appRole property to ApplicationCreateParameters and ApplicationUpdateParameters #3165

wants to merge 3 commits into from

Conversation

jillcary
Copy link

@jillcary jillcary commented May 31, 2018 •
edited by azuresdkci
Loading

  • Add appRole property to ApplicationCreate and ApplicationUpdate
    parameters
  • Validated swagger api changes

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@jillcary
Adding appRole property
f8fe1dd 
* Add appRole property to ApplicationCreate and ApplicationUpdate
parameters
* Validated swagger api changes
@azuresdkci azuresdkci requested a review from hovsepm May 31, 2018 00:29
@AutorestCI
Copy link

AutorestCI commented May 31, 2018 •
edited
Loading

Automation for azure-libraries-for-java

A PR has been created for you:
AutorestCI/azure-libraries-for-java#216

@AutorestCI
Copy link

AutorestCI commented May 31, 2018 •
edited
Loading

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#2931

@AutorestCI
Copy link

AutorestCI commented May 31, 2018 •
edited
Loading

Automation for azure-sdk-for-ruby

A PR has been created for you:
Azure/azure-sdk-for-ruby#1380

@AutorestCI
Copy link

AutorestCI commented May 31, 2018 •
edited
Loading

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#2032

@AutorestCI
Copy link

AutorestCI commented May 31, 2018 •
edited
Loading

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#1955

hovsepm
hovsepm reviewed May 31, 2018
View reviewed changes
specification/graphrbac/data-plane/stable/1.6/graphrbac.json Outdated
"appRoles": {
"type": "array",
"items": {
"type": "string"
Copy link
Contributor

Choose a reason for hiding this comment

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

please doublecheck. your other modification declare "$ref": "#/definitions/appRole" types as array elements but here you have type string. It is kind of inconsistent.

@hovsepm
Copy link
Contributor

hovsepm commented May 31, 2018

@jillcary please fix the linter error - "Error: Could not resolve reference #/definitions/appRole"

@jillcary
Add definition for AppRole object
b8823f8 
* add definition for AppRole object referred to by AppRoles array
* Capitalize AppRole in reference
* Update all appRoles arrays to have AppRole type for consistency
This was referenced May 31, 2018
Closed
Closed
Closed
Closed
Closed
@hovsepm
Copy link
Contributor

hovsepm commented Jun 1, 2018

@jillcary please add/modify operation examples that will show valid values for newly added type(s).

@jillcary
Added example values for new types
a411487 
* Included example and allowed values for allowedMemberTypes,
  displayName, id, and value.
* Used examples from
https://developer.microsoft.com/graph/docs/api-reference/beta/resources/approle
@AutorestCI AutorestCI mentioned this pull request Jun 4, 2018
Closed
@hovsepm
Copy link
Contributor

hovsepm commented Jun 8, 2018 •
edited
Loading

@jillcary your PR introduces a breaking change by adding a new property in the Put(Create) and Get scenarios. The suggested way to do this kind of changes it to park them till next API version update because it adds a new functionality in the already shipped SDK. SDK users will actually be cleaning out the values that were added previously without actually realizing it. Imagine the following scenario:

  1. app using new version of SDK (generated after your changes) creates a record that contains AppRole fields
  2. app using old version of SDK (without these changes) gets the record and deserialize all the fields except AppRole because simply it does not know about that field.
  3. the app from n2 now does some modification to other fields and want to update the server record. And as a result of this update it will not provide AppRole values because simply it was thrown away. So AppRole values are just gone.

Please discuss this flow with your team and provide your decision here.

@azuresdkci azuresdkci assigned sergey-shandar and unassigned hovsepm Jun 8, 2018
@azuresdkci azuresdkci requested a review from sergey-shandar June 8, 2018 21:34
@jillcary jillcary mentioned this pull request Jun 12, 2018
Closed
sergey-shandar
sergey-shandar suggested changes Jun 13, 2018
View reviewed changes
Copy link
Contributor

@sergey-shandar sergey-shandar left a comment

Choose a reason for hiding this comment

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

@jillcary I would recommend to introduce breaking changes to the next API version.

@jillcary jillcary closed this Jun 14, 2018
@jillcary jillcary deleted the jillcary-app-roles branch June 14, 2018 16:55
@AutorestCI AutorestCI mentioned this pull request Jun 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hovsepm hovsepm hovsepm left review comments

@sergey-shandar sergey-shandar sergey-shandar requested changes

Assignees

@sergey-shandar sergey-shandar

Labels
potential-sdk-breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jillcary @AutorestCI @hovsepm @sergey-shandar @azuresdkci