Skip to content

executor: expand args in build logs #6329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

executor: expand args in build logs #6329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2065ea
executor: expand args in build logs
flouthoc Aug 9, 2025
90e866e
retrofit tests
flouthoc Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion imagebuildah/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io"
"os"
"regexp"
"slices"
"strconv"
"strings"
Expand Down Expand Up @@ -519,6 +520,12 @@ func (b *Executor) getImageTypeAndHistoryAndDiffIDs(ctx context.Context, imageID
return oci.OS, oci.Architecture, manifestFormat, oci.History, oci.RootFS.DiffIDs, nil
}

// Checks if is word is using any variable in the form of `$VAR`, `${VAR}` etc
func containsVariable(arg string) bool {
re := regexp.MustCompile(`\$\{?[a-zA-Z_][a-zA-Z0-9_]*\}?`)
return re.MatchString(arg)
Comment on lines +525 to +526
Copy link
Member

@Luap99 Luap99 Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That compiles the regex every single time which is quite inefficient. That should use something like regexp.Delayed() in the global scope.

But is the regex really right? At the very least it ignores escaping rules \$var is not a variable. ProcessWord() has already has a parser so this would need to match exactly what it is doing. I think it would be better to reuse the ProcessWord code and expose some function for this instead, yeah sure more complicated but the only sane way to avoid any differences between the var replacement code (ProcessWord) and this custom regex

}

func (b *Executor) buildStage(ctx context.Context, cleanupStages map[int]*StageExecutor, stages imagebuilder.Stages, stageIndex int) (imageID string, ref reference.Canonical, onlyBaseImage bool, err error) {
stage := stages[stageIndex]
ib := stage.Builder
Expand Down Expand Up @@ -600,7 +607,26 @@ func (b *Executor) buildStage(ctx context.Context, cleanupStages map[int]*StageE
prefix += ": "
}
suffix := "\n"
fmt.Fprintf(stageExecutor.executor.out, prefix+format+suffix, args...)
builtinArgs := argsMapToSlice(stage.Builder.BuiltinArgDefaults)
headingArgs := argsMapToSlice(stage.Builder.HeadingArgs)
userArgs := argsMapToSlice(stage.Builder.Args)
userArgs = append(builtinArgs, append(userArgs, headingArgs...)...)
processedArgs := make([]any, 0, len(args))
for i := range args {
if arg, ok := args[i].(string); ok {
if containsVariable(arg) {
processedArg, err := imagebuilder.ProcessWord(arg, userArgs)
if err != nil {
processedArg = arg
fmt.Fprintf(stageExecutor.executor.err, "while replacing arg variables with values for format in logging %q: %v\n", arg, err)
}
processedArgs = append(processedArgs, processedArg)
} else {
processedArgs = append(processedArgs, arg)
}
}
Comment on lines +616 to +627
Copy link
Member

@Luap99 Luap99 Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still discards any arg which is not a string, the else block must be one lever out because it needs to keep the non string args otherwise the formatting string will be messed up badly.

}
fmt.Fprintf(stageExecutor.executor.out, prefix+format+suffix, processedArgs...)
}
}
b.stagesLock.Unlock()
Expand Down
48 changes: 44 additions & 4 deletions tests/bud.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5425,10 +5425,11 @@ EOM

${OCI} --version
_prefetch alpine busybox

run_buildah build --build-arg base=alpine --build-arg toolchainname=busybox --build-arg destinationpath=/tmp --pull=false $WITH_POLICY_JSON -f $BUDFILES/from-with-arg/Containerfile .
toolchainname=busybox
destinationpath=/tmp
run_buildah build --build-arg base=alpine --build-arg toolchainname=$toolchainname --build-arg destinationpath=$destinationpath --pull=false $WITH_POLICY_JSON -f $BUDFILES/from-with-arg/Containerfile .
expect_output --substring "FROM alpine"
expect_output --substring 'STEP 4/4: COPY --from=\$\{toolchainname\} \/ \$\{destinationpath\}'
expect_output --substring "STEP 4/4: COPY --from=$toolchainname \/ $destinationpath"
run_buildah rm -a
}

Expand Down Expand Up @@ -6186,7 +6187,7 @@ _EOF
# must push cache twice i.e for first step and second step
run printf "STEP 2/3: ARG VAR=hello\n--> Pushing cache"
step1=$output
run printf "STEP 3/3: RUN echo \"Hello \$VAR\""
run printf "STEP 3/3: RUN echo \"Hello hello\""
step2=$output
run printf "Hello hello"
step3=$output
Expand Down Expand Up @@ -8874,3 +8875,42 @@ _EOF
run_buildah --root=${TEST_SCRATCH_DIR}/newroot --storage-opt=imagestore=${TEST_SCRATCH_DIR}/root build --pull=never ${contextdir}
run_buildah --root=${TEST_SCRATCH_DIR}/newroot --storage-opt=imagestore=${TEST_SCRATCH_DIR}/root build --pull=never --squash ${contextdir}
}

@test "bud build argument expansion in log" {
_prefetch alpine
target=build-arg-expand-log

# Test basic build argument expansion
run_buildah build $WITH_POLICY_JSON -t ${target} --build-arg foo=foovalue -f $BUDFILES/build-arg/Dockerfile $BUDFILES/build-arg
expect_output --substring "RUN echo foovalue"

# Test with a more complex build argument
run_buildah build $WITH_POLICY_JSON -t ${target}-complex --build-arg foo="/usr/local/bin" -f $BUDFILES/build-arg/Dockerfile $BUDFILES/build-arg
expect_output --substring "RUN echo /usr/local/bin"

# Test with multiple build arguments
contextdir=${TEST_SCRATCH_DIR}/build-arg-expansion
mkdir -p $contextdir

cat > $contextdir/Containerfile << _EOF
FROM alpine
ARG NEWROOT
ARG PREFIX
RUN mkdir \$NEWROOT
RUN echo \$PREFIX/bin
_EOF

run_buildah build $WITH_POLICY_JSON -t ${target}-multi --build-arg NEWROOT=/new-root-fs --build-arg PREFIX=/usr $contextdir
expect_output --substring "RUN mkdir /new-root-fs"
expect_output --substring "RUN echo /usr/bin"

# Test that unexpanded variables show as empty when no build-arg is provided
cat > $contextdir/Containerfile2 << _EOF
FROM alpine
ARG UNDEFINEDVAR
RUN echo \$UNDEFINEDVAR hello
_EOF

run_buildah build $WITH_POLICY_JSON -t ${target}-undefined -f $contextdir/Containerfile2 $contextdir
expect_output --substring "RUN echo hello"
}