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

Move Dockerfile parsing to a dedicated package, add support for RUN --mount=type=bind,from=... #113

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

tianon
Copy link
Member

@tianon tianon commented Jan 9, 2025

Also, add a bunch of test cases

(made as two commits so it's hopefully easier to review - one that's move + tests, one that's new functionality)

@tianon
Copy link
Member Author

tianon commented Jan 9, 2025

Before this PR: (statements) 84.9%
After this PR: (statements) 85.2%

That only counts code with tests, so I've managed enough coverage here to get a net increase in the covered code even with the added lines. 💪

@tianon tianon force-pushed the run-mount-from-image branch from 66f0a56 to f3d4a41 Compare January 9, 2025 20:41
@tianon tianon force-pushed the run-mount-from-image branch from f3d4a41 to f6b7642 Compare January 9, 2025 20:55
@tianon
Copy link
Member Author

tianon commented Jan 9, 2025

Now with even more coverage!!

-(statements)			84.9%
+(statements)			85.6%
-github.com/docker-library/bashbrew/pkg/dockerfile	0.032s	coverage: 87.1% of statements
+github.com/docker-library/bashbrew/pkg/dockerfile	0.026s	coverage: 91.4% of statements

@tianon tianon force-pushed the run-mount-from-image branch from f6b7642 to c4692fa Compare January 9, 2025 21:03
@tianon tianon force-pushed the run-mount-from-image branch from c4692fa to 0e00438 Compare January 9, 2025 21:06
@tianon
Copy link
Member Author

tianon commented Jan 9, 2025

-(statements)			84.9%
+(statements)			86.3%
-github.com/docker-library/bashbrew/pkg/dockerfile	0.026s	coverage: 91.4% of statements
+github.com/docker-library/bashbrew/pkg/dockerfile	0.027s	coverage: 97.1% of statements

@tianon
Copy link
Member Author

tianon commented Jan 9, 2025

Here's a snapshot:

image

Those are, in order:

  • an impossible codepath (I can't think of any way to reach it -- suggestions welcome!)
  • an error in reading the Dockerfile (which in all our tests comes from a string in memory, so there's no way it could possibly error)

There were some very minor/subtle bugs in how I implemented continuation that wouldn't affect any real-world parsing we did, but still bothered me because I'm me.  This fixes them (and further increases test coverage as a result).
@tianon
Copy link
Member Author

tianon commented Jan 9, 2025

  • an impossible codepath (I can't think of any way to reach it -- suggestions welcome!)

Nerdsniped myself and thought of a way, but it is technically a bug in my parsing of continuation lines, which is now fixed (the previous implementation might've very harmlessly ignored a line whose "instruction" was literally "\" and now it skips that line as empty correctly).

@tianon
Copy link
Member Author

tianon commented Jan 9, 2025

-(statements)			84.9%
+(statements)			86.5%
-github.com/docker-library/bashbrew/pkg/dockerfile	0.027s	coverage: 97.1% of statements
+github.com/docker-library/bashbrew/pkg/dockerfile	0.026s	coverage: 98.7% of statements

This means slightly more typing in "zero-value" cases (`nil` vs `dockerfile.Metadata{}`), but the tradeoff is that it's simpler to use and reason about (and all the struct members are pointer-type map/slice values anyhow, so copying the struct is still pretty cheap).

This also swaps the scanner error handling to return the partially parsed Metadata object alongside the scanner error -- the error already tells us the object isn't fully complete data, so it's fair/fine to return and will likely just be ignored by the caller instead.  This also allows us to get to 100% code coverage. 👀

This also updates our "treat `oci-import` just like `FROM scratch`" code to *actually* parse `FROM scratch` so we can't accidentally cause "missing data" bugs there in the future, and I implemented that using `sync.OnceValues` which requires upgrading to Go 1.21, but IMO that's a worthwhile tradeoff (because `sync.OnceValues` makes that code so clean/simple).
@tianon
Copy link
Member Author

tianon commented Jan 10, 2025

Simplify pkg/dockerfile interface by ditching pointer

This means slightly more typing in "zero-value" cases (nil vs dockerfile.Metadata{}), but the tradeoff is that it's simpler to use and reason about (and all the struct members are pointer-type map/slice values anyhow, so copying the struct is still pretty cheap).

This also swaps the scanner error handling to return the partially parsed Metadata object alongside the scanner error -- the error already tells us the object isn't fully complete data, so it's fair/fine to return and will likely just be ignored by the caller instead. This also allows us to get to 100% code coverage. 👀

This also updates our "treat oci-import just like FROM scratch" code to actually parse FROM scratch so we can't accidentally cause "missing data" bugs there in the future, and I implemented that using sync.OnceValues which requires upgrading to Go 1.21, but IMO that's a worthwhile tradeoff (because sync.OnceValues makes that code so clean/simple).

-(statements)			84.9%
+(statements)			86.7%

@tianon
Copy link
Member Author

tianon commented Jan 10, 2025

https://gha.dag.dev/trace?artifact=2410423127&file=coverage.html&name=coverage&size=22577&uri=https%3A%2F%2Fgithub.com%2Fdocker-library%2Fbashbrew%2Factions%2Fruns%2F12700849704%2Fjob%2F35404320284#file3 if you want to browse the coverage for yourself (time-limited link, because GHA and their artifacts are inherently time-limited)

@yosifkit yosifkit merged commit 882b15e into docker-library:master Jan 17, 2025
9 checks passed
@yosifkit yosifkit deleted the run-mount-from-image branch January 17, 2025 00:51
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.

2 participants