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

vm/vmss: remove the length requirement of admin-username #2400

Closed
wants to merge 1 commit into from

Conversation

yugangw-msft
Copy link
Contributor

Reported by someone whose login name is 5 characters long, and could not create a VM.
Turned out shorter names are accepted by both portal and vm itself. And more important we do not have justification for why chose 6 characters long; hence it appears a bit random

@yugangw-msft yugangw-msft changed the title vm/vmss: remove the length limitation of admin-username vm/vmss: remove the length requirement of admin-username Mar 7, 2017
@codecov-io
Copy link

Codecov Report

Merging #2400 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master    #2400   +/-   ##
=======================================
  Coverage   72.33%   72.33%           
=======================================
  Files         323      323           
  Lines       18273    18273           
  Branches     2701     2701           
=======================================
  Hits        13217    13217           
  Misses       4223     4223           
  Partials      833      833
Impacted Files Coverage Δ
...cli-vm/azure/cli/command_modules/vm/_validators.py 73.43% <0%> (ø)

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 4c0fdcf...2953757. Read the comment docs.

@tjprescott
Copy link
Member

It isn't random--it depends on the underlying OS of the image.

@yugangw-msft
Copy link
Contributor Author

It isn't random--it depends on the underlying OS of the image.

@tjprescott, could you elaborate?I also tried windows os in portal and I am able to use shorter username.
Either way, we need more details

@tjprescott
Copy link
Member

tjprescott commented Mar 7, 2017

#1943 is the issue that originally reported the issue.
This is a related issue: #582

If we remove this, we would essentially be reopening #1943. There's no perfect solution--either we choose a reasonable requirement that works across images (even if it is more conservative) or we just allow people to try and fail if the underlying image doesn't allow it.

@yugangw-msft
Copy link
Contributor Author

@tjprescott, I prefer we just allow people to try and fail if the underlying image doesn't allow it., but let me ask portal team for their validation logic so to get a bit more data. What we have now is a bit confusing, I saw people complaining about it in #1943 as well.

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.

We should discuss at scrum and decide the way forward for this PR.

@troydai
Copy link
Contributor

troydai commented Mar 15, 2017

@tjprescott @yugangw-msft did we reach an agreement on this? It occurs to me what is blocking this PR is about the design rather than the code. If we need more time to go through the design decision, I suggest closing this pull request for now.

@tjprescott
Copy link
Member

We haven't brought it up in scrum yet (unless it was today's).

@troydai
Copy link
Contributor

troydai commented Mar 15, 2017

@yugangw-msft is digging deeper into this topic. I'll close the pull request for now since there is nothing to review.

@troydai troydai closed this Mar 15, 2017
@yugangw-msft
Copy link
Contributor Author

Okay, Portal team is providing me the user name validation code. Once I get the complete info, I shall update the PR

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.

None yet

7 participants