DEVPROD-13370 Validate single task distros during project validation #8624
Conversation
validator/project_validator.go
Outdated
| distroWarnings = map[string]string{} | ||
| singleTaskDistroIDs = []string{} |
There was a problem hiding this comment.
It's not actually necessary to initialize an empty slice here, just appending to the nil slice singleTaskDistroIDs is sufficient. The first append will allocate capacity for the slice. Only maps need to be initialized explicitly before use.
There was a problem hiding this comment.
just curious but it's possible that singleTaskDistroIDs is never appended to or touched in the code. would it not error and just pass in nil automatically?
There was a problem hiding this comment.
It's possible that it's never appended to, but it's also quite rare to find code that makes a distinction between a nil slice vs. an empty slice. A nil slice still evaluates len to zero like an empty slice, and you can for loop through it like an empty slice.
The only way you would really tell it's nil and not empty is if you specifically checked if slice == nil. I find that scenario pretty rare compared to length checking.
validator/project_validator.go
Outdated
| Message: fmt.Sprintf("buildvariant '%s' references a single task distro '%s' which is not allowed for buildvariants", buildVariant.Name, name), | ||
| Level: Error, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
RunOn can also be specified for a specific build variant task. Does that also need to be validated?
There was a problem hiding this comment.
wouldn't the code already take that into account since its looping over all tasks in a bv?
There was a problem hiding this comment.
Oh I see there's an outermost loop. Never mind!
validator/project_validator.go
Outdated
| distroWarnings = map[string]string{} | ||
| singleTaskDistroIDs = []string{} |
There was a problem hiding this comment.
just curious but it's possible that singleTaskDistroIDs is never appended to or touched in the code. would it not error and just pass in nil automatically?
validator/project_validator.go
Outdated
| Message: fmt.Sprintf("buildvariant '%s' references a single task distro '%s' which is not allowed for buildvariants", buildVariant.Name, name), | ||
| Level: Error, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
wouldn't the code already take that into account since its looping over all tasks in a bv?
validator/project_validator.go
Outdated
| distroWarnings = map[string]string{} | ||
| singleTaskDistroIDs = []string{} |
There was a problem hiding this comment.
It's possible that it's never appended to, but it's also quite rare to find code that makes a distinction between a nil slice vs. an empty slice. A nil slice still evaluates len to zero like an empty slice, and you can for loop through it like an empty slice.
The only way you would really tell it's nil and not empty is if you specifically checked if slice == nil. I find that scenario pretty rare compared to length checking.
validator/project_validator.go
Outdated
| Message: fmt.Sprintf("buildvariant '%s' references a single task distro '%s' which is not allowed for buildvariants", buildVariant.Name, name), | ||
| Level: Error, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Oh I see there's an outermost loop. Never mind!
DEVPROD-13370
Description
during project validation, any tasks that specify a single task distro will need to be on an allowed list or it will error
Testing
unit testing
(e2e will be done later)