Move Dockerfile parsing to a dedicated package, add support for RUN --mount=type=bind,from=...#113
Conversation
|
Before this PR: 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. 💪 |
66f0a56 to
f3d4a41
Compare
f3d4a41 to
f6b7642
Compare
|
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 |
f6b7642 to
c4692fa
Compare
Also, add a bunch of test cases / code coverage
c4692fa to
0e00438
Compare
-(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 |
|
Here's a snapshot:
Those are, in order:
|
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).
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 " |
-(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).
-(statements) 84.9%
+(statements) 86.7% |
|
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) |
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)