fix: REPLACE_ME build pack placeholders should work with modern syntax #6700
Conversation
| DockerRegistryOrg string | ||
| } | ||
|
|
||
| func (j *ParsedPipeline) ReplacePlaceholdersInStepAndStageDirs(args StepPlaceholderReplacementArgs) { |
There was a problem hiding this comment.
exported method ParsedPipeline.ReplacePlaceholdersInStepAndStageDirs should have comment or be unexported
abayer
force-pushed
the
replace-placeholder-buildpack-jx-syntax
branch
2 times, most recently
from
a202dd9 to
36cdc41
Compare
|
/retest |
fixes jenkins-x#6698 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer
force-pushed
the
replace-placeholder-buildpack-jx-syntax
branch
from
36cdc41 to
1b2cc1a
Compare
|
/retest |
Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
| } | ||
|
|
||
| // modifyStep allows a container step to be modified to do something different | ||
| func (s *Step) modifyStep(params StepPlaceholderReplacementArgs) { |
There was a problem hiding this comment.
Gee, I was not aware of how tightly the buildpack setup is tied in here. So changes to build packs might have very unexpected side effects. Either when we try to change them or when a user tries to customize.
I realize though that we are just moving code here.
Would it potentially not better to move this somehow out and let the user make a choice, by for example choosing a specific build-pack w/ or w/o kaniko? I seem to recall @garethjevans saying that it would be nice to get this out, but that it would not be so easy?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hferentschik 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 |
Missed this earlier in jenkins-x#6700. Also, when we're converting from a build pack using `jenkins-x.yml` syntax, set the stage directory when unset to the default if the stage contains steps, so we don't need to put a redundant directory on each step in that stage. This logic could probably be improved a bit, but it generates effective syntax that works and is what I would expect it to be. fixes jenkins-x#6717 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Missed this earlier in #6700. Also, when we're converting from a build pack using `jenkins-x.yml` syntax, set the stage directory when unset to the default if the stage contains steps, so we don't need to put a redundant directory on each step in that stage. This logic could probably be improved a bit, but it generates effective syntax that works and is what I would expect it to be. fixes #6717 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Missed this earlier in jenkins-x#6700. Also, when we're converting from a build pack using `jenkins-x.yml` syntax, set the stage directory when unset to the default if the stage contains steps, so we don't need to put a redundant directory on each step in that stage. This logic could probably be improved a bit, but it generates effective syntax that works and is what I would expect it to be. fixes jenkins-x#6717 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Submitter checklist
Description
We were replacing the
REPLACE_ME_*placeholders in directories in lifecycle-basedpipeline.yamlsyntax, but not inpipeline-based syntax inpipeline.yaml, so let's, y'know, do that.Special notes for the reviewer(s)
/assign @hferentschik
Which issue this PR fixes
fixes #6698