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
Bugfix update connect vraserver function #235
Bugfix update connect vraserver function #235
Conversation
Merging in latest changes from Master.
if ($_.Exception.Response.StatusCode -eq "BadRequest") { | ||
# This could be due to an incorrectly set UserAttribute, so customizing the message. | ||
if ($Username -match "@") { | ||
Write-Host -ForegroundColor White -BackgroundColor DarkGray "Unfortunately, you have received a 400 BAD Request Error. Is it possible that your environment has userPrincipalName set as the Username attribute? If so, please make sure to include the parameter UserAttribute and set it to either UPN or userPrincipalName (e.g. -UserAttribute UPN)" |
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 all be part of the throw message, or (my preference) Write-Error with an -ErrorAction Stop. By using Write-Host it's now impossible for an upstream script to capture the error.
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.
Good note, update incoming.
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.
updated here: new commit
if ($Username -match "@") { | ||
Write-Host -ForegroundColor White -BackgroundColor DarkGray "Unfortunately, you have received a 400 BAD Request Error. Is it possible that your environment has userPrincipalName set as the Username attribute? If so, please make sure to include the parameter UserAttribute and set it to either UPN or userPrincipalName (e.g. -UserAttribute UPN)" | ||
} else { | ||
Write-Host -ForegroundColor White -BackgroundColor DarkGray "Unfortunately, you have received a 400 BAD Request Error. It is possible that the Username you provided, $Username, is missing a domain (e.g. $Username@domain.com)?" |
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.
Make this the Throw message (or Write-Error).
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.
good note, update incoming.
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.
updated here: new commit
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.
Looks good to me. Minor typo to correct, then I can approve it.
@@ -12,6 +12,7 @@ | |||
.PARAMETER Username | |||
Username to connect with | |||
For domain accounts ensure to specify the Username in the format username@domain, not Domain\Username | |||
Note: UPN's are valid login Usernames as well and may also be in the format username@domain |
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.
Note: UPN's are valid login Usernames as well and may also be in the format username@domain | |
Note: UPNs are valid login Usernames as well and may also be in the format username@domain |
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.
Just submitted the fix for this typo in latest commit to this pull request.
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.
Approved
This pull request is related here: #233