-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Allow configuring enablePlatformTags via plugins. #3710
Allow configuring enablePlatformTags via plugins. #3710
Conversation
Thank you for following up on the contribution @tylerscoville! We will review this week. |
Allow configuring enablePlatformTags via plugins. Creates a new config parameter jib.to.enablePlatformTags and passes it through to the build context. comment formatting
4dc0a15
to
daa5a4b
Compare
@mpeddada1 Thanks for having a look! I pushed some changes to try and up test coverage and clean up the commit history. |
SonarCloud Quality Gate failed. |
jib-core/src/test/java/com/google/cloud/tools/jib/api/ContainerizerTest.java
Outdated
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java
Outdated
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenRawConfiguration.java
Outdated
Show resolved
Hide resolved
* images are not tagged with platform tags. | ||
* @return this | ||
*/ | ||
public Containerizer setEnablePlatformTags(boolean enablePlatformTags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add an integration test for platform tags in ContainerizerIntegrationTest? Something similar to:
Line 206 in 9027c8d
public void testSteps_forBuildToDockerRegistry_multipleTags() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a little bit of time on this and will probably need some help updating these tests. I'm not quite sure how to add another platform given the test setup?
@tylerscoville Thanks again! I've added a few comments. Let me know if you have any questions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor validation question.
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java
Show resolved
Hide resolved
@mpeddada1 @elefeint Thank you for the review! Sorry it took me so long to get back to this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like adding the new method to RawConfiguration
interface broke maven compilation --
Task :jib-maven-plugin:compileJava FAILED
/home/runner/work/jib/jib/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenRawConfiguration.java:30: error: MavenRawConfiguration is not abstract and does not override abstract method getEnablePlatformTags() in RawConfiguration
public class MavenRawConfiguration implements RawConfiguration {
@tylerscoville THANKS! I went back and looked at attempting this twice. I kept getting lost in the configuration chain. And the job kept getting me distracted with work things. I'm still trying to get some other dependencies updated so I can get to multi-arch. Do you need any help on this? |
I defaulted the RawConfiguration to return false. Hopefully this gets us past that error 🤞 |
@elefeint @mpeddada1 Sorry about the long delay on this. Let me know if there is anything else I can do! |
Continuation of #3518
I'm interested in being able to configure
enablePlatformTags
via the gradle plugin. I think this is the path to do that as described in #3523 (comment).