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

login: support login when there are no subscriptions #2929

Merged
merged 6 commits into from
Apr 20, 2017

Conversation

yugangw-msft
Copy link
Contributor

This is not a common thing, so users should use a flag of --allow-no-subscriptions to opt-in
Fix #2560

General Guidelines

  • The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

  • Each command and parameter has a meaningful description.
    - [ ] Each new command has a test.

(see Authoring Command Modules)

@codecov-io
Copy link

codecov-io commented Apr 19, 2017

Codecov Report

Merging #2929 into master will increase coverage by 0.21%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2929      +/-   ##
==========================================
+ Coverage   63.11%   63.33%   +0.21%     
==========================================
  Files         466      466              
  Lines       26273    26536     +263     
  Branches     4027     4069      +42     
==========================================
+ Hits        16583    16806     +223     
- Misses       8601     8617      +16     
- Partials     1089     1113      +24
Impacted Files Coverage Δ
...ofile/azure/cli/command_modules/profile/_params.py 66.66% <100%> (+1.96%) ⬆️
...rofile/azure/cli/command_modules/profile/custom.py 32.39% <100%> (ø) ⬆️
src/azure-cli-core/azure/cli/core/_profile.py 85.71% <91.66%> (+1.36%) ⬆️
...onitor/azure/cli/command_modules/monitor/custom.py 67.27% <0%> (ø) ⬆️
...network/azure/cli/command_modules/network/_help.py 100% <0%> (ø) ⬆️
...re-cli-vm/azure/cli/command_modules/vm/commands.py 89.53% <0%> (ø) ⬆️
...work/azure/cli/command_modules/network/commands.py 96.84% <0%> (+0.27%) ⬆️
...zure-cli-vm/azure/cli/command_modules/vm/custom.py 73.13% <0%> (+0.32%) ⬆️
...ure/cli/command_modules/network/_client_factory.py 94.02% <0%> (+0.37%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63ee4ad...2530708. Read the comment docs.

if not allow_no_subscriptions and not subscriptions:
raise CLIError("No subscriptions were found for '{}'. If this is expected, use "
"'--allow-no-subscriptions' to have a tenant level account".format(
username))
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need an explicit flag for this? If there are subscriptions but --allow-no-subscriptions is specified, what happens? If the behavior isn't changed, then why wouldn't we just issue a warning and proceed with the --allow-no-subcriptions logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit subtle.
--allow-no-subscriptions can be used even if you have subscriptions. The story is, login will visit every tenants you are member of. Let us say you have [t1,t2], and there is a subscription s1 in t1, but not any in the t2. W/o this flag, the login will only build an account for t1\s1, which makes sense because almost all commands need to have subscriptions. But for a few commands which doesn't need subscriptions, say those ad ones, users have opened the issue saying they like to run them on t2.
I don't like to build up the tenant level accounts by default, because more likely people don't want it. That is particularly true for RBAC scenarios, that it is common people by mistake forgot to create the role assignment, causing subscription access is not built up. I am open to incoming user voices though, and can switch if needed.
Also, let me know if you have suggestions for a better arg name.

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.

5 participants