feature: Add the possibility to give secret to buildx build (#1798)

[kevinleturc]

Fix #1798

[rohanKanojia]

@kevinleturc : Could you please add a line to doc/changelog.md regarding this change?

Do you think it's possible to add an integration test for this feature?

[kevinleturc]

@kevinleturc : Could you please add a line to doc/changelog.md regarding this change?

Sure

Do you think it's possible to add an integration test for this feature?

I will check that.

[kevinleturc]

Hi @rohanKanojia, I added the changelog line and an it test. For the it test, I ran into the following error in E2E Tests at the beginning:

Error:  DOCKER> Unable to build image [dmp/alpine:0.45-SNAPSHOT] : "the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled"  ["the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled" ]

In order to fix this error, I needed to use BuildKit, and so edit the BuildXConfiguration#isBuildX method to enable the BuildKit driver to be created when there's secret element in configuration but not a platforms/platform one, like below:

              <build>
                <dockerFile>${project.basedir}/src/main/docker/Dockerfile</dockerFile>
                <buildx>
                  <secret>
                    <files>
                      <myFile>${project.basedir}/../README.md</myFile>
                    </files>
                  </secret>
                </buildx>
              </build>

Which, in my opinion, is a valid usage of the plugin.

But, I don't know if such case could work on the docker buildx driver with some GitHub runner configuration, and so revert the change on BuildXConfiguration#isBuildX method behavior.
Otherwise, if the change of behavior is considered legitimate, I'm thinking about other buildx configuration the plugin have, they might need the same behavior? For instance buildx/attestations, buildx/cacheFrom, etc...

What do think?

[rohanKanojia]

I'm thinking about other buildx configuration the plugin has, they might need the same behavior?

@kevinleturc : You're right. Adding not null checks for each field might be difficult to keep up with in the future. Do you think we can introduce something like isEmpty at BuildXConfiguration level (just like we have in BuildXConfiguration.Builder?

docker-maven-plugin/src/main/java/io/fabric8/maven/docker/config/BuildXConfiguration.java

Line 108 in e980189

private boolean isEmpty = true;

[rohanKanojia]

This is okay for now but we need to improve this in the future.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[codecov]

Codecov Report

Attention: Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.05%. Comparing base (150970d) to head (09811d0).
Report is 5 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1799      +/-   ##
============================================
+ Coverage     65.87%   66.05%   +0.17%     
- Complexity     2306     2321      +15     
============================================
  Files           172      173       +1     
  Lines         10194    10245      +51     
  Branches       1408     1416       +8     
============================================
+ Hits           6715     6767      +52     
+ Misses         2926     2921       -5     
- Partials        553      557       +4     

Files	Coverage Δ
...bric8/maven/docker/config/SecretConfiguration.java	100.00% <100.00%> (ø)
...aven/docker/config/handler/property/ConfigKey.java	100.00% <100.00%> (ø)
...config/handler/property/PropertyConfigHandler.java	86.20% <85.71%> (-0.02%)	⬇️
...bric8/maven/docker/config/BuildXConfiguration.java	94.73% <71.42%> (-1.35%)	⬇️
...io/fabric8/maven/docker/service/BuildXService.java	73.97% <66.66%> (-1.03%)	⬇️

... and 2 files with indirect coverage changes

[kevinleturc]

@kevinleturc : You're right. Adding not null checks for each field might be difficult to keep up with in the future. Do you think we can introduce something like isEmpty at BuildXConfiguration level (just like we have in BuildXConfiguration.Builder?

docker-maven-plugin/src/main/java/io/fabric8/maven/docker/config/BuildXConfiguration.java

Line 108 in e980189

private boolean isEmpty = true;

Yes exactly, something like the current isBuildX but whitout having to state every fields. This should be possible if upper logic juste check buildx nullity to do the buildx things instead of checking if there's more than 1 platform.

I can prepare a PR for this, maybe outside of this one?
Before changing that, I would check that every configuration in the buildx element require the buildkit engine.

I can work on that by the beginning of the week, if everything is ok for you?

[rohanKanojia]

@kevinleturc : Thanks a lot!