Allow custom releases in ci-operator validations #1410
Conversation
pkg/api/config.go
Outdated
| @@ -684,7 +684,18 @@ func validateLiteralTestStepCommon(fieldRoot string, step LiteralTestStep, seen | |||
| switch obj { | |||
| case PipelineImageStream, ReleaseStreamFor(LatestReleaseName), ReleaseStreamFor(InitialReleaseName), ReleaseImageStream: | |||
| default: | |||
| ret = append(ret, fmt.Errorf("%s.from: unknown imagestream '%s'", fieldRoot, imageParts[0])) | |||
| releaseParts := strings.Split(obj, "-") | |||
There was a problem hiding this comment.
You should just use ReleaseNameFrom
pkg/api/config.go
Outdated
| if len(releaseParts) != 2 || (len(releaseParts) == 2 && releaseParts[0] != StableImageStream) { | ||
| ret = append(ret, fmt.Errorf("%s.from: '%s' is not a valid imagestream reference", fieldRoot, step.From)) | ||
| } else { | ||
| names := sets.NewString() |
There was a problem hiding this comment.
If all we need is the set, why pass the full releases map through the call chain?
There was a problem hiding this comment.
I would expect that performance-wise there wouldn't be much difference since its probably just a pointer being passed around?
If so I would prefer to create the set near where its actually being used (unless it is being reused somewhere else)
There was a problem hiding this comment.
Performance-wise, creating new data structures (the set) on every validation is certainly worse.
The stronger argument, though, is that you should not pass data that is not needed. The callers need to know a list of valid releases (or, even, a closure that could validate an ImageStream) and not anything else. Exposing more data than necessary to other parts of a program leads to bad encapsulation and creep.
GeoBK
force-pushed
the
fix/ci-operator/custom-imagestream-validation
branch
from
7fae688 to
9f71830
Compare
|
Implements DPTP-1671 |
GeoBK
force-pushed
the
fix/ci-operator/custom-imagestream-validation
branch
from
9f71830 to
d00f97b
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GeoBK, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
lgtm
No description provided.