Refactor BMC superuser creation - #93
Conversation
| if strings.Contains(output, "Failure: password incorrect") { | ||
| if strings.Contains(strings.ToLower(output), "password incorrect") { | ||
| // Note: this is also the case if the user does not exist yet but both cases are handled equally | ||
| return true, fmt.Errorf("password for user %s with id %s incorrect: %w change necessary", user.Name, user.Id, err) |
There was a problem hiding this comment.
Should we also remove the error here? At this point we know that password must be changed, so it's safe to conclude that this method must return true. The fact that the password is wrong alone is not an error, it's what the method checks.
This change can slightly simplify the code in metal-hammer
| } | ||
| } | ||
|
|
||
| return false, fmt.Errorf("failed to check if user exists: %w", err) |
There was a problem hiding this comment.
This error is a bit misleading - maybe something like "the requested user is not known to the BMC" will be more accurate? The current one sounds like the check itself failed, while in fact the check proved the requested user doesn't exist.
There was a problem hiding this comment.
Yes, you are absolutely right.
| users = append(users, User{ | ||
| ID: id, | ||
| Name: name, | ||
| ChannelPrivilegeLevel: strings.TrimSpace(fields[5]), |
There was a problem hiding this comment.
Can fields[5] be empty? Probably not, but worth checking/noting in a comment
There was a problem hiding this comment.
As far as I understood, the result should contain some kind of string. In theory, it could be a wrong value, because it is basically just a string. We also verify the output of the strings.Split() operation. However as we do not really need it, I will remove ChannelPrivilegeLevel.
Change the true branch return value to (true, nil) instead of a bit misleading (true, <Error>). The former error is logged at Info level
2 participants
Description
Slightly refactors the code to check, if a password change for the BMC superuser is necessary.
Used AI-Tools ✨