Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix platform configuration in Gradle plugin #2783

Merged
merged 6 commits into from
Sep 24, 2020
Merged

Fix platform configuration in Gradle plugin #2783

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5eede51
Fix platform configuration in Gradle plugin
chanseokoh Sep 23, 2020
d97224c
Remove dead code
chanseokoh Sep 23, 2020
1257b11
CHANGELOG
chanseokoh Sep 23, 2020
a80b6da
code style
chanseokoh Sep 23, 2020
d64c1ec
Reset default platform when given "platforms" action
chanseokoh Sep 24, 2020
939c4ef
Remove unused variable
chanseokoh Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...build-plan/src/main/java/com/google/cloud/tools/jib/api/buildplan/ContainerBuildPlan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ public static class Builder {
private Instant creationTime = Instant.EPOCH;
private ImageFormat format = ImageFormat.Docker;

// note that a LinkedHashSet instead of HashSet has been used so as to preserve the platform
// order
// LinkedHashSet to preserve the order
private Set<Platform> platforms =
new LinkedHashSet<>(Collections.singleton(new Platform("amd64", "linux")));

Expand Down
3 changes: 1 addition & 2 deletions jib-core/src/main/java/com/google/cloud/tools/jib/configuration/ContainerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ public static class Builder {
*/
private static final Instant DEFAULT_CREATION_TIME = Instant.EPOCH;

// note that a LinkedHashSet instead of HashSet has been used so as to preserve the platform
// order
// LinkedHashSet to preserve the order
private Set<Platform> platforms =
new LinkedHashSet<>(Collections.singleton(new Platform("amd64", "linux")));
private Instant creationTime = DEFAULT_CREATION_TIME;
Expand Down
1 change: 1 addition & 0 deletions jib-gradle-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ All notable changes to this project will be documented in this file.
### Fixed

- Fixed `NullPointerException` during input validation (in Java 9+) when configuring Jib parameters using certain immutable collections (such as `List.of()`). ([#2702](https:/GoogleContainerTools/jib/issues/2702))
- Fixed an issue that configuring `jib.from.platforms` was always additive to the default `amd64/linux` platform. ([#2783](https:/GoogleContainerTools/jib/issues/2783))

## 2.5.0

Expand Down
21 changes: 15 additions & 6 deletions jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BaseImageParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,32 @@ public class BaseImageParameters {
private final PlatformParametersSpec platformParametersSpec;
private final ListProperty<PlatformParameters> platforms;

private final ObjectFactory objectFactory;

@Inject
public BaseImageParameters(ObjectFactory objectFactory) {
this.objectFactory = objectFactory;
auth = objectFactory.newInstance(AuthParameters.class, "from.auth");
platforms = objectFactory.listProperty(PlatformParameters.class).empty();
platformParametersSpec =
objectFactory.newInstance(PlatformParametersSpec.class, objectFactory, platforms);

PlatformParameters platform = new PlatformParameters();
platform.setOs("linux");
platform.setArchitecture("amd64");
platforms.add(platform);
}

@Nested
@Optional
public ListProperty<PlatformParameters> getPlatforms() {
return platforms;
if (!platforms.get().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a little weird that a getter is mutating things.

Copy link
Member

@loosebazooka loosebazooka Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we just delegate default setting to a lower level of the system?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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().

return platforms;
}

PlatformParameters amd64Linux = new PlatformParameters();
amd64Linux.setArchitecture("amd64");
amd64Linux.setOs("linux");

ListProperty<PlatformParameters> defaultPlatforms =
objectFactory.listProperty(PlatformParameters.class);
defaultPlatforms.add(amd64Linux);
return defaultPlatforms;
}

public void platforms(Action<? super PlatformParametersSpec> action) {
Expand Down
24 changes: 24 additions & 0 deletions jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Properties;
import org.gradle.api.Project;
import org.gradle.testfixtures.ProjectBuilder;
Expand Down Expand Up @@ -56,17 +57,40 @@ public void testFrom() {
Assert.assertNull(testJibExtension.getFrom().getImage());
Assert.assertNull(testJibExtension.getFrom().getCredHelper());

List<PlatformParameters> defaultPlatforms = testJibExtension.getFrom().getPlatforms().get();
Assert.assertEquals(1, defaultPlatforms.size());
Assert.assertEquals("amd64", defaultPlatforms.get(0).getArchitecture());
Assert.assertEquals("linux", defaultPlatforms.get(0).getOs());

// Ensures calling getPlatforms() again doesn't increase the resulting list.
List<PlatformParameters> defaultPlatformsAgain =
testJibExtension.getFrom().getPlatforms().get();
Assert.assertEquals(1, defaultPlatformsAgain.size());

testJibExtension.from(
from -> {
from.setImage("some image");
from.setCredHelper("some cred helper");
from.auth(auth -> auth.setUsername("some username"));
from.auth(auth -> auth.setPassword("some password"));
from.platforms(
platformSpec -> {
platformSpec.platform(
platform -> {
platform.setArchitecture("arm");
platform.setOs("windows");
});
});
});
Assert.assertEquals("some image", testJibExtension.getFrom().getImage());
Assert.assertEquals("some cred helper", testJibExtension.getFrom().getCredHelper());
Assert.assertEquals("some username", testJibExtension.getFrom().getAuth().getUsername());
Assert.assertEquals("some password", testJibExtension.getFrom().getAuth().getPassword());

List<PlatformParameters> platforms = testJibExtension.getFrom().getPlatforms().get();
Assert.assertEquals(1, platforms.size());
Assert.assertEquals("arm", platforms.get(0).getArchitecture());
Assert.assertEquals("windows", platforms.get(0).getOs());
}

@Test
Expand Down