simpler user invariant: username has "@" => username is not local #56

Merged
merged 1 commit into from Oct 9, 2015

Conversation

Projects
None yet
4 participants
Owner

rogpeppe commented Oct 9, 2015

We lose the concept of "local" as a special name, leading to a much
simpler invariant - a username is external if and only if it has an "@" suffix.
The local domain is intuitively represented by the empty string.

This means that there is no need for the Canonical method, as users
are always in canonical form, and it also means that users as stored
currently in the juju state are already in canonical form as local users,
so there will be no need for a migration when we add information about
external users to the user database.

(Review request: http://reviews.vapour.ws/r/2867/)

user.go
// IsLocal returns true if the tag represents a local user.
func (t UserTag) IsLocal() bool {
- return t.Domain() == LocalUserDomain
+ return t.Domain() == ""
}
// Domain returns the user domain. Users in the local database
// are from the LocalDomain. Other users are considered 'remote' users.
@mhilton

mhilton Oct 9, 2015

Member

please comment that "" is LocalDomain

Member

mhilton commented Oct 9, 2015

LGTM with a thought

LGTM

Owner

rogpeppe commented Oct 9, 2015

$$merge$$

Contributor

jujubot commented Oct 9, 2015

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-names

jujubot added a commit that referenced this pull request Oct 9, 2015

Merge pull request #56 from rogpeppe/002-simpler-local-user-invariant
simpler user invariant: username has "@" => username is not local

We lose the concept of "local" as a special name, leading to a much
simpler invariant - a username is external if and only if it has an "@" suffix.
The local domain is intuitively represented by the empty string.

This means that there is no need for the Canonical method, as users
are always in canonical form, and it also means that users as stored
currently in the juju state are already in canonical form as local users,
so there will be no need for a migration when we add information about
external users to the user database.


(Review request: http://reviews.vapour.ws/r/2867/)

@jujubot jujubot merged commit 657c4ad into juju:master Oct 9, 2015

rogpeppe added a commit to rogpeppe/juju-names that referenced this pull request Oct 14, 2015

revert PR #56
	This is too controversial, so we revert the changes

jujubot added a commit that referenced this pull request Oct 14, 2015

Merge pull request #57 from rogpeppe/003-revert-PR-56
revert PR #56

This is too controversial, so we revert the changes.


(Review request: http://reviews.vapour.ws/r/2897/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment