Skip to content
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.

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

new build-args-file parameter which allows to pass build arguments to #935

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

rcerven
Copy link
Contributor

@rcerven rcerven commented Apr 11, 2024

buildah task

KONFLUX-268

Before you complete this pull request ...

Look for any open pull requests in the repository with the title "e2e-tests update" and
see if there are recent e2e-tests updates that will be applicable to your change.

@rcerven rcerven force-pushed the build_args branch 2 times, most recently from 194a3f5 to 11cbb0e Compare April 12, 2024 13:02
@@ -185,6 +193,10 @@ spec:
BUILDAH_ARGS+=("--target=${TARGET_STAGE}")
fi

if [ -n "${BUILD_ARGS_FILE}" ]; then
BUILDAH_ARGS+=("--build-arg-file=${SOURCE_CODE_DIR}/${CONTEXT}/${BUILD_ARGS_FILE}")
Copy link
Contributor

@chmeliik chmeliik Apr 15, 2024

Choose a reason for hiding this comment

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

IMO we shouldn't assume the BUILD_ARGS_FILE is relative to the context dir

Suggested change
BUILDAH_ARGS+=("--build-arg-file=${SOURCE_CODE_DIR}/${CONTEXT}/${BUILD_ARGS_FILE}")
BUILDAH_ARGS+=("--build-arg-file=${SOURCE_CODE_DIR}/${BUILD_ARGS_FILE}")

Buildah only does this for the Containerfile, not for the build-arg-file

$ mkdir context
$ echo 'FROM scratch' > context/Containerfile
$ buildah build -f Containerfile context/
# works
$ echo foo=bar > context/args.conf
$ buildah build -f Containerfile --build-arg-file args.conf context/
Error: open args.conf: no such file or directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we won't be passing just "args.conf" but full path to the context dir

build args are for component, so using context dir makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

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

build args are for component, so using context dir makes sense to me

I don't think we have any basis for this. The user may want to pass the same args to all their components or different args to each component.

I would say that making the path relative to the context dir makes things unnecessarily more confusing

- name: path-context
  value: context
- name: build-arg-file
  value: context/args.conf

# =>
# Error: open source/context/context/args.conf: no such file or directory

# Huh? Why is Konflux saying the file doesn't exist? It exists in my repo.

Especially considering that buildah doesn't behave like that (doesn't consider the --build-arg-file as relative to the context dir)

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rcerven
Copy link
Contributor Author

rcerven commented Apr 15, 2024

I've changed it to " BUILDAH_ARGS+=("--build-arg-file=${SOURCE_CODE_DIR}/${BUILD_ARGS_FILE}")

because that is what I wanted initially do

and it is up to users, if they will provide builargs from the root of the repo "./args.conf", or from context "./context/args.conf"

anyway it is "full" path within repo which will be provided in the --build-arg-file

@rcerven
Copy link
Contributor Author

rcerven commented Apr 15, 2024

/retest

@rcerven rcerven added this pull request to the merge queue Apr 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2024
@rcerven rcerven added this pull request to the merge queue Apr 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2024
@rcerven rcerven added this pull request to the merge queue Apr 15, 2024
Merged via the queue into konflux-ci:main with commit 3e27482 Apr 15, 2024
6 checks passed
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.

4 participants