Skip to content

fix: update ARG variable handling#665

Merged
patrickhoefler merged 2 commits intomainfrom
fix/arg-resolving
May 23, 2025
Merged

fix: update ARG variable handling#665
patrickhoefler merged 2 commits intomainfrom
fix/arg-resolving

Conversation

@patrickhoefler
Copy link
Copy Markdown
Owner

This pull request introduces enhancements focusing on improving ARG variable substitution, simplifying code, and adding new tests to ensure correctness. The changes include replacing the ARG substitution mechanism with a more robust approach, introducing constants for Dockerfile instructions, and adding test cases to validate nested and unresolved ARG scenarios.

Closes #664.

@patrickhoefler patrickhoefler requested a review from Copilot May 23, 2025 16:04
@patrickhoefler patrickhoefler self-assigned this May 23, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Enhance ARG variable substitution in Dockerfile conversion, streamline constant usage, and add tests to cover nested and unresolved ARG scenarios.

  • Introduce ArgReplacement slices with resolution helpers (appendAndResolveArgReplacement, stripRemainingArgPatterns)
  • Define constants for Dockerfile instructions and precompile regex patterns
  • Add tests for nested ARG substitution and ARG referencing before definition
  • Update linter config to enable ineffassign and staticcheck

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
internal/dockerfile2dot/convert.go Switch to slice-based ARG resolution, add constants, regex, helpers
internal/dockerfile2dot/convert_test.go Add test cases for nested and unresolved ARG variable substitution
.golangci.yaml Enable additional linters (ineffassign, staticcheck)
Comments suppressed due to low confidence (3)

internal/dockerfile2dot/convert.go:75

  • The comment has a typo/grammar issue: 'set is at the name' should be 'set it as the name'.
// If there is an "AS" alias, set is at the name

internal/dockerfile2dot/convert_test.go:315

  • Consider adding a test case for a stage-specific ARG (defined after the first FROM) to verify that the implementation correctly ignores or handles stage-scoped ARG variables.
args: args{

internal/dockerfile2dot/convert.go:217

  • [nitpick] The function name 'appendAndResolveArgReplacement' is quite long; consider renaming to something more concise like 'resolveAndAppendArg' for readability.
func appendAndResolveArgReplacement(

@patrickhoefler patrickhoefler enabled auto-merge (squash) May 23, 2025 16:12
@patrickhoefler patrickhoefler merged commit 199d36f into main May 23, 2025
8 checks passed
@patrickhoefler patrickhoefler deleted the fix/arg-resolving branch May 23, 2025 16:14
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.

Issue resolving nested arguments

2 participants