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

[Jib CLI WAR] Implement exploded mode for standard WAR #3226

Merged
merged 6 commits into from
May 7, 2021

Conversation

mpeddada1
Copy link
Contributor

Major Changes:

  • Implemented StandardWarExplodedProcessor
  • Renamed JarProcessor to ArtifactProcessor (is used by both jar processors and war processors)
  • Renamed JarProcessors to ArtifactProcessors. It will eventually have two methods, fromJar(already implemented) and fromWar, which will contain logic to select mode processors for JARs and WARs respectively.
  • Moved common code from JarLayers to ArtifactLayers (to be used by both jar and war processors)

@google-cla google-cla bot added the cla: yes label Apr 30, 2021
@mpeddada1 mpeddada1 requested a review from a team April 30, 2021 20:05
@mpeddada1 mpeddada1 marked this pull request as ready for review April 30, 2021 20:05
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.

Looks good to me.

* @param warPath path to WAR file
* @param targetExplodedWarRoot path to exploded-war root
* @param warJavaVersion war java version
* @param appRoot the absolute path of the app on the container
Copy link
Member

Choose a reason for hiding this comment

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

We don't provide the --appRoot option yet, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good question. Nope, we don't provide the --appRoot option yet, it will be added when War.class is implemented. This class will only accept appRoot as an input but the idea is to set it in ArtifactProcessors. If a base image is specified then the user is expected to provide the appRoot (otherwise we throw an exception). If a base image is not specified then we create StandardWarExplodedProcessor with the default jetty app root.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good plan.

If a base image is specified then the user is expected to provide the appRoot (otherwise we throw an exception).

Maybe throw an exception only when it's not the jetty on Docker Hub. I imagine often you set a base image only to use a tag or digest for jetty, which was the case in #3204.

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 is a great point! Will have to keep this in mind when implementing ArtifactProcessors

* @param warPath path to WAR file
* @param targetExplodedWarRoot path to exploded-war root
* @param warJavaVersion war java version
* @param appRoot the absolute path of the app on the container
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good plan.

If a base image is specified then the user is expected to provide the appRoot (otherwise we throw an exception).

Maybe throw an exception only when it's not the jetty on Docker Hub. I imagine often you set a base image only to use a tag or digest for jetty, which was the case in #3204.

@mpeddada1 mpeddada1 merged commit d5da6c5 into master May 7, 2021
@mpeddada1 mpeddada1 deleted the cli-war-exploded branch May 7, 2021 21:46
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.

2 participants