pkg/steps: some cleanups for index+bundle image building#1136
Conversation
openshift-ci-robot
added
the
approved
|
/test e2e |
| replace := strings.ReplaceAll(replaceCommand(sub.PullSpec, replaceSpec), `\`, `\\`) | ||
| dockerCommands = append(dockerCommands, fmt.Sprintf(`RUN ["bash", "-c", "%s"]`, replace)) | ||
| // format command for dockerfile | ||
| dockerFormattedCommand := `"` + strings.Join(replaceCommand(sub.PullSpec, replaceSpec), `" "`) + `"` |
There was a problem hiding this comment.
Hrm - shouldn't this be something like
var formattedParts []string
for _, part := range replaceCommand(sub.PullSpec, replaceSpec) {
formattedParts = append(formattedParts. fmt.Sprintf("%q", part))
}
dockerFormattedCommand := strings.Join(formattedParts, " ")?
There was a problem hiding this comment.
Maybe write a function and unit test?
There was a problem hiding this comment.
I've changed the Dockerfile RUN commands to just take the command directly, without quoting every argument. This way, it is run exactly as it would be in the /bin/sh of the container. This is the easiest to understand way to generate the dockerfile IMO.
|
/assign @petr-muller |
|
/close |
|
@stevekuznetsov: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@stevekuznetsov: Reopened this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold This looks legit: |
openshift-ci-robot
added
the
do-not-merge/hold
This PR addresses a variety of comments from openshift#1084 that were unaddressed when the PR merged. These are the changes: - Make functions to create bundle image name and detect if image is bundle name - Add brief Godoc for api.Bundle - Use `find` command directly in bundle substitution instead of executing through bash - Use exact pullspecs from ResolvePullSpec, which also verifies that the image tag exists - Add image substitutions that get built in the pipeline to the required images for bundle-sub - Don't change global state (changing directory) in unit tests - Handle both yaml and yml file extensions for manifests - Handle escaping in dockerfile more cleanly - Remove extra, empty "" lines from beginning and end of generated dockerfiles - Expand replaceCommand unit tests by adding a .yml and .txt file
AlexNPavel
force-pushed
the
fixup-index
branch
from
c6480ca to
14166c3
Compare
DependencyParts now detects the bundle and index images as pipeline images. This commit also adds the PipelineImageStreamTagReference prefix to the statically named bundle and index images.
|
Everything should be functional now |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexNPavel, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
6 participants
This PR addresses a variety of comments from #1084 that were unaddressed when the PR merged.
These are the changes:
findcommand directly in bundle substitution instead of executing through bash/cc @stevekuznetsov