-
Notifications
You must be signed in to change notification settings - Fork 350
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
feat(#1656) : jib poc basic integration use case #4315
feat(#1656) : jib poc basic integration use case #4315
Conversation
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.
Overall it looks a great work, thanks!
@@ -240,6 +245,11 @@ func generateProjectStructure(context Context, project Project) error { | |||
return nil | |||
} | |||
|
|||
// Create a MAVEN_CONTEXT file containing all arguments for a maven command | |||
func generateMavenContext(context Context, args []string) error { | |||
return util.WriteToFile(filepath.Join(context.Path, "MAVEN_CONTEXT"), strings.Join(args, " ")) |
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.
What is this doing?
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.
In short : I write into a /tmp/kit-xxxxxx-yyyyy/maven/MAVEN_CONTEXT file all the parameters of the build maven command, and then I re-use them in the jib task. The way I see it is that we are doing maven command in both case, so any maven repository, or any other configuration used in the build context should work in the jib task.
But that's clearly the part that needs to be refined. I suspect I write too many arguments in the file. To illustrate this for better understanding that is why I need to remove the goal (package
) in the jib task.
} | ||
log.Info(string(mavenCommand)) | ||
|
||
// ugly replaces |
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.
Just use fmt.Sprintf
and I think should be fine.
Having a very first look at the smoke tests (common single op installation), it even seems faster than Spectrum (60 minutes vs 65/70 minutes)... |
Why not use the quarkus jib ? |
There was a full analysis in #1656 (comment) |
Implementation: "com.google.cloud.tools.jib.maven.extension.layerfilter.Configuration", | ||
Filters: v1.Filters{ | ||
Filter: v1.Filter{ | ||
Glob: "/app/**", |
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.
this creates a single layer right ?
One of the nice things about the quarkus jib extensions is that it automatically configure jib to have some layers according to the app layout the quarkus builder generates
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.
In short yes we have a single layer created by the configuration:
ExtraDirectories: v1.ExtraDirectories{
Paths: v1.Paths{
Path: v1.Path{
From: "../context",
Into: "/deployments",
},
},
},
The Filter code remove the default layers created by the google jib plugin done into the app
folder by default.
I agree with you that the quarkus jib extension creates good layers according to the quarkus build, but sadly it can't make any incremental build. If you know a way to have actual incremental layers please don't hesitate. As it is the fact that the quarkus extension does not offer the possibility to use another folder that the default 'target' creates 2 full layers for libs that double the size of these dependencies in the final image.
I will see if I can make a better set of layers with the ExtraDirectories
plugin configuration, that is a good idea.
* Add publish jib strategy * Use google jib plugin
57df949
to
95c89c3
Compare
Closing for now in favor of #4545 |
This is only a basic POC. I want to run the e2e tests on it.
I will close this PR after.