Skip to content

feat(ISV-7361): add ENV support to Capo#48

Merged
BorekZnovustvoritel merged 3 commits into
mainfrom
ISV-7361
Jul 1, 2026
Merged

feat(ISV-7361): add ENV support to Capo#48
BorekZnovustvoritel merged 3 commits into
mainfrom
ISV-7361

Conversation

@BorekZnovustvoritel

@BorekZnovustvoritel BorekZnovustvoritel commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

According to the Dockerfile spec, the supported statements are:

  • ADD
  • COPY
  • ENV
  • EXPOSE
  • FROM
  • LABEL
  • STOPSIGNAL
  • USER
  • VOLUME
  • WORKDIR
  • ONBUILD

from which the bold ones we parse in Capo.

Aproach

I found out that even the env-variable names can be expanded by variables (testing dockerfile below), so I simply reused the code for labels.

ARG b=foo

FROM alpine:latest
ENV a=b
ENV $a=spam
RUN echo HELLO $b HI   # displays HELLO spam HI

@codecov-commenter

codecov-commenter commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.58%. Comparing base (f728a62) to head (5859392).

Files with missing lines Patch % Lines
cmd/capo/main.go 0.00% 15 Missing ⚠️
cmd/buildprobe/main.go 0.00% 13 Missing ⚠️
pkg/containerfile/containerfile.go 73.68% 5 Missing and 5 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
- Coverage   63.76%   62.58%   -1.18%     
==========================================
  Files           9        9              
  Lines        1217     1259      +42     
==========================================
+ Hits          776      788      +12     
- Misses        330      355      +25     
- Partials      111      116       +5     
Flag Coverage Δ
integration-tests 64.76% <53.33%> (+0.47%) ⬆️
unit-tests 37.64% <50.00%> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/buildvars/buildvars.go 90.24% <100.00%> (ø)
pkg/probe/probe.go 81.51% <100.00%> (+0.15%) ⬆️
pkg/containerfile/containerfile.go 78.05% <73.68%> (-2.50%) ⬇️
cmd/buildprobe/main.go 0.00% <0.00%> (ø)
cmd/capo/main.go 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f728a62...5859392. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BorekZnovustvoritel BorekZnovustvoritel marked this pull request as draft June 25, 2026 14:36
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again

Grey Divider

Qodo Logo

@BorekZnovustvoritel BorekZnovustvoritel marked this pull request as ready for review June 25, 2026 14:50
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Summary by Qodo

Add Dockerfile ENV support for Capo variable substitution
✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

Description

• Track per-stage ENV values and apply Dockerfile-style environment replacement semantics.
• Expand ${VAR} substitutions in WORKDIR and COPY (sources and destination) parsing.
• Add unit/integration coverage for ENV substitution behavior and stage scoping.
Diagram

graph TD
A["Containerfile AST"] --> B["parseStage"] --> C[("envMap")]
B --> F["parseEnv"] --> C
C --> D["ProcessWord"] --> E["WORKDIR/COPY parsing"] --> G["Stage model"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep ordered env slice (append-only) + explicit override resolution
  • ➕ Avoids map->slice regeneration and preserves instruction ordering naturally
  • ➖ More complex to ensure correct 'last value wins' behavior for ProcessWord
  • ➖ Harder to reason about lookups and to keep slice free of stale keys
2. Delegate Dockerfile semantics to an upstream parser/executor model
  • ➕ Potentially closer to Docker/buildah behavior across more instructions
  • ➖ Much heavier dependency/complexity for Capo’s current needs
  • ➖ May require significant refactor and broader test surface

Recommendation: The current approach (maintaining a per-stage env map, updating it once per ENV instruction, and regenerating the env slice for ProcessWord) is a good fit: it’s simple, override-correct, and matches the intended Dockerfile environment replacement semantics validated by the new tests. The slice-only alternative is feasible but would likely add complexity and subtle bugs around overrides.

Files changed (6) +125 / -24

Enhancement (1) +56 / -12
containerfile.goAdd ENV instruction handling and variable expansion for WORKDIR/COPY +56/-12

Add ENV instruction handling and variable expansion for WORKDIR/COPY

• Introduces per-stage env tracking (envMap) initialized from ARGs, updates it on ENV instructions, and uses imagebuilder.ProcessWord to expand variables in WORKDIR, COPY sources, and COPY destination. Updates source normalization to return errors and to expand environment variables after path normalization.

pkg/containerfile/containerfile.go

Refactor (1) +0 / -1
buildargs.goFormatting cleanup in build-arg parsing module +0/-1

Formatting cleanup in build-arg parsing module

• Removes stray whitespace/newline around the build-arg error declaration; no functional behavior changes.

pkg/buildargs/buildargs.go

Tests (4) +69 / -11
buildargs_test.goFix unit test file build tag formatting +1/-0

Fix unit test file build tag formatting

• Adds a blank line after the //go:build directive to satisfy Go formatting/build tag conventions.

pkg/buildargs/buildargs_test.go

containerfile_test.goAdd unit test for ENV substitution semantics and stage scoping +57/-2

Add unit test for ENV substitution semantics and stage scoping

• Adds an "env substitution" test covering Dockerfile-style ENV replacement behavior (instruction-scoped evaluation) and validating that ENV does not leak across stages.

pkg/containerfile/containerfile_test.go

integration_test.goExercise ENV-based path substitution in integration ARG substitution scenario +9/-7

Exercise ENV-based path substitution in integration ARG substitution scenario

• Updates the ARG substitution integration test to set an ENV variable and use it in COPY destination paths, validating end-to-end ENV expansion behavior.

pkg/integration_test.go

scan_test.goTest file formatting alignment +2/-2

Test file formatting alignment

• Applies minor formatting changes (alignment/spacing) in scan tests; no behavioral change.

pkg/scan_test.go

Signed-off-by: Marek Szymutko <mszymutk@redhat.com>
* ran golangci-lint fmt

Signed-off-by: Marek Szymutko <mszymutk@redhat.com>
Signed-off-by: Marek Szymutko <mszymutk@redhat.com>

@bclindner bclindner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No real notes here, looks good. Test changes are clean as always. Very glad we can handle the actual substitution w/ imagebuilder, I was really worried we were gonna have to maintain some shaky solution ourselves

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i assume this rename is because we're using this to parse ENV as well as ARG, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, precisely 😄 I tried to state that in the docstring on top

@BorekZnovustvoritel BorekZnovustvoritel merged commit 26f4957 into main Jul 1, 2026
4 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