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

[ACR] Support Managed Identities for Task #9317

Merged
merged 32 commits into from
May 16, 2019
Merged

[ACR] Support Managed Identities for Task #9317

merged 32 commits into from
May 16, 2019

Conversation

jaysterp
Copy link
Member

@jaysterp jaysterp commented May 6, 2019 •
edited
Loading

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

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

djyou
djyou reviewed May 6, 2019
View reviewed changes
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_help.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_params.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_utils.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Outdated Show resolved Hide resolved
northtyphoon
northtyphoon suggested changes May 6, 2019
View reviewed changes
Copy link
Member

@northtyphoon northtyphoon left a comment

Choose a reason for hiding this comment

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

🕐

djyou
djyou approved these changes May 7, 2019
View reviewed changes
Copy link
Member

@djyou djyou left a comment

Choose a reason for hiding this comment

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

LGTM with some minor comments.

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_utils.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Outdated Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Show resolved Hide resolved
src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py Outdated Show resolved Hide resolved
@jaysterp
Copy link
Member Author

jaysterp commented May 7, 2019 •
edited
Loading

@tjprescott Can you review when you have a chance? Thank you.

@sajayantony sajayantony mentioned this pull request May 8, 2019
Closed
@shahzzzam
Copy link
Contributor

LGTM

northtyphoon
northtyphoon approved these changes May 12, 2019
View reviewed changes
Copy link
Member

@northtyphoon northtyphoon left a comment

Choose a reason for hiding this comment

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

:shipit:

@yugangw-msft yugangw-msft mentioned this pull request May 14, 2019
Merged
2 tasks
@jaysterp
Copy link
Member Author

@tjprescott Do these changes look good? //cc @northtyphoon @sajayantony

tjprescott
tjprescott suggested changes May 15, 2019
View reviewed changes
Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

