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

Adding Cmdlet New-AzApplicationGatewayRewriteRuleUrlConfiguration to support url rewrite capability for V2 application gateways #10894

Merged
merged 6 commits into from
Jan 21, 2020

Conversation

abjai
Copy link
Contributor

@abjai abjai commented Jan 14, 2020

Description

This should be released as a part of Powershell January Release

Adding Support for UrlConfiguration under RewriteRuleActionset for Application Gateway Url Rewrite Feature

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

Powershell design review link: Azure/azure-powershell-cmdlet-review-pr#339
Swagger PR: Azure/azure-rest-api-specs#7905

Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

Hi @abjai, please see my inline comments. Thanks!
Oh and please also fix the merge conflict.

@isra-fel
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Pull request contains merge conflicts.

@abjai
Copy link
Contributor Author

abjai commented Jan 16, 2020

Hi @abjai, please see my inline comments. Thanks!
Oh and please also fix the merge conflict.

@isra-fel I have addressed your comments. I have added the local feed so that tests can run. Once swagger SDK got published and updated in december-release branch, i will pull those changes and resolve the conflicts.

Please note that we are targetting these changes to be released as a part of January end release.

@isra-fel
Copy link
Member

isra-fel commented Jan 19, 2020

Hi @abjai
I'm good with the change. However the CI is blocked by the merge conflict.

@abjai
Copy link
Contributor Author

abjai commented Jan 20, 2020

Hi @abjai
I'm good with the change. However the CI is blocked by the merge conflict.

@isra-fel : I have removed the conflict changes however i could see that SDK is still not updated in Network.csproj so tests might fail.

@number213 : Can you please update the network.csproj with latest SDK so that tests start passing?

Please note that these changes need to be part of Januray 20 release

@isra-fel
Copy link
Member

So, your test project still relies on 21.0.00preview SDK.

@abjai
Copy link
Contributor Author

abjai commented Jan 20, 2020

So, your test project still relies on 21.0.00preview SDK.

@isra-fel : Yes it is still dependent on the updated SDK published as a part of January 17 release. I don't know the exact version published as a part of SDK release. Adding @number213 to help in updating the SDK version for Network.csproj

@abjai
Copy link
Contributor Author

abjai commented Jan 20, 2020

So, your test project still relies on 21.0.00preview SDK.

@isra-fel : Yes it is still dependent on the updated SDK published as a part of January 17 release. I don't know the exact version published as a part of SDK release. Adding @number213 to help in updating the SDK version for Network.csproj

@isra-fel @number213 has raised a PR to update SDK #10928. Once it is merged, my changes should be able to pass the tests

@abjai abjai changed the base branch from master to network-december January 20, 2020 11:15
@abjai abjai changed the base branch from network-december to master January 20, 2020 11:23
@abjai abjai changed the base branch from master to network-december January 20, 2020 11:37
@abjai
Copy link
Contributor Author

abjai commented Jan 20, 2020

So, your test project still relies on 21.0.00preview SDK.

@isra-fel : Yes it is still dependent on the updated SDK published as a part of January 17 release. I don't know the exact version published as a part of SDK release. Adding @number213 to help in updating the SDK version for Network.csproj

@isra-fel @number213 has raised a PR to update SDK #10928. Once it is merged, my changes should be able to pass the tests

@isra-fel : updated the PR with latest sdk.

Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

LGTM

@isra-fel
Copy link
Member

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@isra-fel
Copy link
Member

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@anton-evseev
Copy link
Contributor

Need to work on PR for merging network into master. Since @isra-fel approved and added "waiting for CI" I'll go ahead and merge this PR

@anton-evseev anton-evseev merged commit 295cdcd into Azure:network-december Jan 21, 2020
dingmeng-xue pushed a commit to dingmeng-xue/azure-powershell that referenced this pull request Mar 29, 2020
Adding Cmdlet New-AzApplicationGatewayRewriteRuleUrlConfiguration to support url rewrite capability for V2 application gateways
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants