-
Notifications
You must be signed in to change notification settings - Fork 584
Add the support for ARG in the native builder parser #516
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
Conversation
Sources/NativeBuilder/ContainerBuildIR/Graph/GraphBuilder+Arg.swift
Outdated
Show resolved
Hide resolved
Sources/NativeBuilder/ContainerBuildIR/Graph/GraphBuilder+Arg.swift
Outdated
Show resolved
Hide resolved
Sources/NativeBuilder/ContainerBuildParser/Docker/Instructions/ExposeInstruction.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only fix needed is to fix NSRange vs. String counting.
LGTM otherwise!
Also, the current impl doesn't support the following instructions right?
ARG var=i.e. no valueRUN $vari.e. without enclosing${}ARG var2=${var1:-default}
Could we add an issue for these (if true)
| #expect(Bool(false), "Expected registry image source") | ||
| } | ||
|
|
||
| if let run = stage.nodes[1].operation as? ExecOperation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add guards for accessing the nodes in the stage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crashing immediately with an index out of bounds error in a test provides a clear, specific failure message that pinpoints exactly what assumption was violated, whereas a guard might obscure the root cause. This will also make it easier to add new tests for added instructions in the future.
Tests/NativeBuilderTests/ContainerBuildParserTests/ParserTests.swift
Outdated
Show resolved
Hide resolved
Sources/NativeBuilder/ContainerBuildParser/Docker/DockerInstructionVisitor.swift
Outdated
Show resolved
Hide resolved
Tests/NativeBuilderTests/ContainerBuildParserTests/ParserTests.swift
Outdated
Show resolved
Hide resolved
Sources/NativeBuilder/ContainerBuildParser/Docker/DockerfileParser.swift
Show resolved
Hide resolved
Sources/NativeBuilder/ContainerBuildParser/Docker/DockerInstructionVisitor.swift
Show resolved
Hide resolved
Fixes a warning missed in #516.
This PR adds the support for the ARG instruction in the native builder parser, implements the support for different kinds of ARGs, and performs the substitution of ARG variables in the supported instructions. Resolves #437.
Some features are currently blocked and not included into this PR: