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

Update to latest Snowdrop Buildpack Platform Impl #41936

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BarDweller
Copy link
Contributor

@BarDweller BarDweller commented Jul 16, 2024

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.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 16, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with dot

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jul 16, 2024
@BarDweller BarDweller changed the title Update to latest Snowdrop Buildpack Platform Impl. Update to latest Snowdrop Buildpack Platform Impl Jul 16, 2024
@gastaldi gastaldi requested a review from geoand July 16, 2024 23:08
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 17, 2024
Copy link
Member

@gsmet gsmet left a 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.

Comment on lines 17 to 18
@ConfigItem(defaultValue = "paketocommunity/builder-ubi-base")
public Optional<String> jvmBuilderImage;
Copy link
Member

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!

@quarkus-bot

This comment has been minimized.

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 18, 2024
@geoand
Copy link
Contributor

geoand commented Jul 19, 2024

What's the status of this?

@BarDweller
Copy link
Contributor Author

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

@BarDweller
Copy link
Contributor Author

Summary of findings from post-review comments...

  • builder default removed for native build, the paketo ubi builder does not do native builds, there is currently no available builder to configure for quarkus native builds.
  • testing performed locally to validate that native builds were possible for quarkus using a custom builder image derived from the quarkus mandrel container.
  • testing uncovered issue in snowdrop lib related to builder images with unexpected entrypoint configurations. Snowdrop lib fixed, and re-released as 0.0.11
  • exposed a few more config options from the snowdrop lib via quarkus that were helpful during native testing.

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.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM

@geoand geoand requested a review from iocanel July 29, 2024 13:26
@geoand
Copy link
Contributor

geoand commented Jul 29, 2024

@iocanel mind taking a look as well?

Copy link
Contributor

@iocanel iocanel left a 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
Copy link
Member

Choose a reason for hiding this comment

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

typo mode.pu ?

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 29, 2024
@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jul 30, 2024

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());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
b.editPlatformConfig().withTrustBuilder(buildpackConfig.trustBuilderImage.get());
b.editPlatformConfig().withTrustBuilder(buildpackConfig.trustBuilderImage.get()).endPlatformConfig();

@iocanel iocanel self-requested a review July 30, 2024 07:43
Copy link
Contributor

@iocanel iocanel left a 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());
    }

@BarDweller
Copy link
Contributor Author

Updating to add endXXXConfig and rerunning local tests.. will squash merge when done

@BarDweller BarDweller force-pushed the buildpack-update branch 2 times, most recently from 3ee34fa to 846d887 Compare August 19, 2024 13:55
@BarDweller
Copy link
Contributor Author

Updated, found issue with snowdrop library not honoring docker pull timeout/timeout increase amounts, fixed library, updated quarkus usage.. tested as functional.

@cmoulliard
Copy link

What is the status ? @BarDweller

@BarDweller
Copy link
Contributor Author

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)

@BarDweller BarDweller force-pushed the buildpack-update branch 2 times, most recently from c3d52ab to c18c4b2 Compare September 19, 2024 14:51
Copy link

🎊 PR Preview bddf9a3 has been successfully built and deployed to https://quarkus-pr-main-41936-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 19, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit c18c4b2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/documentation triage/flaky-test triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
6 participants