-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update to latest Snowdrop Buildpack Platform Impl #41936
Conversation
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
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.
Thanks! I added a small minor comment, either for this time or next depending on how the CI goes.
@ConfigItem(defaultValue = "paketocommunity/builder-ubi-base") | ||
public Optional<String> jvmBuilderImage; |
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.
Given you provided a default value, you should drop the Optional
.
Same for the one just below.
It's not mandatory so I suppose we can live with it if you don't have to push other changes but if CI doesn't pass and you do, please drop the Optional. Thanks!
This comment has been minimized.
This comment has been minimized.
What's the status of this? |
I'm removing the optional, and also performing a few more tests locally with native builds, that have thrown a curveball.. I'll add the PR for the Optional & removal of the native builder default.. There will probably be a new PR for the native changes |
Summary of findings from post-review comments...
Quarkus native builder image can be constructed relatively simply, although it ends up as a builder per java version, need to look into if the github project that publishes the quarkus mandrel images could also do buildpack builder images.. Quarkus buildpack also needs an update, as it's using deprecated property names.. raised paketo-buildpacks/quarkus#83 and will help get that updated. |
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.
LGTM
@iocanel mind taking a look as well? |
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.
LGTM
|
||
/** | ||
* The buildpacks builder image to use when building the project in jvm mode. | ||
* The buildpacks builder image to use when building the project in native mode.pu |
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.
typo mode.pu
?
This comment has been minimized.
This comment has been minimized.
Please squash the commits |
if (buildpackConfig.dockerHost.isPresent()) { | ||
log.info("Using DockerHost of " + buildpackConfig.dockerHost.get()); | ||
b.withDockerHost(buildpackConfig.dockerHost.get()); | ||
b.editDockerConfig().withDockerHost(buildpackConfig.dockerHost.get()); |
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.
b.editDockerConfig().withDockerHost(buildpackConfig.dockerHost.get()); | |
b.editDockerConfig().withDockerHost(buildpackConfig.dockerHost.get()).endDockerConfig(); |
}).build(); | ||
if (buildpackConfig.trustBuilderImage.isPresent()) { | ||
log.info("Setting trusted image to " + buildpackConfig.trustBuilderImage.get()); | ||
b.editPlatformConfig().withTrustBuilder(buildpackConfig.trustBuilderImage.get()); |
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.
b.editPlatformConfig().withTrustBuilder(buildpackConfig.trustBuilderImage.get()); | |
b.editPlatformConfig().withTrustBuilder(buildpackConfig.trustBuilderImage.get()).endPlatformConfig(); |
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.
There is a minor issue with the the use of editXXX
as its seems that the endXXX
is missing. The intention is that the actual edited value is set when you actually call the endXXX
method.
For example:
ObjectMetaBuilder builder;
public N endMetadata() {
return (N) MyFluent.this.withMetadata(builder.build());
}
Updating to add endXXXConfig and rerunning local tests.. will squash merge when done |
3ee34fa
to
846d887
Compare
Updated, found issue with snowdrop library not honoring docker pull timeout/timeout increase amounts, fixed library, updated quarkus usage.. tested as functional. |
What is the status ? @BarDweller |
Just needs a small git tidy up so it can be re-reviewed/merged.. (merge commit snuck in when I updated to latest branch.. need to rebase & force push to sort it out.. will be done by eod) |
c3d52ab
to
c18c4b2
Compare
c18c4b2
to
e83abe0
Compare
🎊 PR Preview bddf9a3 has been successfully built and deployed to https://quarkus-pr-main-41936-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
Status for workflow
|
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.
LGTM
Significant update to the buildpack platform implementation used by the buildpack container image extension.
Updates from snowdrop lib 0.0.6 to 0.0.10 bringing support for builder images using image extensions, and updating the default builder to be the paketo ubi builder image. Also contains updates for podman compatibility, image pull retry, and various other minor fixes.