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

Enable context autosave by default #6003

Merged
merged 7 commits into from Apr 30, 2018

Conversation

@cormacpayne
Copy link
Member

commented Apr 20, 2018

Description

Fix for issue #5902

  • Enable context autosave by default
  • When the user runs Connect-AzureRmAccount with the default (empty) context, populate the context list with all subscriptions in the current tenant

Checklist

…tions when logging in with default context set
Copy link
Member

left a comment

Looks like some test updates are needed

@@ -97,6 +97,12 @@ static ContextAutosaveSettings InitializeSessionSettings(IDataStore store, strin
result.Mode = settings.Mode;
result.ContextFile = settings.ContextFile ?? result.ContextFile;
}
else
{
FileUtilities.EnsureDirectoryExists(profileDirectory);

This comment has been minimized.

Copy link
@markcowl

markcowl Apr 20, 2018

Member

Note that we still need to be able to switch back to Process mode if there are any errors in writing these files

This comment has been minimized.

Copy link
@cormacpayne

cormacpayne Apr 20, 2018

Author Member

@markcowl updated this so it remains Process by default but switches to CurrentUser after we are able to write the settings file

{
var tempContext = new AzureContext(subscription, account, environment, newTenant);
string tempName = null;
if (!_profile.TryGetContextName(tempContext, out tempName))

This comment has been minimized.

Copy link
@markcowl

markcowl Apr 20, 2018

Member

Is this using the name of the subcription? I forgot the logic for this

This comment has been minimized.

Copy link
@cormacpayne

cormacpayne Apr 20, 2018

Author Member

@markcowl updated the logic for this after discussion so that the name is now {{subscriptionName}} - {{subscriptionId}}

@markcowl markcowl assigned cormacpayne and unassigned markcowl Apr 20, 2018
@@ -227,6 +226,7 @@ public bool TryRemoveContext(IAzureContext context)
}
}

bool shouldPopulateContextList = _profile.DefaultContext?.Account == null;

This comment has been minimized.

Copy link
@markcowl

markcowl Apr 23, 2018

Member

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

This comment has been minimized.

Copy link
@cormacpayne

cormacpayne Apr 24, 2018

Author Member

@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?

This comment has been minimized.

Copy link
@markcowl

markcowl Apr 24, 2018

Member

makes sense. As long as we are not overwriting existing contexts, I think we are ok here

@markcowl markcowl assigned cormacpayne and unassigned markcowl Apr 24, 2018
@maddieclayton maddieclayton changed the base branch from preview to release-6.0.0 Apr 26, 2018
@cormacpayne

This comment has been minimized.

cormacpayne added 2 commits Apr 28, 2018
@cormacpayne

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

@markcowl markcowl merged commit d512e5c into Azure:release-6.0.0 Apr 30, 2018
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
license/cla All CLA requirements met.
Details
@cormacpayne cormacpayne deleted the cormacpayne:context-default branch May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.