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

Adding the from.platforms Jib config parameter to jib-maven-plugin and to the jib-gradle-plugin #2568

Merged
merged 21 commits into from
Jul 10, 2020
Merged

Adding the from.platforms Jib config parameter to jib-maven-plugin and to the jib-gradle-plugin #2568

merged 21 commits into from
Jul 10, 2020

Conversation

louismurerwa
Copy link
Contributor

@louismurerwa louismurerwa commented Jul 6, 2020 •
edited by chanseokoh
Loading

Toward #1567

@louismurerwa louismurerwa changed the title WIP : Adding the platforms tag to jib-maven-plugin #2148 Adding the platforms tag to jib-maven-plugin #2148 Jul 7, 2020
@louismurerwa louismurerwa requested a review from a team July 7, 2020 18:31
karlmuscat and others added 3 commits July 8, 2020 15:03
@karlmuscat @chanseokoh @louismurerwa
New option to skip pushing images (manifests) if the image (manifest)…
0082ce9 
… already exists in the target registry (#2531)

* Adds proposal to control the pushing of tags on existing images in target registry.

* Updates proposal with more detail.

* Work for tags-on-existing-images.md

* Cleanup.

* * Simplified implementation:
  * Removed newly added class ManifestDescriptor, replaced it with already existing ManifestAndDigest.
  * Removed the CheckImageStep and incorporated the check within the PushImageStep. Implementation now follows similar checks, eg, in PushBlopStep.
* Adds unit and integration tests.
* Fixes resulting from testing.

* Code formatting changes.

* Code cleanup, post-review.

* Code cleanup, post-review.

* * Adds a new CheckImageStep and integrates it in the StepsRunner.
* Reverts PushImageStep to the state before the PR started, apart from the manifest existence check.

* Refactor code

* CHANGELOG

* Update comments

* Improve test

Co-authored-by: Chanseok Oh <[email protected]>
@louismurerwa
Making the os/platform nullable
22fcbbf
@louismurerwa
Merge remote-tracking branch 'origin/master' into MavenPluginPlatform…
fcf6d62 
…sTag
chanseokoh
chanseokoh previously approved these changes Jul 8, 2020
View reviewed changes
Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

LGTM. Fix the minor Javadoc issue, and should be good to go.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java Outdated Show resolved Hide resolved
@chanseokoh chanseokoh changed the title Adding the platforms tag to jib-maven-plugin #2148 Adding the from.platforms Jib config parameter to jib-maven-plugin #2148 Jul 8, 2020
@chanseokoh chanseokoh changed the title Adding the from.platforms Jib config parameter to jib-maven-plugin #2148 Adding the from.platforms Jib config parameter to jib-maven-plugin Jul 8, 2020
@chanseokoh
Copy link
Member

Oh, I thought you'd merge this and start a new PR for further changes. Whatever works, I'm good with it.

And it's a good practice to put some relevant description in the first PR comment.

@chanseokoh chanseokoh dismissed their stale review July 8, 2020 18:07

PR still work in progress

@louismurerwa
Copy link
Contributor Author

louismurerwa commented Jul 8, 2020 •
edited
Loading

I added the getPlaforms method to the RawConfiguration interface .This interface has not be impleneted by GradleRawConfiguration yet so some of the tests will fail because of that.I am working on implementing the Gradle side right now but feel free to review the current changes .

@louismurerwa louismurerwa changed the title Adding the from.platforms Jib config parameter to jib-maven-plugin PR still work in progress : Adding the from.platforms Jib config parameter to jib-maven-plugin and to the jib-gradle-plugin Jul 8, 2020
@chanseokoh
Copy link
Member

The RawConfiguration changes look good to me.

@louismurerwa louismurerwa changed the title PR still work in progress : Adding the from.platforms Jib config parameter to jib-maven-plugin and to the jib-gradle-plugin Adding the from.platforms Jib config parameter to jib-maven-plugin and to the jib-gradle-plugin Jul 9, 2020
@louismurerwa louismurerwa requested review from chanseokoh and a team July 9, 2020 17:16
chanseokoh
chanseokoh reviewed Jul 9, 2020
View reviewed changes
...plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/RawConfiguration.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/PlatformsParametersSpec.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/PlatformsParametersSpec.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/PlatformsParameters.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/PlatformsParameters.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/PlatformsParameters.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BaseImageParameters.java Outdated Show resolved Hide resolved
@louismurerwa
Copy link
Contributor Author

I am getting a Warning: Task type 'com.google.cloud.tools.jib.gradle.BuildDockerTask': property 'jib.from.platforms' is not annotated with an input or output annotation. when I run ./gradlew check .What is the output/input annotation?

@chanseokoh
Copy link
Member

FTR: we realized there was a really tricky issue on the Gradle side, so I needed to take care of that part.

@louismurerwa please verify there's no issue when running a snapshot version.

@chanseokoh
Copy link
Member

@louismurerwa actually, I think we can go simpler with renaming the interface method names to getOsName() and getOsArchitecture(). I thought this would change the Maven config UI, but having extra getters doesn't seem to matter. But please verify both Maven and Gradle snapshot versions.

@louismurerwa
Copy link
Contributor Author

Gradle plugin built and ran successfully

@louismurerwa louismurerwa reopened this Jul 10, 2020
@louismurerwa
Copy link
Contributor Author

I have tested both the gradlew and maven plugins using the hellowolrd example and they are working well

@louismurerwa
Copy link
Contributor Author

@louismurerwa actually, I think we can go simpler with renaming the interface method names to getOsName() and getOsArchitecture(). I thought this would change the Maven config UI, but having extra getters doesn't seem to matter. But please verify both Maven and Gradle snapshot versions.

The plugins are compiling and executing without errors on my local environment.

chanseokoh
chanseokoh reviewed Jul 10, 2020
View reviewed changes
Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

LGTM. Only some nits.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BaseImageParameters.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BaseImageParameters.java Show resolved Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java Outdated Show resolved Hide resolved
@louismurerwa louismurerwa merged commit daed808 into master Jul 10, 2020
@louismurerwa louismurerwa deleted the MavenPluginPlatformsTag branch July 10, 2020 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chanseokoh chanseokoh chanseokoh approved these changes

@briandealwis briandealwis Awaiting requested review from briandealwis

@loosebazooka loosebazooka Awaiting requested review from loosebazooka

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@louismurerwa @chanseokoh @loosebazooka @googlebot @karlmuscat