fix: Dockerfile template substitution fails with multi-line profile installations#99
Open
b00y0h wants to merge 1 commit into
Open
fix: Dockerfile template substitution fails with multi-line profile installations#99b00y0h wants to merge 1 commit into
b00y0h wants to merge 1 commit into
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the Dockerfile template substitution logic to pass profile installations and labels into awk via environment variables instead of -v (fixing multi-line handling) and corrects the guard that checks for unreplaced placeholders. Sequence diagram for Dockerfile generation with ENVIRON-based substitutionsequenceDiagram
participant User
participant main_sh as main.sh
participant Awk as awk
participant Env as ENVIRON
User->>main_sh: Invoke Dockerfile generation
main_sh->>main_sh: Prepare base_dockerfile
main_sh->>Env: Set PI=profile_installations
main_sh->>Env: Set LBS=labels
main_sh->>Awk: Execute awk with here-string base_dockerfile
Awk->>Env: Read PI via ENVIRON[PI]
Awk->>Env: Read LBS via ENVIRON[LBS]
Awk->>Awk: For each line
Awk-->>main_sh: Return final_dockerfile
main_sh->>main_sh: grep for unreplaced placeholders
alt Placeholders remain
main_sh-->>User: error Unreplaced placeholders remain
else All placeholders replaced
main_sh->>main_sh: Write final_dockerfile to Dockerfile
main_sh-->>User: Dockerfile generation successful
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The guard that checks for unreplaced placeholders only looks for the exact
{{PROFILE_INSTALLATIONS}}and{{LABELS}}strings, but your awk pattern allows optional whitespace inside the braces, so you might want to update the grep patterns (e.g., usegrep -E '\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}') to consistently catch all unreplaced forms. - Since you’re now relying on ENVIRON, consider briefly noting in a code comment that the
PI=andLBS=prefixes are intentional and required for the awk ENVIRON lookup, so future changes don’t inadvertently switch back to-vor remove those environment assignments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The guard that checks for unreplaced placeholders only looks for the exact `{{PROFILE_INSTALLATIONS}}` and `{{LABELS}}` strings, but your awk pattern allows optional whitespace inside the braces, so you might want to update the grep patterns (e.g., use `grep -E '\{\{[[:space:]]*PROFILE_INSTALLATIONS[[:space:]]*\}\}'`) to consistently catch all unreplaced forms.
- Since you’re now relying on ENVIRON, consider briefly noting in a code comment that the `PI=` and `LBS=` prefixes are intentional and required for the awk ENVIRON lookup, so future changes don’t inadvertently switch back to `-v` or remove those environment assignments.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
64bbd58 to
f0a7be5
Compare
awk -v does not support literal newlines in variable assignments, causing multi-line profile installations to fail with "newline in string" errors. The && operators in RUN commands were also mangled because & has special meaning in awk -v value processing. Switch to ENVIRON array which correctly handles multi-line values and special characters. Also fix the guard check which was broken — the || between the two grep commands was accidentally removed in 634a40d, causing grep to interpret the second grep command as a filename argument. Fixes RchGrav#91
f0a7be5 to
6f2242d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
awk -vfailing on multi-line$profile_installationsvalues by switching toENVIRONarray, which correctly handles newlines and special characters||between grep commands)Root cause
awk -v pi="$profile_installations"does not support literal newlines in variable assignments. When profile functions return multi-line RUN commands containing&&, the substitution fails withawk: newline in stringerrors, leaving{{PROFILE_INSTALLATIONS}}placeholders in the generated Dockerfile.The
ENVIRONarray reads variables from the process environment, which has no issues with newlines or special characters like&.Before (broken)
After (fixed)
Test
Fixes #91
Summary by Sourcery
Fix Dockerfile template substitution to correctly handle multi-line profile installation blocks and restore the guard that detects unreplaced placeholders.
Bug Fixes: