-
Notifications
You must be signed in to change notification settings - Fork 349
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
updating application gateway #46
base: master
Are you sure you want to change the base?
Conversation
.travis.yml
Outdated
@@ -52,7 +52,7 @@ before_install: | |||
|
|||
diffstr=$(git diff $branch remotes/origin/master --name-only -- '*.yml' --no-pager); | |||
changedfiles=($diffstr); | |||
excludedList="vm_create_existingvnet_deployjavaapp.yml .travis.yml aks_create_scale.yml webapp.yml rest/sql-managed-instance.yml vm_create_image.yml"; | |||
excludedList="vm_create_existingvnet_deployjavaapp.yml .travis.yml aks_create_scale.yml webapp.yml rest/sql-managed-instance.yml vm_create_image.yml appgateway_create.yml"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why appgate is excluded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because preview moduled don't have right version yet....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preview_modules should keep sync with upstream, pls fix preview_module firstly
@@ -0,0 +1 @@ | |||
MIIMAjCCCeqgAwIBAgITLQAAMpnXBx230XCKQgAAAAAymTANBgkqhkiG9w0BAQsFADCBizELMAkGA1UEBhMCVVMxEzARBgNVBAgTCldhc2hpbmd0b24xEDAOBgNVBAcTB1JlZG1vbmQxHjAcBgNVBAoTFU1pY3Jvc29mdCBDb3Jwb3JhdGlvbjEVMBMGA1UECxMMTWljcm9zb2Z0IElUMR4wHAYDVQQDExVNaWNyb3NvZnQgSVQgVExTIENBIDUwHhcNMTcwNzIwMTc0NzA4WhcNMTkwNzEwMTc0NzA4WjAXMRUwEwYDVQQDEwx3d3cuYmluZy5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC6jsg+/7DlIrdgFOcaDlK3RQ9sIgkJsgpj+ZxAbIe3ziyimIxjVlHX87pqgXcNhaYNbCFD0iPm+aUfbv4GDTLR+AIr8eSegqxZ+CBToYM67NhpVYra1KAvY4XgqxorO4FB9IWYJRqhI3SZeZ3lLK5t9XuUMicG8l52nJfpPdXXvBca2wUCq8FHEObG81vJzESA0htLLPTjdUWBQnXPiW5bqzlGHzzv8ISV6jtDLNNa5JRlhSlXho+6pCedhNF7MP4yTaantPvAELLRWX13VhjgoCcRCCu0s8rxW5DuVWl2Pb2iw35MFnNWlcoVwq0AjAfGA+xEba/WLid6qfkQctYjAgMBAAGjggfQMIIHzDAdBgNVHQ4EFgQUCYflhSl4MCAls91+3GztpSmoA3AwCwYDVR0PBAQDAgSwMB8GA1UdIwQYMBaAFAj+JZ906ocEwry7jqg4XzPG0WxlMIGsBgNVHR8EgaQwgaEwgZ6ggZuggZiGS2h0dHA6Ly9tc2NybC5taWNyb3NvZnQuY29tL3BraS9tc2NvcnAvY3JsL01pY3Jvc29mdCUyMElUJTIwVExTJTIwQ0ElMjA1LmNybIZJaHR0cDovL2NybC5taWNyb3NvZnQuY29tL3BraS9tc2NvcnAvY3JsL01pY3Jvc29mdCUyMElUJTIwVExTJTIwQ0ElMjA1LmNybDCBhQYIKwYBBQUHAQEEeTB3MFEGCCsGAQUFBzAChkVodHRwOi8vd3d3Lm1pY3Jvc29mdC5jb20vcGtpL21zY29ycC9NaWNyb3NvZnQlMjBJVCUyMFRMUyUyMENBJTIwNS5jcnQwIgYIKwYBBQUHMAGGFmh0dHA6Ly9vY3NwLm1zb2NzcC5jb20wPgYJKwYBBAGCNxUHBDEwLwYnKwYBBAGCNxUIh9qGdYPu2QGCyYUbgbWeYYX062CBXYTS30KC55N6AgFkAgEQMB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEFBQcDATBNBgNVHSAERjBEMEIGCSsGAQQBgjcqATA1MDMGCCsGAQUFBwIBFidodHRwOi8vd3d3Lm1pY3Jvc29mdC5jb20vcGtpL21zY29ycC9jcHMwJwYJKwYBBAGCNxUKBBowGDAKBggrBgEFBQcDAjAKBggrBgEFBQcDATCCBW0GA1UdEQSCBWQwggVgggx3d3cuYmluZy5jb22CEGRpY3QuYmluZy5jb20uY26CEyoucGxhdGZvcm0uYmluZy5jb22CCiouYmluZy5jb22CCGJpbmcuY29tghZpZW9ubGluZS5taWNyb3NvZnQuY29tghMqLndpbmRvd3NzZWFyY2guY29tghljbi5pZW9ubGluZS5taWNyb3NvZnQuY29tghEqLm9yaWdpbi5iaW5nLmNvbYINKi5tbS5iaW5nLm5ldIIOKi5hcGkuYmluZy5jb22CGGVjbi5kZXYudmlydHVhbGVhcnRoLm5ldIINKi5jbi5iaW5nLm5ldIINKi5jbi5iaW5nLmNvbYIQc3NsLWFwaS5iaW5nLmNvbYIQc3NsLWFwaS5iaW5nLm5ldIIOKi5hcGkuYmluZy5uZXSCDiouYmluZ2FwaXMuY29tgg9iaW5nc2FuZGJveC5jb22CFmZlZWRiYWNrLm1pY3Jvc29mdC5jb22CG2luc2VydG1lZGlhLmJpbmcub2ZmaWNlLm5ldIIOci5iYXQuYmluZy5jb22CECouci5iYXQuYmluZy5jb22CEiouZGljdC5iaW5nLmNvbS5jboIPKi5kaWN0LmJpbmcuY29tgg4qLnNzbC5iaW5nLmNvbYIQKi5hcHBleC5iaW5nLmNvbYIWKi5wbGF0Zm9ybS5jbi5iaW5nLmNvbYINd3AubS5iaW5nLmNvbYIMKi5tLmJpbmcuY29tgg9nbG9iYWwuYmluZy5jb22CEXdpbmRvd3NzZWFyY2guY29tgg5zZWFyY2gubXNuLmNvbYIRKi5iaW5nc2FuZGJveC5jb22CGSouYXBpLnRpbGVzLmRpdHUubGl2ZS5jb22CDyouZGl0dS5saXZlLmNvbYIYKi50MC50aWxlcy5kaXR1LmxpdmUuY29tghgqLnQxLnRpbGVzLmRpdHUubGl2ZS5jb22CGCoudDIudGlsZXMuZGl0dS5saXZlLmNvbYIYKi50My50aWxlcy5kaXR1LmxpdmUuY29tghUqLnRpbGVzLmRpdHUubGl2ZS5jb22CCzNkLmxpdmUuY29tghNhcGkuc2VhcmNoLmxpdmUuY29tghRiZXRhLnNlYXJjaC5saXZlLmNvbYIVY253ZWIuc2VhcmNoLmxpdmUuY29tggxkZXYubGl2ZS5jb22CDWRpdHUubGl2ZS5jb22CEWZhcmVjYXN0LmxpdmUuY29tgg5pbWFnZS5saXZlLmNvbYIPaW1hZ2VzLmxpdmUuY29tghFsb2NhbC5saXZlLmNvbS5hdYIUbG9jYWxzZWFyY2gubGl2ZS5jb22CFGxzNGQuc2VhcmNoLmxpdmUuY29tgg1tYWlsLmxpdmUuY29tghFtYXBpbmRpYS5saXZlLmNvbYIObG9jYWwubGl2ZS5jb22CDW1hcHMubGl2ZS5jb22CEG1hcHMubGl2ZS5jb20uYXWCD21pbmRpYS5saXZlLmNvbYINbmV3cy5saXZlLmNvbYIcb3JpZ2luLmNud2ViLnNlYXJjaC5saXZlLmNvbYIWcHJldmlldy5sb2NhbC5saXZlLmNvbYIPc2VhcmNoLmxpdmUuY29tghJ0ZXN0Lm1hcHMubGl2ZS5jb22CDnZpZGVvLmxpdmUuY29tgg92aWRlb3MubGl2ZS5jb22CFXZpcnR1YWxlYXJ0aC5saXZlLmNvbYIMd2FwLmxpdmUuY29tghJ3ZWJtYXN0ZXIubGl2ZS5jb22CE3dlYm1hc3RlcnMubGl2ZS5jb22CFXd3dy5sb2NhbC5saXZlLmNvbS5hdYIUd3d3Lm1hcHMubGl2ZS5jb20uYXUwDQYJKoZIhvcNAQELBQADggIBADTpW/UWeupk40OP6k4yxihKStswxwqPAfMRmx4XyqmTAawAKRNM+6EZth1BQdPdOplwRTvs69kkmUHJH+ZjYXBezEACWkzEiNUQnzkRWajdSQIz08Ubj/mBD6U8xLYD+NXgiB0xNWabd8aiPsqPaj6I3qkNw4JvtgtHZQG1zlwC5/Lu6yV3DM3sKpQMyBmOnX6nVUiS0MTOzLgZOQzRk07nO7EXWGcKTmDBjE8cqv5IA/jQ6gtaxCI5pDxfXK4ct7oQyoChfxOXcEDKMmMndFmg9ch5c4an/FRM2cgzDfjR01A71LNUpLUdOjNV0T+ZEStqEpdyDFfjrHGDtzLyqEz3iyvvQFyjmlGh6OtZXwjCPpnVSrKCmfJKio0kUxyq+6t5tZAQbPVgFKiMrVnU+sgvmNVip1toijyz8vMVCkwJ2G++7xjJukoELMxZ50W4/SAMZLy1Asx02NBwYCu9+CTQPVnmPe7rmxhlQRBOfDNa1+5jwRHY64YudEzKhWR1uqS3ABd/fk+TL86yuNYGAgxnOm1FtOGieRgViV3+NzC+bDbuUOtmbD/GvDGmRwJRcCTHL7jBmkHePh2ABY93NE/IbkaDP6l1Kw98AfqkzSUxhqHXuThe7KIoX9/0zv4AA1WZFis1QvAG7dpl9eio6vCdC/73HvBAlqRL+7Mb1uu0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any other decent way for cert access..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would you suggest? should we include keyvault in the sample or sth like that?
I think it's ok like this as this is just a sample.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will update preview modules first :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could have better names for the certs, like sample-xx-cert?
appgateway_create.yml
Outdated
subnet_name: ansiblesubnetname | ||
appgw_name: appgw{{ rpfx }} | ||
azure_subscription_id: "{{ lookup('env','AZURE_SUBSCRIPTION_ID') }}" | ||
resource_group: zimsappgwnew |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use personal name as for the name of resource_group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, and we have lots of that even in the oldest modules. we should have some naming conventions so documentation looks consinstent, and not "MyResourceGroup" / "MyRG" /"someones_rg"
debug: | ||
var: pip_output | ||
|
||
- name: Create instance of Application Gateway | ||
azure_rm_appgw: | ||
azure_rm_appgateway: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 2.7, the name is azure_rm_appgateway. But in roles, the nae is azure_rm_appgw. We should align them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
azure_rm_appgateway is correct, name was changed while reviewing / merging process to ansible/ansible. now propagating back to preview modules
location: eastus | ||
vnet_name: ansiblevnetname | ||
subnet_name: ansiblesubnetname | ||
appgw_name: zimsappgw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls use name like 'myAppGW'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per comments
@zikalino, reviewers request change, Thanks! |
@zikalino need your update |
@zikalino Could you help recheck the PR and update according by yungezz's comment? Thanks! |
kindly ping |
kindly ping |
@zikalino Could you help recheck this and push for merge ? Thanks! |
1 similar comment
kindly ping |
@zikalino Please change this PR according by above comments? Thanks! |
@zikalino Please update the PR according by the above comment? Push this to review! Thank you very much! |
@zikalino need you changed! Thanks a lot! |
@zikalino When you are free, please help to solve the PR mistake. Thank you very much! |
@zikalino Need you to change! Please update when you're free! Thank you very much! |
@zikalino This PR has been open for a long time, please follow the comments to change and promote the review. thank you very much! |
@zikalino How will this PR be handled? hasn't been updated in a long time. Please check, thank you very much! |
@zikalino Please help finish PR changed and push for review when you're free! Thanks a lot! |
@zikalino Please help update this PR when you're free! Thank you very much! |
kindly ping |
@zikalino Can you help complete the change of PR and promote the merger? It's been around for a long time--Not much to change!. Thank you very much! |
@haiyuazhang @gavinfish Can you help maitain this PR? Thank you very much! |
@zikalino Thank you for taking the time to contribute to this PR. We will transfer ansible 's azure module related Issue and PR to azure collection (https://github.com/ansible-collections/azure/pulls), can you transfer the Issue to azure collection repo? |
Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test
$ ansible-galaxy install Azure.azure_preview_modules $ pip install -r ~/.ansible/roles/Azure.azure_preview_modules/files/requirements-azure.txt
What to Check
Verify that the playbook is successfully run.
Other Information