One question/suggestion on --os/--platform but the username and password stuff are 👍

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/build.py Outdated
@@ -47,6 +47,9 @@ def acr_build(cmd, # pylint: disable=too-many-locals
from ._client_factory import cf_acr_registries
client_registries = cf_acr_registries(cmd.cli_ctx)

if os_type and platform:
raise CLIError("[--os] has been depricated. Please use [--platform] instead.")
Copy link
Member

Choose a reason for hiding this comment

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

There is a dedicated mechanism in the CLI for this situation:
https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#deprecating-commands-and-arguments

The previous logic was "both could be used but had to match". The current logic is "--os can't be used". This is a breaking change. A more logical way would be for --os and --platform to reference the same variable, but have --os deprecated in favor of --platform. It would look something like:

with self.argument_context(scope) as c:
  c.argument('platform', options_list=['--platform', c.deprecate(target='--os', redirect='--platform', hide=True)])

In this way, --platform no longer appears, but does work, and fills the same value as using --os would.

Copy link
Member Author

Choose a reason for hiding this comment

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

So as opposed to introducing a separate argument --platform alongside --os, we should remove the --os parameter and add it to the options of --platform with the deprecate wrapper? What do you mean by "--platform no longer appears"? Do you mean --os no longer appears, but does work, and fills the same value as using --platform would?

Copy link
Member

Choose a reason for hiding this comment

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

If you use hide=True then --os won't appear when using -h but it will still work. It is effectively an alias to --platform and the user will get a warning if they use it that it is deprecated and they should use --platform instead.

Copy link
Member

Choose a reason for hiding this comment

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

If your situation is different feel free to not use it, but it seems like it fits your scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand. This is definitely a better option for maintaining the code as well. Will make this change of integrating --os into --platform. //cc @shahzzzam

@tjprescott tjprescott added this to the Sprint 61 milestone May 15, 2019
@shahzzzam
Copy link
Contributor

os/platform change LGTM

@yugangw-msft yugangw-msft merged commit f5811bc into Azure:dev May 16, 2019
leonardbf added a commit to leonardbf/azure-cli that referenced this pull request Jun 4, 2019
@leonardbf
Get latest from azure dev (#24)
4f08e84 
* ppg create: `--type` is now optional. (Azure#9391)

* [functionapps] Improvements of devops-pipeline command (Azure#9383)

*  role assignment: support scope of management group (Azure#9321)

* [function-app]: wording change from iteractive to interactive (Azure#9402)

* [Network] NAT Gateway support (Azure#9379)

* Azure CLI Nat Gateway contract changes (Azure#9071)

* Azure CLI Nat Gateway contract changes

* Update help text to nat gateway sku

* Update setup files

* Make azure-cli-natgateway installable.

* Re-record tests for 2018-02-01.

* Re-record tests.

* Re-record tests. Remove accidentally committed files.

* Nat Gateway Resource (Azure#9147)

* Azure CLI Nat Gateway contract changes

* Update help text to nat gateway sku

* Update setup files

* Add Get,Create,Remove commands for Nat Gateway and Test for Create,Remove NatGateway

* Update PR comments and tests

* Update Recordings

* Update recordings.

* Nat Gateway Resource (Azure#9330)

* Azure CLI Nat Gateway contract changes

* Update help text to nat gateway sku

* Update setup files

* Add Get,Create,Remove commands for Nat Gateway and Test for Create,Remove NatGateway

* Update PR comments and tests

* Update Recordings

* Update linter, style changes, convert update from regular to properties

* Update help examples and natgateway properties update

* Add ID support

* Update Recordings and merge branch to local

* Update azure cli pyproj

* Attach subnet to Nat Gateway (Azure#9333)

* Azure CLI Nat Gateway contract changes

* Update help text to nat gateway sku

* Update setup files

* Add Get,Create,Remove commands for Nat Gateway and Test for Create,Remove NatGateway

* Update PR comments and tests

* Update Recordings

* Update linter, style changes, convert update from regular to properties

* Update help examples and natgateway properties update

* Add ID support

* Update Recordings and merge branch to local

* Update azure cli pyproj

* Subnet attached to managed nat

* Update PR comments

* Nat Gateway (Azure#9373)

* Azure CLI Nat Gateway contract changes

* Update help text to nat gateway sku

* Update setup files

* Add Get,Create,Remove commands for Nat Gateway and Test for Create,Remove NatGateway

* Update PR comments and tests

* Update Recordings

* Update linter, style changes, convert update from regular to properties

* Update help examples and natgateway properties update

* Add ID support

* Update Recordings and merge branch to local

* Update azure cli pyproj

* Subnet attached to managed nat

* Update PR comments

* Updates to NAT gateway module.

* Update tests and validator'

* Update Params

* Rerecord tests.

* Re-record tests.

* Convert AKS tests to live only.

* Convert SQL test to live only.

* Final fixes.

* Disable flaky security test.

* Convert `network nat-gateway` group to `network nat gateway`.

* Code review feedback.

* [ACR] Support Managed Identities for Task (Azure#9317)

* [webapp]Fixing bug where the result was not showing the ASP (Azure#9408)

* BotService 0.2.1: Update help text, minor UX improvements (Azure#9412)

* Add support for Manual-Failover (Azure#9413)

* add new command

* add support for manual-failover

* update help content

* re-record test session

* Bump SDK version

* [core] Fix bug where --query cannot be used with --output yaml (Azure#9403)

* [core] Fix bug where --query cannot be used with --output yaml

* Add comment explaining yaml formatter error handling logic.

* update default log analytics workspace region mapping for aks monitoring addon (Azure#9434)

* add cluster spn with metric publisher role to aks cluster resource

* refactor the code

* fix default workspace issue in china cloud

* add metrics publisher role assignment only in public cloud

* fix pr feedback

* fix build error

* fix lint error

* update region mapping

* order regions in sortorder

* fix trailing whitespaces

* Fix JP installer line (Azure#9438)

Curl must follow redirects. Fixes Azure#9435

* ACS - OSA - Whitelist message (Azure#9409)

* add friendly msg when sub is not whitelisted for GA API

* bump version and add history

* release: update versions (Azure#9418)

* [vm] Fix bug introduced in Azure#9306.  (Azure#9421)

* auth: handle unicode in subscription name when under python 2.7 (Azure#9439)

* role: update the help of --assignee-object-id (Azure#9457)

* role: update the help of --assignee-object-id

* fix grammar

* Update error logging for reported unclear issues (Azure#9180)

* Update error logging for reported unclear issues

* Combine error messages

* fix ci

* Enable/Disable AKS kube-dashboard addon (Azure#9458)

* Disable/Enable kube-dashboard addon

* Minor fixes

* change HISTORY.rst

* Added ACR command to create a buildpack task (Azure#9366)

* Added acr pack command scaffold

* Added acr pack test

* Fixed command registration

* Consolidated value

* Added command module scaffold

* Extracted method

* Added linux assertion

* Fixed params

* Moved validation to be earlier

No need to upload sources if the build cannot proceed

* Removed --file, added --image

* Improved docstring

* Added builder flag

* Fixed flake9 errors in run.py

* Fixed flake8 erros in pack.py

* Added registry name env var to build

* Removed test

* Unused

* CR remark

* Merge remote-tracking branch 'refs/remotes/Azure/dev' into dev

# Conflicts:
#	src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/run.py

* Added task version

* Removed unnecessary doc

* Removed unused imports

* Added no-pull arg

* Flipped arg

* Added automatic registry prefix

* CR

* Make the RPM Spec flexible to EL7 vs EL8 (Azure#9394)

* Add python3 macro to RPM spec

* fixing missed raw python use

* fixing another missed raw python use

* Don't look for el7, look for el*

* Try removing duplicate dependencies in Runtime and buildtime requirements

* Declare dependency on virtualenv instead of downloading it

* Disable python-devel requirement

* WIP

* More fixes

* Exclude hardlinks created by virtualenv

In reality, all we need from virtualenv is an easy way to build up an alternate site-packages directory. Infact, what this is doing just emulates how a virtualenv works, but without the hardlinks that were confusing rpmbuild!

* Ignore user site-packages

* [ACR] Return patch response directly for acr repository update (Azure#9420)

* Return patch response directly for acr repository update

* fix ci

* Remove deprecated classic registry error handling

* Fixing Dockerfile name of RPM builder for centos in pipeline definition (Azure#9493)

* test: fix a flaky test (Azure#9494)

* Update _help.py (Azure#9502)

Corrected a small typo.

* Added Error Message when no TTY is available (Azure#9481)

* Added Error Message when no TTY is available

Fix Azure#3876

When performing a resource group deployment, if no TTY is available,
a CLIError will be raised detailing the parameters that were not
provided.

* Fixes for python2 compatibility

* [Network] AppGateway WAF Policy commands (Azure#9470)

* initial work.

* WAF Policy progress.

* Progress.

* WAF policy commands and test.

* Fix and re-record test.

* Closes Azure#9255.

* Fix test issue.

* Code review feedback.

* Fix update logic.

* Fix CI

* [vm] create now more intelligently uses image data disk lun info. (Azure#9504)

* vm create now more intelligently uses image data disk lun information.

* Re-record and update a test. Other minor updates.

* Update tests. Minor bug fixes.

* Upate hybrid profile tests

* Fixed registry prefix check (Azure#9509)

* Added acr pack command scaffold

* Added acr pack test

* Fixed command registration

* Consolidated value

* Added command module scaffold

* Extracted method

* Added linux assertion

* Fixed params

* Moved validation to be earlier

No need to upload sources if the build cannot proceed

* Removed --file, added --image

* Improved docstring

* Added builder flag

* Fixed flake9 errors in run.py

* Fixed flake8 erros in pack.py

* Added registry name env var to build

* Removed test

* Unused

* CR remark

* Merge remote-tracking branch 'refs/remotes/Azure/dev' into dev

# Conflicts:
#	src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/run.py

* Added task version

* Removed unnecessary doc

* Removed unused imports

* Added no-pull arg

* Flipped arg

* Added automatic registry prefix

* CR

* Fixed registry prefix

* Updated history.rst

* Update CLI to use Knack 0.6.2 (preview mechanism) (Azure#9456)

* Initial work.

* Update preview references.

* Additional work to wire up preview arguments.

* Update for knack changes.

* Update dependencies. Remove command suggestion code. Deprecate BatchAI. Hide ACS.

* Restore CLI-specific spellchecker.

* Update history entries.

* Network API 2019-04-01 (Azure#9523)

* Re-record tests for 2019-04-01.

* Closes Azure#9463.

* Closes Azure#9401. Restore private endpoint tests and commands.

* support ssh_public_key (Azure#9512)

* Fix Azure#9489. (Azure#9531)

* Fix Azure#9443. (Azure#9533)

* ServiceBus: Fix for Issue 9319 (Azure#9538)

* Fix for Issue - 9319

* updated version in histroy and setup files

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

@shahzzzam shahzzzam shahzzzam left review comments

@northtyphoon northtyphoon northtyphoon approved these changes

@djyou djyou djyou approved these changes

@tjprescott tjprescott tjprescott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint 61
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jaysterp @shahzzzam @northtyphoon @djyou @tjprescott @yugangw-msft