-
Notifications
You must be signed in to change notification settings - Fork 4k
Enable context autosave by default #6003
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
Changes from all commits
0bdc3f0
c26f24c
e429c1d
f3e4bbc
27949d5
65eb980
9ddec26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,7 @@ public AzureRmProfile Login( | |
string subscriptionId, | ||
string subscriptionName, | ||
SecureString password, | ||
bool skipValidation, | ||
bool skipValidation, | ||
Action<string> promptAction, | ||
string name = null) | ||
{ | ||
|
@@ -140,7 +140,6 @@ public AzureRmProfile Login( | |
Id = tenantId | ||
}; | ||
} | ||
|
||
else | ||
{ | ||
// (tenant and subscription are present) OR | ||
|
@@ -215,7 +214,7 @@ public AzureRmProfile Login( | |
newTenant == null && | ||
TryGetTenantSubscription(token, account, environment, tenant, subscriptionId, subscriptionName, out tempSubscription, out tempTenant)) | ||
{ | ||
// If no subscription found for the given token/tenant | ||
// If no subscription found for the given token/tenant | ||
// discard tempTenant value unless current token/tenant is the last one. | ||
if (tempSubscription != null || tenant.Equals(tenants[tenants.Count - 1])) | ||
{ | ||
|
@@ -227,6 +226,7 @@ public AzureRmProfile Login( | |
} | ||
} | ||
|
||
bool shouldPopulateContextList = _profile.DefaultContext?.Account == null; | ||
if (newSubscription == null) | ||
{ | ||
if (subscriptionId != null) | ||
|
@@ -261,6 +261,29 @@ public AzureRmProfile Login( | |
} | ||
|
||
_profile.DefaultContext.TokenCache = _cache; | ||
var defaultContext = _profile.DefaultContext; | ||
if (shouldPopulateContextList) | ||
{ | ||
var subscriptions = ListSubscriptions(tenantId); | ||
foreach (var subscription in subscriptions) | ||
{ | ||
var tempContext = new AzureContext(subscription, account, environment, newTenant); | ||
string tempName = null; | ||
if (!_profile.TryGetContextName(tempContext, out tempName)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this using the name of the subcription? I forgot the logic for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @markcowl updated the logic for this after discussion so that the name is now |
||
{ | ||
WriteWarningMessage(string.Format("Unable to get context name for subscription with id '{0}'.", subscription.Id)); | ||
continue; | ||
} | ||
|
||
if (!_profile.TrySetContext(tempName, tempContext)) | ||
{ | ||
WriteWarningMessage(string.Format("Cannot create a context for subscription with id '{0}'.", subscription.Id)); | ||
} | ||
} | ||
|
||
_profile.TrySetDefaultContext(defaultContext); | ||
_profile.TryRemoveContext("Default"); | ||
} | ||
|
||
return _profile.ToProfile(); | ||
} | ||
|
@@ -405,10 +428,10 @@ public IAzureEnvironment RemoveEnvironment(string name) | |
public IAccessToken AcquireAccessToken(string tenantId, Action<string> promptAction = null) | ||
{ | ||
return AcquireAccessToken( | ||
_profile.DefaultContext.Account, | ||
_profile.DefaultContext.Environment, | ||
_profile.DefaultContext.Account, | ||
_profile.DefaultContext.Environment, | ||
tenantId, null, | ||
ShowDialog.Auto, | ||
ShowDialog.Auto, | ||
promptAction); | ||
} | ||
|
||
|
@@ -589,9 +612,9 @@ private string GetCommonTenant(IAzureAccount account) | |
return result; | ||
} | ||
private List<AzureTenant> ListAccountTenants( | ||
IAzureAccount account, | ||
IAzureEnvironment environment, | ||
SecureString password, | ||
IAzureAccount account, | ||
IAzureEnvironment environment, | ||
SecureString password, | ||
string promptBehavior, | ||
Action<string> promptAction) | ||
{ | ||
|
@@ -600,10 +623,10 @@ private List<AzureTenant> ListAccountTenants( | |
try | ||
{ | ||
var commonTenantToken = AcquireAccessToken( | ||
account, | ||
environment, | ||
account, | ||
environment, | ||
commonTenant, | ||
password, | ||
password, | ||
promptBehavior, | ||
promptAction); | ||
|
||
|
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 in case, we should also ensure that _profile?.Contexts?.Count < 2 (actually, this will need to be two null checks, but you get the idea).
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.
@markcowl in this class,
_profile
is of the abstracted typeIProfileOperations
, which doesn't contain theContexts
property (the implementationAzureRmProfile
does contain this property, but I doubt we want to use that in this class). Is there a separate check we can do here?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.
makes sense. As long as we are not overwriting existing contexts, I think we are ok here