Fix platform configuration in Gradle plugin #2783
Conversation
| } | ||
|
|
||
| @Nested | ||
| @Optional | ||
| public ListProperty<PlatformParameters> getPlatforms() { | ||
| return platforms; | ||
| if (!platforms.get().isEmpty()) { |
There was a problem hiding this comment.
It does feel a little weird that a getter is mutating things.
There was a problem hiding this comment.
could we just delegate default setting to a lower level of the system?
There was a problem hiding this comment.
Technically it doesn't mutate an internal state, but it's true the class is no longer a POJO or bean. I also didn't like it and wondered other solutions yesterday. Setting the default at other levels could be another way, but it also had some cons.
Today, I think I found a better way than these: start the class with the default value as before, but only reset it when the user specifies platforms {}. This behavior is also more or less similar to how it works in Maven; if nothing is defined, Maven assumes amd64/linux, but as soon as the user specifies <platforms>, it becomes an empty list.. What do you think?
There was a problem hiding this comment.
Hrmm, I just realized I don't understand how this affects verifying image compatibility? Do we have code somewhere that verifies and image matches the specified platform?
There was a problem hiding this comment.
#2781 and this PR have nothing to do with verifying image platforms. The issue was that defining
jib.from.platforms {
platform {
architecture = 'non-amd64'
os = '...'
}
}
incorrectly resulted in two platforms configured: the configured one plus the default amd64/linux. This was because platform { ...} just called ListProperty.add().
| @@ -38,14 +38,14 @@ | |||
| @Inject | |||
| public BaseImageParameters(ObjectFactory objectFactory) { | |||
| auth = objectFactory.newInstance(AuthParameters.class, "from.auth"); | |||
| platforms = objectFactory.listProperty(PlatformParameters.class).empty(); | |||
| platforms = objectFactory.listProperty(PlatformParameters.class); | |||
There was a problem hiding this comment.
empty() turns out to be unnecessary.
* Fix platform configuration in Gradle plugin * CHANGELOG * Reset default platform when given "platforms" action
Fixes #2781.
We started with the default amd64/linux, and we were always adding new platforms.