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

Add 'container' configuration object to wrap container-specific parameters #421

Merged
merged 11 commits into from
Jun 20, 2018

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Jun 19, 2018

jvmFlags, mainClass, args, and format configuration parameters are now nested inside a container object, much like image and credHelper being nested inside to and from. The original parameters are also now deprecated.

Fixes #384.

Copy link
Contributor

@coollog coollog left a comment

Choose a reason for hiding this comment

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

Perhaps we should not remove the original parameters for now and instead just annotate them with @Deprecated so that we can do a patch release (aka v0.9.1) with the other small fixes, and then remove the deprecated parameters when we do a minor release (aka v0.10.0).

@@ -69,7 +69,7 @@ public Blob getContainerConfigurationBlob() {
// Sets the entrypoint.
template.setContainerEntrypoint(image.getEntrypoint());

// Sets the entrypoint.
// Sets the main class arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

main method?

* A bean that configures properties of the container run from the image. This is configurable with
* Groovy closures and can be validated when used as a task input.
*/
public class ContainerConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ContainerParameters so that it's not confused with the container configuration that is part of the image format?

.setJavaArguments(jibExtension.getContainer().getArgs())
.setJvmFlags(jibExtension.getContainer().getJvmFlags())
.setTargetFormat(jibExtension.getContainer().getFormat())
.setJavaArguments(jibExtension.getArgs())
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be left as jibExtension.getContainer().getArgs() since jibExtension.getArgs() (and the like) will be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed jibExtension.getArgs() to return container.getArgs(), and my plan was to tag jibExtension.getArgs() with @Internal instead of removing it in the future. Sort of like how getTargetImage() and getBaseImage() are handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay that sounds good.

@TadCordle TadCordle merged commit 4077a22 into master Jun 20, 2018
@TadCordle TadCordle deleted the container_config_object branch June 20, 2018 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants