Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Remove @local from local users #6388
Conversation
axw
reviewed
Oct 6, 2016
Nothing glaring, will have to come back to this later. Have you tested that the updated client still works with an rc2 server without upgrading?
| @@ -62,37 +82,3 @@ func ParseAccounts(data []byte) (map[string]AccountDetails, error) { | ||
| type accountsCollection struct { | ||
| ControllerAccounts map[string]AccountDetails `yaml:"controllers"` | ||
| } | ||
| - | ||
| -// TODO(axw) 2016-07-14 #1603841 |
axw
Oct 6, 2016
Member
Please add "fixes ..." to the PR, and mark fix committed when this lands. I've just assigned it to you.
| @@ -88,7 +82,7 @@ func validateUser(name string) error { | ||
| } | ||
| func validateUserTag(tag names.UserTag) error { | ||
| - if tag.Id() != tag.Canonical() { | ||
| + if tag.Id() != tag.Id() { |
| @@ -163,15 +163,15 @@ func TargetPrecheck(backend PrecheckBackend, modelInfo coremigration.ModelInfo) | ||
| if err != nil { | ||
| return errors.Annotate(err, "retrieving models") | ||
| } | ||
| - canonicalOwner := modelInfo.Owner.Canonical() | ||
| + canonicalOwner := modelInfo.Owner.Id() |
axw
Oct 6, 2016
Member
s/canonicalOwner/owner/?
or perhaps just use modelInfo.Owner.Id() inline below
mjs
reviewed
Oct 6, 2016
Generally ok (mammoth effort!) but would like issues considered before approving.
| ) | ||
| var upgradesLogger = loggo.GetLogger("juju.state.upgrade") | ||
| -func AddPreferredAddressesToMachines(st *State) error { |
| + newId := id | ||
| + | ||
| + // Take a copy of the current doc fields. | ||
| + newDoc := make(bson.M) |
mjs
Oct 6, 2016
Contributor
Using bson.M instead of bson.D will rearrange field ordering which makes me nervous. See https://bugs.launchpad.net/juju-core/+bug/1451674
We had helpers in this file in 1.25 do make reading and updating bson.D based documents easier. See: https://github.com/juju/juju/blame/1.25/state/upgrades.go#L1188
| @@ -18,7 +18,7 @@ import ( | ||
| // (below). | ||
| var stateUpgradeOperations = func() []Operation { | ||
| steps := []Operation{ | ||
| - upgradeToVersion{version.MustParse("2.0.0"), []Step{}}, | ||
| + upgradeToVersion{version.MustParse("2.0.1"), stateStepsFor20()}, |
| @@ -24,6 +24,20 @@ type steps20Suite struct { | ||
| var _ = gc.Suite(&steps20Suite{}) | ||
| +func (s *steps20Suite) TestStateStepsFor20(c *gc.C) { |
| @@ -54,7 +54,7 @@ func NewCloudAPI(backend Backend, authorizer facade.Authorizer) (*CloudAPI, erro | ||
| if !ok { | ||
| return false | ||
| } | ||
| - return isAdmin || userTag.Canonical() == authUser.Canonical() | ||
| + return isAdmin || userTag.Id() == authUser.Id() |
| return nil | ||
| } | ||
| // We can't just compare the UserTags themselves as the provider part | ||
| // may be unset, and gets replaced with 'local'. We must compare against | ||
| // the Canonical value of the user tag. | ||
| - if m.apiUser.Canonical() == user.Canonical() { | ||
| + if m.apiUser.Id() == user.Id() { |
| @@ -245,7 +245,7 @@ func (m *ModelManagerAPI) CreateModel(args params.ModelCreateArgs) (params.Model | ||
| return result, errors.Trace(err) | ||
| } | ||
| } else { | ||
| - if ownerTag.Canonical() == controllerModel.Owner().Canonical() { | ||
| + if ownerTag.Id() == controllerModel.Owner().Id() { |
| for _, model := range models { | ||
| // If the model is importing then it's probably left behind | ||
| // from a previous migration attempt. It will be removed | ||
| // before the next import. | ||
| if model.UUID() == modelInfo.UUID && model.MigrationMode() != state.MigrationModeImporting { | ||
| return errors.Errorf("model with same UUID already exists (%s)", modelInfo.UUID) | ||
| } | ||
| - if model.Name() == modelInfo.Name && model.Owner().Canonical() == canonicalOwner { | ||
| + if model.Name() == modelInfo.Name && model.Owner().Id() == modelInfo.Owner.Id() { |
| c.Assert(modelUser.DisplayName, gc.Equals, user.DisplayName()) | ||
| c.Assert(modelUser.Access, gc.Equals, permission.WriteAccess) | ||
| - c.Assert(modelUser.CreatedBy.Id(), gc.Equals, "createdby@local") | ||
| + c.Assert(modelUser.CreatedBy.Id(), gc.Equals, "createdbycmdCredentialSuite") |
| c.Assert(modelUser.DisplayName, gc.Equals, user.DisplayName()) | ||
| c.Assert(modelUser.Access, gc.Equals, permission.WriteAccess) | ||
| - c.Assert(modelUser.CreatedBy.Id(), gc.Equals, "createdby@local") | ||
| + c.Assert(modelUser.CreatedBy.Id(), gc.Equals, "createdbycmdCredentialSuite") |
| + isId := field == "_id" | ||
| + fieldVal, err := readBsonDField(doc, field) | ||
| + if err != nil { | ||
| + continue |
axw
Oct 6, 2016
•
Edited 1 time
-
axw
Oct 6, 2016
Member
-
Oct 6, 2016
it's a bit alarming to ignore the error. can we either check for NotFound specifically, or otherwise change readBsonField to return a bool instead of an error?
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedOct 6, 2016
•
Edited 1 time
-
wallyworld
Oct 6, 2016
Fixes: https://bugs.launchpad.net/juju/+bug/1596605
Fixes: https://bugs.launchpad.net/juju/+bug/1603841
Local users now no longer use the "@Local" suffix.
There's lots of small changes and a few larger ones. The many, many small changes are to stop calling user.Canonical() and instead use user.Id(). The Canonical() method no longer exists.
The larger change is the addition of upgrade steps. Database fields written with user@local need to be upgraded. The local client yaml files are also migrated. Note that there was a bunch of old upgrade code still present which was removed, and also some missing upgrade tests which were added.
QA: bootstrap from a beta. Upgrade juju. Check the machine log to ensure upgrades steps were run. Ensure that the local yaml files are updated. Do a db-dump and check the relevant fields. Ensure that Juju commands still work and existing permissions are still in place.
Also check that you can use an updated client with a rc2 server without upgrading. Ran various commands to grant and show permissions, deployed a charm etc.