Skip to content

Conversation

@cormacpayne
Copy link
Member

@cormacpayne cormacpayne commented May 7, 2018

Description

Fix for #6115

Checklist

Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

A couple of small changes. Also, can we write a unit test for this

string tempName = null;
if (!_profile.TryGetContextName(tempContext, out tempName))
{
WriteWarningMessage(string.Format("Unable to get context name for subscription with id '{0}'.", subscription.Id));
Copy link
Member

Choose a reason for hiding this comment

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

Let's put these strings into the assembly resources


if (!_profile.TrySetContext(tempName, tempContext))
{
WriteWarningMessage(string.Format("Cannot create a context for subscription with id '{0}'.", subscription.Id));
Copy link
Member

Choose a reason for hiding this comment

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

same

}

_profile.DefaultContext.TokenCache = _cache;
if (shouldPopulateContextList)
Copy link
Member

Choose a reason for hiding this comment

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

Were we going to add an option in to the login cmdlet that would disable populating the contexts? Also, I think we should set a default limit to the number of populated contexts (say, 25).

@MiYanni
Copy link
Contributor

MiYanni commented Jun 6, 2018

@cormacpayne You have merge conflicts that need to be resolved.

@cormacpayne cormacpayne self-assigned this Jun 6, 2018
@markcowl
Copy link
Member

markcowl commented Jun 9, 2018

@markcowl markcowl removed their assignment Jun 9, 2018
@maddieclayton maddieclayton merged commit 663aa42 into Azure:preview Jun 9, 2018
@cormacpayne cormacpayne deleted the populate-contexts branch September 12, 2018 16:58
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