project-copacetic / copacetic

🧵 CLI tool for directly patching container images!
https://project-copacetic.github.io/copacetic/
Apache License 2.0
960 stars 63 forks source link

test: buildkit with defaults #682

Closed MiahaCybersec closed 2 months ago

MiahaCybersec commented 3 months ago

Closes #595

MiahaCybersec commented 3 months ago

It looks like the failing test is "default buildkit address", but the test runs and passes locally. Is it possible that the test is failing because GitHub actions doesn't have a locally running BuildKit client?

The test initializes bkOpts with no values, so bkOpts.Addr is an empty string when NewClient is called. It then tries to connect to a locally running BuildKit service (autoClient) which is where the errors are originating from.

@cpuguy83 mentioned at a previous LFX mentorship meeting that the old TODO is likely referring to testing BuildKit with nothing passed in, which is why this test does not perfectly match up with the old TODO in code.

ashnamehrotra commented 3 months ago

@MiahaCybersec can you check what docker version is being used in the workflow? If it is not 23.0 or later, buildkit will not be enabled by default, and you would need to change the unit test workflow to enable it

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 34.22%. Comparing base (1fcc6e0) to head (c68fbc4). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #682 +/- ## ========================================== + Coverage 33.01% 34.22% +1.20% ========================================== Files 18 18 Lines 1578 1578 ========================================== + Hits 521 540 +19 + Misses 1024 1007 -17 + Partials 33 31 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MiahaCybersec commented 2 months ago

I'm not sure why the lint is failing. It says platforms.Normalize is deprecated, but then says to use platforms.Normalize.

sozercan commented 2 months ago

@MiahaCybersec haha that's confusing. Looks like it's deprecated in favor of platforms.Normalize in github.com/containerd/platforms. If you change the import from github.com/containerd/containerd/platforms to github.com/containerd/platforms it should work.

MiahaCybersec commented 2 months ago

The unit tests for GetDockerTransport have been removed because they were incomplete. The tests did not include all of the necessary checks, and mocks would be required to properly test them. The unit tests will be added in a separate pull request to increase test coverage.