-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Integrate Availability Zone from private repository #4523
Conversation
* wire up the initial zone support work * add output * add tests * disable package verifications
…zure#16) * Support for zoned public IP. Make global zone_type and zones_type. * Code review feedback.
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.
Mainly questions.
.travis.yml
Outdated
- stage: publish | ||
script: ./scripts/ci/publish.sh | ||
python: 3.6 | ||
if: branch = master and type = push |
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.
I think this is still needed. @troydai?
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.
@derekbekoe, I will undo this when SDK gets published. The roll back list is being tracked at
https://github.com/Azure/azure-cli-pr/issues/13
@@ -28,7 +28,7 @@ class ResourceType(Enum): # pylint: disable=too-few-public-methods | |||
|
|||
MGMT_STORAGE = ('azure.mgmt.storage', | |||
'StorageManagementClient') | |||
MGMT_COMPUTE = ('azure.mgmt.compute.compute', | |||
MGMT_COMPUTE = ('azure.mgmt.compute', |
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.
With the new SDK, is azure.mgmt.compute
still correct?
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.
yes
@@ -29,6 +29,7 @@ unreleased | |||
|
|||
2.0.12 (2017-08-11) | |||
+++++++++++++++++++ | |||
* `public-ip`: Add availability zone support. |
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.
This should be in unreleased
I believe.
@@ -908,6 +908,12 @@ | |||
- name: Create a load balancer on a specific virtual network and subnet. | |||
text: > | |||
az network lb create -g MyResourceGroup -n MyLb --vnet-name MyVnet --subnet MySubnet | |||
- name: create a zone flavored public facing load balancer through provisiong a zonal public ip |
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.
Typo: provisiong
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.
Zone flavored? What does that taste like? 🍨
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.
We don't want to say zonal lb
which is conceptual wrong, but LB does have some relationships with zone. The flavor
is the word picked when I chatted with NRP folks.
test_clouds/brazilus.json
Outdated
"EndpointActiveDirectory": "https://login.windows.net/", | ||
"EndpointActiveDirectoryGraphWindowsId": "https://graph.windows.net/", | ||
"EndpointVmImageAliasDoc": "https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/arm-compute/quickstart-templates/aliases.json" | ||
} |
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.
Still keep this in public? @tjprescott?
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.
No no. This should only be in the private repo.
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.
Please remove the two testing cloud configs.
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.
This PR does more than "Integrate Availability Zone from private repository". It also introduces packaged_releases/windows/scripts/build_local.cmd
which is very similar to packaged_releases/windows/scripts/prepareBuild.cmd
(introduced by #2655). packaged_releases/windows/scripts/build_local.cmd
should have been moved to a separate PR.
Later in #4696, packaged_releases/windows/scripts/build_local.cmd
was renamed to build_scripts/windows/scripts/build.cmd
and packaged_releases/windows/scripts/prepareBuild.cmd
was deleted.
Mark as do not merge as I need 1 more change to use official compute and network sdks
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)