MM-56823: Validate license only when deploying app nodes #822
Conversation
When deploying no app nodes, the license file setting may be empty, since we don't need a license to install anywhere.
defaults/validate.go
Outdated
| @@ -123,6 +123,14 @@ func validate(validation, fieldName string, p, v reflect.Value) error { | |||
| if s == "" || !isAlphanumeric(s) { | |||
| return fmt.Errorf("%s is not alphanumeric", fieldName) | |||
| } | |||
| case "emptyorfile": | |||
There was a problem hiding this comment.
Given the license is the only use of the file tag I wonder if we could more simply make it accept empty values. It would be cool to allow multiple subtags, so one could define something like validate:"notempty,file".
defaults/validate_test.go
Outdated
| err := Validate(&cfg) | ||
| require.Error(t, err) | ||
| }) | ||
|
|
||
| t.Run("not a path for emptyorfile", func(t *testing.T) { | ||
| var cfg serverConfiguration | ||
| Set(&cfg) | ||
|
|
||
| cfg.LicenseFile = "not a file" | ||
|
|
||
| err := Validate(&cfg) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
nit: I usually find value in using EqualError rather than simply checking if it errored. It saved me from a few bugs in the past :)
Add a new "empty" validation as well, that verifies the value is empty (useful only for multiple validations, like in empty;file).
|
I added an OR clause: simply concatenate multiple validations separated by a colon. If one of those validations pass, it is considered valid. What we need for the license in this case is |
|
Re-requesting review just to double-check this is what was on your minds. I do like this solution myself :) |
defaults/validate.go
Outdated
| return nil | ||
| } | ||
|
|
||
| merr.Append(err) |
There was a problem hiding this comment.
You can just use the standard library: https://pkg.go.dev/errors#Join
There was a problem hiding this comment.
Oh, didn't know about that one, thanks!
defaults/validate.go
Outdated
| // Multiple validations separated by a colon (as in "empty;file") behave like an OR clause | ||
| if strings.Contains(validation, ";") { | ||
| individualValidations := strings.Split(validation, ";") |
There was a problem hiding this comment.
I am thinking that calling Split directly and checking if we get more than a single result would be enough?
There was a problem hiding this comment.
Mmmm we could, but I feel like
if individualValidations := strings.Split(validation, "|"); len(individualValidations) > 1 {
is a bit less readable than what we have now, and
individualValidations := strings.Split(validation, "|")
if len(individualValidations) > 1 {
is basically the same, but it makes my brain slightly uncomfortable, it's like it's the other way around 😂
If you feel strongly about this, or if you were thinking about something else, please let me know!
There was a problem hiding this comment.
Nope, I just prefer to keep the scope level down when feasible. It's a preference really so not at all blocking.
defaults/validate_test.go
Outdated
| @@ -18,6 +19,8 @@ func TestValidate(t *testing.T) { | |||
| MaxUsers int `default:"1000" validate:"range:(0,]"` | |||
| LogLevel string `default:"ERROR" validate:"oneof:{TRACE, INFO, WARN, ERROR}"` | |||
| S3URI string `default:"" validate:"s3uri"` | |||
| LicenseFile string `default:"" validate:"empty;file"` | |||
There was a problem hiding this comment.
I recommend | instead of ;. Then we could add & for improved boolean logic :)
There was a problem hiding this comment.
Oooh, I like it! Let's do that :)
There was a problem hiding this comment.
that was half a joke you know :p
There was a problem hiding this comment.
You have to be careful with your jokes!
streamer45
added
4: Reviews Complete
agarciamontoro
deleted the
MM-56823.remove.license.check.when.no.app.deployed
branch
Summary
If we are not deploying app nodes (to load-test an already existing Mattermost installation), we don't need to validate the license (first commit). Actually, we don't even need to specify a file in the config at all (second commit).
Ticket Link
https://mattermost.atlassian.net/browse/MM-56823