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.

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

Adds to.tags configuration for Gradle. #978

Merged
merged 10 commits into from
Sep 21, 2018
Merged

Conversation

coollog
Copy link
Contributor

@coollog coollog commented Sep 12, 2018

@coollog coollog changed the title [WIP] Adds to.tags configuration for Gradle. Adds to.tags configuration for Gradle. Sep 14, 2018
@coollog coollog requested a review from a team September 14, 2018 18:49
@@ -190,6 +190,11 @@ public void testBuild_empty() throws IOException, InterruptedException {
new Command("docker", "inspect", "-f", "{{.Created}}", targetImage).run().trim());
}

@Test
public void testBuild_multipleTags() throws IOException, InterruptedException {
// TODO: Implement
Copy link
Member

Choose a reason for hiding this comment

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

If this is to be implemented in a later PR, I'd remove it from here. It's just a test method, and I don't think we need a placeholder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea, I will implement this in this PR. I wanted to solicit some comments about the way BaseImageParameters and TargetImageParameters are laid out first.

/** {@link ImageParameters} that configure the base image. */
public class BaseImageParameters implements ImageParameters {

private AuthParameters auth;
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this can be final.

public ImageParameters(ObjectFactory objectFactory, String imageDescriptor) {
auth = objectFactory.newInstance(AuthParameters.class, imageDescriptor + ".auth");
}
interface ImageParameters {

@Input
Copy link
Member

Choose a reason for hiding this comment

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

Is this @Input in effect in implementation classes? Should this be on implementation classes? (Not sure.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I believe so, since it does not work without @Optional here (Gradle fails with jib.to.image not set), so that indicates that the annotations are inherited from the interface.

}

@Override
public void auth(Action<? super AuthParameters> action) {
Copy link
Member

Choose a reason for hiding this comment

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

Does Action<AuthParameters> work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would work - is there a preference for using the specific type over allowing super types?

Copy link
Member

Choose a reason for hiding this comment

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

This is not a public facing API, and I was wondering if there is anyone calling auth() with a type Action<AuthProperty> for example. Maybe I'm not familiar with Gradle idioms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for like the jib-gradle-plugin jib.to.auth configuration so that users can configure the auth for a base or target image. The actual action that Groovy generates for this may be just Action<AuthParameters>, but I guess someone could decide to use like an Action<Object> that did some arbitrary thing on the auth object.

Copy link
Member

Choose a reason for hiding this comment

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

I think it might have something to do with gradle decorating things.

@coollog coollog requested a review from a team September 18, 2018 20:52
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

It LGTM but I'm not a gradle expert by any means.

@coollog
Copy link
Contributor Author

coollog commented Sep 20, 2018

Will merge master after #1019 to fix the failing tests.

@coollog coollog requested a review from a team September 20, 2018 21:03
@coollog coollog merged commit 0998f24 into master Sep 21, 2018
@coollog coollog deleted the additional-tags-gradle branch September 21, 2018 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants