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.

Sign up for GitHub

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

Feature/userdel diffs #584

Merged
merged 3 commits into from
Jan 11, 2017
Merged

Feature/userdel diffs #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df71cb5
user.user: modify diffs for deleting a user
Jan 9, 2017
007c1c5
user.user: verify uid belongs to the user to be deleted
Jan 10, 2017
73bd07e
address PR feedback
Jan 10, 2017
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
148 changes: 58 additions & 90 deletions resource/user/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,19 +131,10 @@ func NewUser(system SystemUtils) *User {

// Check if a user user exists
func (u *User) Check(context.Context, resource.Renderer) (resource.TaskStatus, error) {
var (
userByID *user.User
uidErr error
)

// lookup the user by name and lookup the user by uid
// the lookups return ErrUnsupported if the system is not supported
// lookup the user by name
// ErrUnsupported is returned if the system is not supported
// Lookup returns user.UnknownUserError if the user is not found
// LookupID returns user.UnknownUserIdError if the uid is not found
userByName, nameErr := u.system.Lookup(u.Username)
if u.UID != "" {
userByID, uidErr = u.system.LookupID(u.UID)
}

status := resource.NewStatus()

Expand All @@ -152,10 +143,10 @@ func (u *User) Check(context.Context, resource.Renderer) (resource.TaskStatus, e
return status, ErrUnsupported
}

_, nameNotFound := nameErr.(user.UnknownUserError)

switch u.State {
case StatePresent:
_, nameNotFound := nameErr.(user.UnknownUserError)

switch {
case nameNotFound:
_, err := u.DiffAdd(status)
Expand All @@ -175,37 +166,12 @@ func (u *User) Check(context.Context, resource.Renderer) (resource.TaskStatus, e
}
}
case StateAbsent:
switch {
case u.UID == "":
_, nameNotFound := nameErr.(user.UnknownUserError)

switch {
case nameNotFound:
status.AddMessage(fmt.Sprintf("user %s does not exist", u.Username))
case userByName != nil:
status.RaiseLevel(resource.StatusWillChange)
status.AddDifference("user", fmt.Sprintf("user %s", u.Username), fmt.Sprintf("<%s>", string(StateAbsent)), "")
}
case u.UID != "":
_, nameNotFound := nameErr.(user.UnknownUserError)
_, uidNotFound := uidErr.(user.UnknownUserIdError)

switch {
case nameNotFound && uidNotFound:
status.AddMessage(fmt.Sprintf("user %s and uid %s do not exist", u.Username, u.UID))
case nameNotFound:
status.RaiseLevel(resource.StatusCantChange)
return status, fmt.Errorf("cannot delete user %s with uid %s: user does not exist", u.Username, u.UID)
case uidNotFound:
status.RaiseLevel(resource.StatusCantChange)
return status, fmt.Errorf("cannot delete user %s with uid %s: uid does not exist", u.Username, u.UID)
case userByName != nil && userByID != nil && userByName.Name != userByID.Name || userByName.Uid != userByID.Uid:
status.RaiseLevel(resource.StatusCantChange)
return status, fmt.Errorf("cannot delete user %s with uid %s: user and uid belong to different users", u.Username, u.UID)
case userByName != nil && userByID != nil && *userByName == *userByID:
status.RaiseLevel(resource.StatusWillChange)
status.AddDifference("user", fmt.Sprintf("user %s with uid %s", u.Username, u.UID), fmt.Sprintf("<%s>", string(StateAbsent)), "")
}
err := u.DiffDel(status, userByName, nameNotFound)
if err != nil {
return status, errors.Wrapf(err, "cannot delete user %s", u.Username)
}
if resource.AnyChanges(status.Differences) {
status.AddMessage("delete user")
}
default:
status.RaiseLevel(resource.StatusFatal)
Expand All @@ -217,19 +183,10 @@ func (u *User) Check(context.Context, resource.Renderer) (resource.TaskStatus, e

// Apply changes for user
func (u *User) Apply(context.Context) (resource.TaskStatus, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why couldn't apply use the status that you've created already, again? I feel like we had this discussion, but a) I don't remember and b) has the answer changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe we can since we no longer keep status as a field of the task struct.

var (
userByID *user.User
uidErr error
)

// lookup the user by name and lookup the user by uid
// the lookups return ErrUnsupported if the system is not supported
// lookup the user by name
// ErrUnsupported is returned if the system is not supported
// Lookup returns user.UnknownUserError if the user is not found
// LookupID returns user.UnknownUserIdError if the uid is not found
userByName, nameErr := u.system.Lookup(u.Username)
if u.UID != "" {
userByID, uidErr = u.system.LookupID(u.UID)
}

status := resource.NewStatus()

Expand All @@ -238,10 +195,10 @@ func (u *User) Apply(context.Context) (resource.TaskStatus, error) {
return status, ErrUnsupported
}

_, nameNotFound := nameErr.(user.UnknownUserError)

switch u.State {
case StatePresent:
_, nameNotFound := nameErr.(user.UnknownUserError)

switch {
case nameNotFound:
options, err := u.DiffAdd(status)
Expand Down Expand Up @@ -273,40 +230,18 @@ func (u *User) Apply(context.Context) (resource.TaskStatus, error) {
}
}
case StateAbsent:
switch {
case u.UID == "":
_, nameNotFound := nameErr.(user.UnknownUserError)

switch {
case !nameNotFound && userByName != nil:
err := u.system.DelUser(u.Username)
if err != nil {
status.RaiseLevel(resource.StatusFatal)
status.AddMessage(fmt.Sprintf("error deleting user %s", u.Username))
return status, errors.Wrap(err, "user delete")
}
status.AddMessage(fmt.Sprintf("deleted user %s", u.Username))
default:
status.RaiseLevel(resource.StatusCantChange)
return status, fmt.Errorf("will not attempt to delete user %s", u.Username)
}
case u.UID != "":
_, nameNotFound := nameErr.(user.UnknownUserError)
_, uidNotFound := uidErr.(user.UnknownUserIdError)

switch {
case !nameNotFound && !uidNotFound && userByName != nil && userByID != nil && *userByName == *userByID:
err := u.system.DelUser(u.Username)
if err != nil {
status.RaiseLevel(resource.StatusFatal)
status.AddMessage(fmt.Sprintf("error deleting user %s with uid %s", u.Username, u.UID))
return status, errors.Wrap(err, "user delete")
}
status.AddMessage(fmt.Sprintf("deleted user %s with uid %s", u.Username, u.UID))
default:
status.RaiseLevel(resource.StatusCantChange)
return status, fmt.Errorf("will not attempt to delete user %s with uid %s", u.Username, u.UID)
err := u.DiffDel(status, userByName, nameNotFound)
if err != nil {
return status, errors.Wrapf(err, "will not attempt to delete user %s", u.Username)
}
if resource.AnyChanges(status.Differences) {
err = u.system.DelUser(u.Username)
if err != nil {
status.RaiseLevel(resource.StatusFatal)
status.AddMessage(fmt.Sprintf("error deleting user %s", u.Username))
return status, errors.Wrap(err, "user delete")
}
status.AddMessage(fmt.Sprintf("deleted user %s", u.Username))
}
default:
status.RaiseLevel(resource.StatusFatal)
Expand Down Expand Up @@ -401,6 +336,39 @@ func (u *User) DiffAdd(status *resource.Status) (*AddUserOptions, error) {
return options, nil
}

// DiffDel checks for differences between the current and desired state for the
// user to be deleted indicated by the User fields.
func (u *User) DiffDel(status *resource.Status, userByName *user.User, nameNotFound bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: should this be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to be private, but is public for testing purposes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be better to make the tests part of the package than it would be to expose extra members. But it's definitely not a blocker. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion, we've concluded to leave as is for user since testing is currently performed for a package <name> in package <name_test>. In the next resource added to Converge, we will try testing as part of the package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘 ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #585

if nameNotFound || userByName == nil {
return nil
}

switch {
case u.UID == "":
status.AddDifference("user", u.Username, fmt.Sprintf("<%s>", string(StateAbsent)), "")
case u.UID != "":
userByID, err := user.LookupId(u.UID)
_, uidNotFound := err.(user.UnknownUserIdError)

switch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: this if … else in fancier clothes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you liked switch better. ;) I will change it back to if...else.

case uidNotFound:
status.RaiseLevel(resource.StatusCantChange)
return fmt.Errorf("uid %s does not exist", u.UID)
case userByID != nil && *userByID != *userByName:
status.RaiseLevel(resource.StatusCantChange)
return fmt.Errorf("uid %s belongs to different user", u.UID)
case userByID != nil && *userByID == *userByName:
status.AddDifference("user", u.Username, fmt.Sprintf("<%s>", string(StateAbsent)), "")
}
}

if resource.AnyChanges(status.Differences) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible enhancement: why isn't this a method on status? (cc @rebeccaskinner)

status.RaiseLevel(resource.StatusWillChange)
}

return nil
}

// DiffMod checks for differences between the user associated with u.Username
// and the desired modifications of that user indicated by the other User
// fields. The options to be used for the modify command are set.
Expand Down
Loading