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.

Sign up for GitHub

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

Jump to bottom

[Batch] Update Batch API #13321

Closed
wants to merge 11 commits into from
Closed

[Batch] Update Batch API #13321

wants to merge 11 commits into from

Conversation

bgklein
Copy link
Contributor

@bgklein bgklein commented May 4, 2020

Description
Update Batch CLI to support latest management API

Testing Guide
Currently only get/list operations are supported for non-whitelisted accounts for private* accounts

History Notes
[Batch] az batch private-endpoint-connection: Added a list, get, and update command for the new resource PrivateEndpointConnection.
[Batch] az batch private-link-resource: Added a list and get command for the new resource PrivateLinkResource.
[Batch] BREAKING CHANGE: az batch pool create: When creating a poolusing a custom image, the --image property of can now only refer to a Shared Image Gallery image.
[Batch] BREAKING CHANGE: az batch pool create: When creating a pool with --json-file option and specifying a networkConfiguration, the publicIPs property has moved in to a new property publicIPAddressConfiguration. This new property also supports a new ipAddressProvisioningType property which specifies how the pool should allocate IP's and a publicIPs property which allows for configuration of a list of PublicIP resources to use in the case ipAddressProvisioningType is set to UserManaged

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

@bgklein bgklein requested a review from xingwu1 May 4, 2020 19:51
@bgklein bgklein force-pushed the brklein/mgmt-mar branch from fa0a1fa to c6c804a Compare May 4, 2020 22:54
@yonzhan yonzhan added this to the S169 - For Build milestone May 4, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented May 4, 2020

add to S169

@bgklein bgklein force-pushed the brklein/mgmt-mar branch 3 times, most recently from b423cc2 to 1e1c07e Compare May 5, 2020 03:09
@mmyyrroonn
Copy link
Contributor

@bgklein Hello. Currently we're trying to support generic commands for private link scenario. #13225. We would be able to merge this PR within this week. In this PR, there are two parts. One is for generic commands, the other one is about three optional arguments. We can continue use this PR to support those three arguments.

@bgklein
Copy link
Contributor Author

bgklein commented May 7, 2020

@bgklein Hello. Currently we're trying to support generic commands for private link scenario. #13225. We would be able to merge this PR within this week. In this PR, there are two parts. One is for generic commands, the other one is about three optional arguments. We can continue use this PR to support those three arguments.

I am unclear if there is an ask here? Are you requesting that we allow you to add us into your generic support for get/list private link on a resource and get/list/update of private endpoint on a resource, if so I can remove our code. If you are solely pointing out that there is a generic portion as well then good to know.

We added support in the 2020-03-01 API for azure batch https://docs.microsoft.com/en-us/rest/api/batchmanagement/privateendpointconnection/update#privatelinkserviceconnectionstatus

@bgklein bgklein force-pushed the brklein/mgmt-mar branch from 1e1c07e to e78a745 Compare May 12, 2020 21:16
@bgklein bgklein requested a review from haroldrandom as a code owner May 12, 2020 21:16
@bgklein bgklein force-pushed the brklein/mgmt-mar branch from 75bb431 to 25bfbf9 Compare May 12, 2020 21:20
@bgklein
Copy link
Contributor Author

bgklein commented May 12, 2020

Just need to fix style and add tests

@mmyyrroonn
Copy link
Contributor

@bgklein Sorry. I merged another fix PR. You need to change your code based on the latest dev. Meanwhile, what's the ETA for this PR? When will you add the tests for private endpoint connection scenario?

mmyyrroonn
mmyyrroonn reviewed May 13, 2020
View reviewed changes
Copy link
Contributor

@mmyyrroonn mmyyrroonn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please add a test for other three arguments as well.

src/azure-cli/azure/cli/command_modules/batch/_params.py Outdated Show resolved Hide resolved
src/azure-cli/azure/cli/command_modules/batch/_params.py Outdated Show resolved Hide resolved
src/azure-cli/azure/cli/command_modules/batch/custom.py Outdated Show resolved Hide resolved
mmyyrroonn
mmyyrroonn reviewed May 13, 2020
View reviewed changes
src/azure-cli/setup.py Outdated Show resolved Hide resolved
@bgklein
Copy link
Contributor Author

bgklein commented May 13, 2020 •
edited
Loading

@bgklein Sorry. I merged another fix PR. You need to change your code based on the latest dev. Meanwhile, what's the ETA for this PR? When will you add the tests for private endpoint connection scenario?

@myronfanqiu This needs to be released with S169 to meet JEDI deadlines (hence why it was sent out last week). Tests for approve/get operations will be added once the service removes the requirement for whitelisting allowing it to be test on new accounts. Feel free to edit the PR/Merge if you have additional comments.

@bgklein bgklein force-pushed the brklein/mgmt-mar branch from 4e87561 to 274df15 Compare May 13, 2020 17:31
@bgklein
Copy link
Contributor Author

bgklein commented May 13, 2020

@myronfanqiu once CI passes I will merge this due to code complete being today for the build.

@bgklein
Copy link
Contributor Author

bgklein commented May 13, 2020

Please add a test for other three arguments as well.

Get and set operation won't work as this is currently whitelist only for batch and CI accounts won't have permissions

@bgklein bgklein requested a review from mmyyrroonn May 13, 2020 21:39
@mmyyrroonn
Copy link
Contributor

Please add a test for other three arguments as well.

Get and set operation won't work as this is currently whitelist only for batch and CI accounts won't have permissions

CI is based on recording file. You can write tests and run it targeting at dogfood env with whitelist. We cannot merge code into main repo without tests.

@bgklein
Copy link
Contributor Author

bgklein commented May 14, 2020

We can probably wait. Think I found a service bug in the feature anyways but checking with some people

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mmyyrroonn
Copy link
Contributor

@bgklein What's status of this PR?

@bgklein
Copy link
Contributor Author

bgklein commented May 26, 2020

@myronfanqiu Waiting on Azure/azure-sdk-for-python#11604 to merge and then I can release it so CI can pass

@bgklein bgklein force-pushed the brklein/mgmt-mar branch from 3d227db to f851398 Compare May 26, 2020 15:50
@bgklein bgklein force-pushed the brklein/mgmt-mar branch 2 times, most recently from 6d3c216 to 945659f Compare May 26, 2020 22:34
@bgklein bgklein force-pushed the brklein/mgmt-mar branch 2 times, most recently from 436ffcf to d039e32 Compare May 26, 2020 23:59
@bgklein bgklein force-pushed the brklein/mgmt-mar branch from d039e32 to b5e56d8 Compare May 27, 2020 01:13
@bgklein
Copy link
Contributor Author

bgklein commented May 27, 2020 •
edited
Loading

@myronfanqiu should be good to go. Full end to end tests for private endpoint features have been added, and CI looks good. I might have an hour to do minor fix-ups tomorrow morning in case you have any minor comments, but should stay in this release.

@bgklein bgklein requested a review from mmyyrroonn May 27, 2020 15:52
@bgklein bgklein closed this May 27, 2020
@bgklein
Copy link
Contributor Author

bgklein commented May 27, 2020

Sorry for the headache of a PR this was. Aborting and merging with our next API release

@mmyyrroonn
Copy link
Contributor

@bgklein Hi, Why do we close this PR? Will we have another one? Do you still need this feature be merged in this release?

@bgklein
Copy link
Contributor Author

bgklein commented May 28, 2020

@myronfanqiu We no longer need this PR, as we got approval to delay. I will open a separate one next week targeting the 6/17 code complete sprint. The changes will mainly be the same but targeting a different SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xingwu1 xingwu1 xingwu1 approved these changes

@Juliehzl Juliehzl Awaiting requested review from Juliehzl

@jsntcy jsntcy Awaiting requested review from jsntcy jsntcy is a code owner

@qianwens qianwens Awaiting requested review from qianwens

@haroldrandom haroldrandom Awaiting requested review from haroldrandom

@arrownj arrownj Awaiting requested review from arrownj

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

@jiasli jiasli Awaiting requested review from jiasli

@mmyyrroonn mmyyrroonn Awaiting requested review from mmyyrroonn

Assignees

@mmyyrroonn mmyyrroonn

Labels
None yet
Projects
None yet
Milestone
S170
Development

Successfully merging this pull request may close these issues.
5 participants
@bgklein @yonzhan @mmyyrroonn @xingwu1 @dkmiller