build: Add additional step nodes when labels are modified #3517
Conversation
|
Will add tests once we finalize on this discussion. |
|
@containers/buildah-maintainers @nalind PTAL |
|
Looks reasonable. Tests should definitely make sure this doesn't break weirdly on multistage builds and when the build replaces the base image (i.e., when the |
imagebuildah/build.go
Outdated
| value = "" | ||
| } | ||
| labelLine = fmt.Sprintf("LABEL %s=%s\n", key, value) | ||
| if len(labelLine) > 1 { |
There was a problem hiding this comment.
yeah, might be better to check if key and value don't equal ""
There was a problem hiding this comment.
I think this is needed for basic validation since --label accepts arbitrary string similar check is also present in executor itself.
There was a problem hiding this comment.
Right, but isn't labelLine at a minimum going to contain "LABEL = "? That would never be 0 or less right?
There was a problem hiding this comment.
@TomSweeneyRedHat oh right thanks changing this to key != "", we can support empty value since docker supports empty values (manually verified).
|
@flouthoc are you planning on updating this PR? |
|
@rhatdan Just writing some test cases which covers all edge-cases. |
flouthoc
force-pushed
the
step-modified-labels
branch
2 times, most recently
from
4082903
to
684bb18
Compare
|
@nalind added tests for Muti-stage build test covers
Regular build with --from
Just waiting for everything to pass. |
flouthoc
force-pushed
the
step-modified-labels
branch
2 times, most recently
from
0cb43af
to
1d6ffeb
Compare
flouthoc
force-pushed
the
step-modified-labels
branch
from
1d6ffeb
to
915d975
Compare
imagebuildah/build.go
Outdated
| if len(label) > 1 { | ||
| value = label[1] | ||
| } | ||
| labelLine = fmt.Sprintf("LABEL \"%s\"=\"%s\"\n", key, value) |
There was a problem hiding this comment.
This doesn't look like it'll catch weirdness like values that include internal quotation marks of one kind or another.
There was a problem hiding this comment.
@nalind i tried this buildah build --layers --label "label1=value'2" --from=alpine -t image1 works ! but this is failing buildah build --layers --label 'label1=value"2' --from=alpine -t image1 let me get a fix.
There was a problem hiding this comment.
@nalind I have changed this formatting with simple %q it autoescapes things for us tested both the cases, " will be autoescaped but just thinking if buildah build --layers --label 'label1=value"2' --from=alpine -t image1 is even a valid input since --label accepts a string.
- labelLine = fmt.Sprintf("LABEL \"%s\"=\"%s\"\n", key, value)
+ labelLine = fmt.Sprintf("LABEL %q=%q\n", key, value)
flouthoc
force-pushed
the
step-modified-labels
branch
from
915d975
to
a92e331
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan 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 |
|
Just a rebase since main is updated. |
flouthoc
force-pushed
the
step-modified-labels
branch
from
a92e331
to
ce1b22e
Compare
flouthoc
force-pushed
the
step-modified-labels
branch
from
ce1b22e
to
ec55420
Compare
Make sure label modification behaviour stays similar to docker i.e new step is added when labels are modified and leave caching of layers to executor Advantages. * Layers will be automatically cached if needed. * If Labels are modified , new step will be built and cached if configured instead of forcing pre-built cached layers. Signed-off-by: Aditya Rajan <arajan@redhat.com>
flouthoc
force-pushed
the
step-modified-labels
branch
from
ec55420
to
dd6a599
Compare
|
LGTM |
|
/lgtm |
Make sure label modification behaviour stays similar to docker i.e
new step is added when labels are modified and leave caching of layers
to executor
Advantages.
configured instead of forcing pre-built cached layers.
See detailed discussion here: #3370 (comment)
Closes: #3370