Skip to content

Conversation

@dustymabe
Copy link
Member

  • cmd-build-with-buildah: pass --build-args to versionary
  • cmd-build-with-buildah: refactor versionary logic
  • cmd-build-with-buildah: move version logic into function

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors how the versionary script is called by moving the logic into the build_with_buildah function and passing the --build-args parameter. The changes are well-structured and improve the script's logic. I have one suggestion to further enhance the code by using a bash array to construct arguments, which is a more robust and readable approach.

Comment on lines +128 to +132
if [ -z "${VERSIONARY}" ]; then
dev_arg='--dev'
else
dev_arg=''
fi
# Call versionary. Let it error out if file does not exist.
VERSION=$(src/config/versionary --build-args="src/config/${argsfile}" ${dev_arg})
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To improve robustness and readability, consider building the arguments for the versionary command in an array. This practice avoids potential word-splitting issues with unquoted variables and makes the argument construction clearer.

        local versionary_args=("--build-args=src/config/${argsfile}")
        if [ -z "${VERSIONARY}" ]; then
            versionary_args+=('--dev')
        fi
        # Call versionary. Let it error out if file does not exist.
        VERSION=$(src/config/versionary "${versionary_args[@]}")

No functional change. Prep for later refactor.
No functional change, just reorganizing a bit and adding comments.
This is for downstream where we have different variants and hardcoding
the path to the build-args doesn't work quite so well.
@dustymabe dustymabe force-pushed the dusty-call-versionary branch from 356050f to 5b9bbdd Compare January 27, 2026 22:27
Copy link
Member

@marmijo marmijo left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe dustymabe enabled auto-merge (rebase) January 28, 2026 01:43
@dustymabe dustymabe merged commit 8b5a318 into coreos:main Jan 28, 2026
6 checks passed
@dustymabe dustymabe deleted the dusty-call-versionary branch January 28, 2026 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants