add env vars to pipeline strategy #12323
Conversation
bparees
changed the title
|
this can't be merged until the sync plugin is updated to consume these values, but would like to get @openshift/api-review approval rolling. |
pkg/build/api/types.go
Outdated
| @@ -616,6 +616,9 @@ type JenkinsPipelineBuildStrategy struct { | |||
|
|
|||
| // Jenkinsfile defines the optional raw contents of a Jenkinsfile which defines a Jenkins pipeline build. | |||
| Jenkinsfile string | |||
|
|
|||
| // Env contains additional environment variables you want to pass into a build pipeline | |||
| Env []kapi.EnvVar | |||
There was a problem hiding this comment.
Is the full power supported? (ValueFrom, etc). If not, document or limit via validation
There was a problem hiding this comment.
it is not currently (the other env fields for other strategies don't support the full power either, we have a card for that).
i'm not sure if we can ever reasonably support the full power in this case, it would require us doing awkward things like resolving the value before sending the build object to the sync plugin, which i dislike (similar to the awkward things we're doing w/ imagestreams today). (I would not want to make the sync plugin responsible for resolving the value)
There was a problem hiding this comment.
The problem with validation would be breaking anyone who already set it. But adding validation when we create a field is ok. I'll give it a pass for now.
bparees
force-pushed
the
pipeline_env
branch
2 times, most recently
from
c37bbc8 to
064daf7
Compare
|
Jordan if you want to make a case for validation I'm ok with it either way. |
I'd rather prevent the API from accepting data that will a) not behave as expected (since we don't hook up valueFrom stuff), and b) sit inertly and spring into action if we ever do add valueFrom behavior in the future I don't care strongly if that is done via a separate type that omits valueFrom or via validation that prevents it from being set |
ok, i'll plan to add validation to prevent valueFrom on this new field only (as @smarterclayton said, we can't start blocking it on the existing fields, so if/when we get around to supporting it we're still going to have the case of it suddenly springing to life on those fields). |
|
@openshift/api-review i've updated this PR to add validation logic for the env field on pipeline strategies (block use of envVar) |
|
thanks, LGTM |
|
As it turns out we actually have already been enforcing that valueFrom can't be used in the other strategies. So we're in good shape there, at least. |
openshift-bot
added
needs-rebase
openshift-bot
added
the
needs-rebase
|
@bparees - is it reasonable to assume that once the 3.5 branch is cut, and master transitions to hosting 3.6 work, that this PR can be rebased and then merged? |
|
@gabemontero yes |
|
@bparees bump rebase/merge ? thanks |
bparees
force-pushed
the
pipeline_env
branch
from
53514f2 to
4d2ea3c
Compare
openshift-bot
removed
the
needs-rebase
bparees
changed the title
bparees
force-pushed
the
pipeline_env
branch
from
4d2ea3c to
4bfdac8
Compare
|
[merge] |
|
Evaluated for origin merge up to 4bfdac8 |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 4bfdac8 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/559/) (Base Commit: f984cc7) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/568/) (Base Commit: bc6686d) (Image: devenv-rhel7_5982) |
No description provided.