Skip to content

Route Filter Feature Support from Express Route#2795

Closed
dihan0604 wants to merge 0 commit intoAzure:AutoRestfrom
dihan0604:AutoRest
Closed

Route Filter Feature Support from Express Route#2795
dihan0604 wants to merge 0 commit intoAzure:AutoRestfrom
dihan0604:AutoRest

Conversation

@dihan0604
Copy link
Copy Markdown
Contributor

@dihan0604 dihan0604 commented Feb 7, 2017

Description


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

General Guidelines

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as a link to the swagger spec, used to generate the code.
  • The project.json and AssemblyInfo.cs files have been updated with the new version of the SDK.

@dihan0604
Copy link
Copy Markdown
Contributor Author

Swagger Spec:
Azure/azure-rest-api-specs#855

@cormacpayne
Copy link
Copy Markdown
Member

@dihan0604 it looks like the following session record was not found: Network.Tests.Tests.RouteFilterTests\EmptyRouteFilterTest.json

@dihan0604
Copy link
Copy Markdown
Contributor Author

@cormacpayne , I debugged this with Abhijeet and we realize we can not have tests in SDK. My feature is enabled in brazilUS (Stage ARM), however, for the current SDK tests, we do not provide this environment. I discussed with Abhijeet and he agreed that I can add tests in PS instead of SDK. I have removed the tests from this PR. Once brazilUS environment is available in SDK, I will add these tests back.

@cormacpayne
Copy link
Copy Markdown
Member

@azuresdkci test this please

Copy link
Copy Markdown
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

@dihan0604 a few comments about breaking changes in this pull request. Since this SDK is in preview, it is fine to introduce breaking changes, but you will need to mitigate these changes in PowerShell

Also, you have a large number of commits, so you are going to need to rebase to clean this up

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dihan0604 changing the order of parameters in the constructor is a breaking change

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dihan0604 changing the name of this class is a breaking change

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dihan0604 removing this class is a breaking change

@cormacpayne
Copy link
Copy Markdown
Member

@dihan0604 it looks like @lamchester just opened a similar PR to this one (#2798). Can we try and consolidate both PR changes into one?

@dihan0604
Copy link
Copy Markdown
Contributor Author

@cormacpayne Comments addressed

@dihan0604
Copy link
Copy Markdown
Contributor Author

@cormacpayne , I chatted with Chester and he agrees to wait my PR get merged and he makes his PR on top of mine. This is due to my swagger changes already merged but not the SDK.

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.

3 participants