Skip to content

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

Merged
merged 7 commits into from
Apr 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,17 @@ static ContextAutosaveSettings InitializeSessionSettings(IDataStore store, strin
result.Mode = settings.Mode;
result.ContextFile = settings.ContextFile ?? result.ContextFile;
}
else
{
string directoryPath = Path.GetDirectoryName(profileDirectory);
if (!store.DirectoryExists(directoryPath))
{
store.CreateDirectory(directoryPath);
}
string autoSavePath = Path.Combine(profileDirectory, settingsFile);
store.WriteFile(autoSavePath, JsonConvert.SerializeObject(result));
result.Mode = ContextSaveMode.CurrentUser;
}
}
catch
{
Expand Down Expand Up @@ -191,7 +202,7 @@ public override System.Diagnostics.TraceLevel AuthenticationLegacyTraceLevel
}
set
{

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class AzureRmProfile : IAzureContextContainer, IProfileOperations
public IDictionary<string, IAzureContext> Contexts { get; set; } = new ConcurrentDictionary<string, IAzureContext>(StringComparer.CurrentCultureIgnoreCase);

/// <summary>
/// Gets the path of the profile file.
/// Gets the path of the profile file.
/// </summary>
[JsonIgnore]
[XmlIgnore]
Expand Down Expand Up @@ -271,7 +271,7 @@ public void Save(IFileProvider provider, bool serializeCache = true)
writer.Write(contents);
writer.Flush();

// When writing to a stream, ensure that the file is truncated
// When writing to a stream, ensure that the file is truncated
// so that previous data is overwritten
provider.Stream.SetLength(provider.Stream.Position);
}
Expand Down Expand Up @@ -386,20 +386,9 @@ public bool TryGetContextName(IAzureContext context, out string name)
if (context != null)
{

if ((context.Account != null && !string.IsNullOrWhiteSpace(context.Account.Id)) || context.Subscription != null)
if (context.Subscription != null)
{
List<string> components = new List<string>();
if (context.Account != null && !string.IsNullOrWhiteSpace(context.Account.Id))
{
components.Add(context.Account.Id);
}

if (context.Subscription != null)
{
components.Add(context.Subscription.GetId().ToString());
}

name = string.Format("[{0}]", string.Join(", ", components));
name = string.Format("{0} - {1}", context.Subscription.Name, context.Subscription.Id);
result = true;
}
else
Expand Down
2 changes: 2 additions & 0 deletions src/ResourceManager/Profile/ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
-->
## Current Release
* Set minimum dependency of module to PowerShell 5.0
* Enable context autosave by default
* Create a context for each subscription when running `Connect-AzureRmAccount` with no previous context
* Add USGovernmentOperationalInsightsEndpoint and USGovernmentOperationalInsightsEndpointResourceId properties to Azure environment for US Gov.

## Version 4.6.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,7 @@ public void GetAzureContextNoLogin()
}

// Verify
Assert.True(commandRuntimeMock.OutputPipeline.Count == 1);
var context = (PSAzureContext)commandRuntimeMock.OutputPipeline[0];
Assert.True(context == null || context.Account == null || context.Account.Id == null);
Assert.True(commandRuntimeMock.ErrorStream.Count == 1);
var error = commandRuntimeMock.ErrorStream[0];
Assert.Equal("Run Connect-AzureRmAccount to login.", error.Exception.Message);
Assert.True(commandRuntimeMock.OutputPipeline.Count == 0);
}

[Fact]
Expand Down Expand Up @@ -131,9 +126,7 @@ public void ListAzureContextNoLogin()

// Verify
Assert.Equal(0, commandRuntimeMock.OutputPipeline.Count);
Assert.True(commandRuntimeMock.ErrorStream.Count == 1);
var error = commandRuntimeMock.ErrorStream[0];
Assert.Equal("Run Connect-AzureRmAccount to login.", error.Exception.Message);
Assert.True(commandRuntimeMock.ErrorStream.Count == 0);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public void DataCollectionSettingPreventsFileWrite()
Assert.Equal(typeof(AuthenticationStoreTokenCache), session.TokenCache.GetType());
Assert.NotNull(AzureRmProfileProvider.Instance);
Assert.Equal(typeof(ResourceManagerProfileProvider), AzureRmProfileProvider.Instance.GetType());
store.Verify(f => f.WriteFile(It.IsAny<string>(), It.IsAny<string>()), Times.Never);
store.Verify(f => f.WriteFile(It.IsAny<string>(), It.IsAny<string>()), Times.Once);
}
finally
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
namespace Microsoft.Azure.Commands.Profile
{
/// <summary>
/// Cmdlet to get current context.
/// Cmdlet to get current context.
/// </summary>
[Cmdlet(VerbsCommon.Get, "AzureRmContext", DefaultParameterSetName = GetSingleParameterSet)]
[OutputType(typeof(PSAzureContext))]
Expand Down Expand Up @@ -53,13 +53,10 @@ protected override IAzureContext DefaultContext

public override void ExecuteCmdlet()
{
// If no context is found, return
if (DefaultContext == null)
{
WriteError(new ErrorRecord(
new PSInvalidOperationException(Resources.RunLoginCmdlet),
string.Empty,
ErrorCategory.AuthenticationError,
null));
return;
}

if (ListAvailable.IsPresent)
Expand Down Expand Up @@ -102,6 +99,15 @@ void WriteContext(IAzureContext azureContext, string name)
context.Name = name;
}

// Don't write the default (empty) context to the output stream
if (context.Account == null &&
context.Environment == null &&
context.Subscription == null &&
context.Tenant == null)
{
return;
}

WriteObject(context);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public AzureRmProfile Login(
string subscriptionId,
string subscriptionName,
SecureString password,
bool skipValidation,
bool skipValidation,
Action<string> promptAction,
string name = null)
{
Expand Down Expand Up @@ -140,7 +140,6 @@ public AzureRmProfile Login(
Id = tenantId
};
}

else
{
// (tenant and subscription are present) OR
Expand Down Expand Up @@ -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]))
{
Expand All @@ -227,6 +226,7 @@ public AzureRmProfile Login(
}
}

bool shouldPopulateContextList = _profile.DefaultContext?.Account == null;
Copy link
Member

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).

Copy link
Member Author

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 type IProfileOperations, which doesn't contain the Contexts property (the implementation AzureRmProfile does contain this property, but I doubt we want to use that in this class). Is there a separate check we can do here?

Copy link
Member

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

if (newSubscription == null)
{
if (subscriptionId != null)
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 {{subscriptionName}} - {{subscriptionId}}

{
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();
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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)
{
Expand All @@ -600,10 +623,10 @@ private List<AzureTenant> ListAccountTenants(
try
{
var commonTenantToken = AcquireAccessToken(
account,
environment,
account,
environment,
commonTenant,
password,
password,
promptBehavior,
promptAction);

Expand Down