Add a more generic "prepend or append instructions" method#6700
Add a more generic "prepend or append instructions" method#6700mtrmac merged 1 commit intocontainers:mainfrom
Conversation
nalind
added
the
No New Tests
dosubot
bot
added
the
size:M
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Refactor our handling of --env and --label options so that it'll be easier to add more instructions at the beginning and end of stages. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
nalind
force-pushed
the
pend-instructions
branch
from
1351fc0 to
b66c80d
Compare
|
@containers/buildah-maintainers PTAL, I'd like to make this available for use in #6556 |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
LGTM — feel free to merge with or without the append part.
Generally, I’d aesthetically prefer an architecture where the in-memory data is constructed ~directly (perhaps with a createNameVal counterpart to parseNameVal instead of the round-trip through a text syntax (and worrying about escaping control syntax), but 1) this is pre-existing, and 2) we would need to construct Node.Original anyway, so there would not be much of a benefit.
| } | ||
| stage.Node.Children = append(stage.Node.Children, additionalNode.Children...) | ||
| } | ||
| appendInstructions = slices.Concat(appendInstructions, []string{labelLine}) |
There was a problem hiding this comment.
Isn’t this just append? Also in the other code.
There was a problem hiding this comment.
My expectation is that we'll need to turn prependInstructions and appendInstructions into arguments for this method in #6556, so we'd need to change this instance to something similar. You're right about the other one.
There was a problem hiding this comment.
ACK, slices.Concat is certainly safer.
2 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Refactor our handling of
--envand--labeloptions so that it'll be easier to add more instructions at the beginning and end of stages.How to verify it
This shouldn't break the current set of tests.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?