spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
73.68k stars 40.34k forks source link

Docker publishRegistry in Maven plugin configuration is validated when publish option is false #29756

Open cdprete opened 2 years ago

cdprete commented 2 years ago

Hi. I'm using Spring Boot 2.6.3 and I've the spring-boot-maven-plugin configured in the following way:

                <plugin>
                    <groupId>org.springframework.boot</groupId>
                    <artifactId>spring-boot-maven-plugin</artifactId>
                    <version>${spring-boot.version}</version>
                    <executions>
                        <execution>
                            <goals>
                                <goal>repackage</goal>
                            </goals>
                        </execution>
                        <execution>
                            <id>build-image</id>
                            <goals>
                                <goal>build-image</goal>
                            </goals>
                            <configuration>
                                <image>
                                    <name>${docker.image-name}</name>
                                </image>
                                <docker>
                                    <publishRegistry>
                                        <username>${docker.credentials.username}</username>
                                        <password>${docker.credentials.password}</password>
                                    </publishRegistry>
                                </docker>
                            </configuration>
                        </execution>
                    </executions>
                </plugin>

where the registry credentials are simply empty as

<docker.credentials.username />
<docker.credentials.password />

I've tried to set the publish flag dynamically with Groovy in order to set it to false if one of the credentials is missing. Everything works fine if both the credentials are missing. As soon as one is specified, the publishRegistry block complains even though the publish flag is false.

The Groovy part is

                <plugin>
                    <groupId>org.codehaus.gmavenplus</groupId>
                    <artifactId>gmavenplus-plugin</artifactId>
                    <version>1.13.1</version>
                    <dependencies>
                        <dependency>
                            <groupId>org.codehaus.groovy</groupId>
                            <artifactId>groovy-all</artifactId>
                            <version>3.0.9</version>
                            <type>pom</type>
                        </dependency>
                    </dependencies>
                    <executions>
                        <execution>
                            <id>check-if-image-must-be-published</id>
                            <phase>validate</phase>
                            <goals>
                                <goal>execute</goal>
                            </goals>
                        </execution>
                    </executions>
                    <configuration>
                        <scripts>
                            <script><![CDATA[
                                def dockerCredentialsUsernamePropertyName = 'docker.credentials.username'
                                def dockerCredentialsPasswordPropertyName = 'docker.credentials.password'
                                def dockerCredentialsUsername = checkPropertyValue(dockerCredentialsUsernamePropertyName)
                                def dockerCredentialsPassword = checkPropertyValue(dockerCredentialsPasswordPropertyName)
                                if(!dockerCredentialsUsername || !dockerCredentialsPassword) {
                                    project.properties['spring-boot.build-image.publish'] = false
                                }

                                String checkPropertyValue(String name) {
                                    def value = getPropertyValue(name)
                                    if(!value)  {
                                        log.warn "The property '$name' is not set or it's blank, therefore no image will be pushed."
                                    }

                                    return value
                                }

                                String getPropertyValue(String name) {
                                    // property was defined from command line e.g.: -DpropertyName=value
                                    def value = session.userProperties[name]?.trim()
                                    if (!value) {
                                        value = project.properties[name]?.trim()
                                    }
                                    return value
                                }
                            ]]></script>
                        </scripts>
                    </configuration>
                </plugin>

Ideally, if the publish flag is false, I would expect that the registry is completely ignored since it will not be used anyway.

wilkinsona commented 2 years ago

As with #29706, you can save us all some time by providing a minimal sample that reproduces the problem. It would also be useful if you shared the build output when the problem occurs.

cdprete commented 2 years ago

@wilkinsona I gave you here already all the needed building blocks ;) Anyway, here there is a demo for it: https://github.com/cdprete/github_spring_boot_issue_29756 As you can see from the README, the publishRegistry block is evaluated anyway, even if the spring-boot.build-image.publish flag is set to false.

scottfrederick commented 2 years ago

@cdprete Is there a real-world reason for providing either the username or password alone, instead of always providing neither or both, or is this something you happened up when testing? The plugin is working as designed now, and I'm not sure that ignoring invalid configuration that might not be used is a good idea. I'll label it so the rest of the team can provide opinions on this also.

cdprete commented 2 years ago

@scottfrederick I want to prevent build failures if not all the properties have been provided. Moreover, if I set the plugin to skip the publishing, I assume it should not really care what it's under the publishRegistry block.

In the ideal world, you would have 2 different goals bound to 2 different phases. One for building the image (done on package for example) and one for publishing it (done on deploy for example).

snicoll commented 2 years ago

In the ideal world, you would have 2 different goals bound to 2 different phases. One for building the image (done on package for example) and one for publishing it (done on deploy for example).

The Spring Boot plugin is not a general purpose plugin for Docker operations so we are not going to do that, see #26187.

I agree that we should not attempt to validate that part of a plugin configuration is valid if the feature is disabled. I can see now that everything is backed by a DockerConfiguration object. Perhaps this can be refactored to lazily request things rather than doing this upfront?

cdprete commented 2 years ago

I was not aware of https://github.com/spring-projects/spring-boot/issues/26187 :) But yes, the main issue remains ;)