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

[Do Not Merge]Adding Cmdlet New-AzApplicationGatewayRewriteRuleUrlConfiguration to support url rewrite capability for V2 application gateways #10097

Closed
wants to merge 14 commits into from

Conversation

abjai
Copy link
Contributor

@abjai abjai commented Sep 24, 2019

Description

This should not be released as a part of October 15 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: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/339
Swagger link:
Azure/azure-rest-api-specs#7030
Azure/azure-rest-api-specs#7275

@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@abjai abjai added this to the 2019-10-15 - AZ release milestone Sep 24, 2019
@abjai
Copy link
Contributor Author

abjai commented Sep 24, 2019

Changes made in according with NRP 127 (2019-08-01) and targetting network-september branch and need to release by Oct 15. Please verify the milestone and update if it is wrong.

Update: Should not be released as a part of OCT 15.

@@ -973,6 +973,262 @@ function Test-ApplicationGatewayCRUDRewriteRuleSetWithConditions
}
}

function Test-ApplicationGatewayCRUDRewriteRuleSetWithUrlConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test your changes in existing test function like ApplicationGatewayCRUDRewriteRuleSet or ApplicationGatewayCRUDSubItems? I'm concerned that there are a lot of very similar tests in AppGw when each of them takes about 30 minutes to run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For v2 gateways, any update should take maximum 5 mins. In this test, there are two create/update hence it will take maximum 10-12 minutes. You must be seeing 30 mins in the test cases when there are many creates/updates. We have around 20 mins time for V1 gateways so you might see high test running time for V1 gateways.

Is there any check in gate where these tests run in record mode? I won't mind adding each tests with different scenario if they are running locally only and check in gates run the playback tests.

I agree that there are many tests which can be merged into single test like RewriteRuleCRUD and RewriteRuleCRUDwithConditions. I think it would be better to open a work item on AppGW team to merge them and it should not be the part of this PR.

Apart from that, it would require making changes in the existing test cases which would require loop from the tests author itself to avoid any potential assert missing.

@abjai
Copy link
Contributor Author

abjai commented Sep 25, 2019

@number213 Checks are still failing. Can you point me the reason? Is it related to checkin of local sdk?

@anton-evseev
Copy link
Contributor

@abjai right now there are two issues

  1. Static Analysis is failing because New-AzApplicationGatewayRewriteRuleUrlConfiguration doesn't support ShouldProcess (see docs on how to analyze static analisys errors)
  2. Since local SDK package uses 2019-08-01 and there are no recordings with that API version, all tests are failing. We are almost finished with opening PR to add those recordings. I'll let you know when it's ready so you would rebase/merge

@abjai
Copy link
Contributor Author

abjai commented Sep 26, 2019

This should not be released as the part of October 15 release. We have reverted the swagger changes as well. However we want to continue with the PR so that we can release it as a part of later milestone.

@abjai abjai changed the title Adding Cmdlet New-AzApplicationGatewayRewriteRuleUrlConfiguration to support url rewrite capability for V2 application gateways [Do Not Merge]Adding Cmdlet New-AzApplicationGatewayRewriteRuleUrlConfiguration to support url rewrite capability for V2 application gateways Sep 26, 2019
@abjai abjai removed this from the 2019-10-15 - AZ release milestone Sep 26, 2019
@anton-evseev
Copy link
Contributor

Closing for now. When you're ready please rebase against release branch and re-open

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.

7 participants