-
Notifications
You must be signed in to change notification settings - Fork 7
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
Updates to support Platform Spec 0.4-0.12 #59
Conversation
Heads up that the integration tests fail, because they are using jbang to run against the artifact in maven central not the artifact built by this build, so they still point at the old version, and thus cannot work.. Safe to ignore for this commit, but will need updating once the artifact is built & published to maven central |
Slowly working through testing with Podman .. Rootless podman:
Rootful podman:
|
Github flows are failing. Can you fix them ? @BarDweller |
|
So I can then review it ? @BarDweller |
Added 0a7fbbd which is a fairly decent update to allow this to work with podman rootless. This PR is now ready for review. |
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.
Can you also update the github flow to test the quarkus example using our ubi builder image ?
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.
Can you also update the github flow to test the quarkus example using our ubi builder image ?
Signed-off-by: Ozzy Osborne <[email protected]>
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 a great PR :-) Well done !
README.md
Outdated
|
||
## FAQ: | ||
|
||
**Will this work with Podman?:** | ||
|
||
Not yet. Once the regular `pack` cli works with Podman, I'll revisit this and ensure it works too. | ||
Yes, tested with podman on fedora, rootless and rootful. |
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.
Should we here define the podman version ?
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.
Will do, although it may work with earlier revisions, it should at least work with the level I've tested and onwards
int uid = (Integer)Files.getAttribute(Paths.get("/proc/self"), "unix:uid"); | ||
File podmanUserSock = new File("/var/run/user/"+uid+"/podman/podman.sock"); | ||
if(podmanUserSock.exists()){ | ||
return "unix:///var/run/user/1000/podman/podman.sock"; | ||
} |
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.
Do I miss something as it seems that you return unix:///var/run/user/1000/podman/podman.sock
even if a different uid has been passed ?
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.
Good catch, this was a dev time hack that accidently made it into the commit at some point.. I'll fix this.
client/src/main/java/dev/snowdrop/buildpack/lifecycle/LifecycleExecutor.java
Show resolved
Hide resolved
I compiled the PR locally and got this error using JDK 21.x
The project is using a old java JDK
We should certainly bump it. WDYT ? |
The main README explaining how to use
should also explain what to do when the image has been built
|
Why don't we mention end of the building process of a sample that an image has been built ?
|
The process to build the sample
|
iirc, 1.8 was being used to ensure the created library would be consumable by projects running on java 8 jvms.. I'd need to dig to look into why, but that isn't something that will be addressed in this PR. |
There is no application to access.. the spring boot code contains no resources for you to access, and never has. It has only ever been there to test that the library can build the application successfully. |
The readme is talking about how to run the sample of code provided, and links to the samples by way for further examples of using jbang.. in the case of the supplied code fragment.. we cannot tell someone how to run an image built from code we do not know, and the intent & meaning for this part of the README has not been changed by this PR. If you think this needs addressing, please raise an Issue. |
Because we never have tested this, and this PR doesn't alter that. The library returns an exit code, and really, the pack.java is just there to check it all works. The library should not be mentioning anything, you are viewing the trace logs & info logs output by the lib, which confirm it did its job correctly.. the exit code tells the caller if it worked, and its up to the caller to decide what to do with that status.. in this case, the caller is pack.java, which takes that exit code and makes it its own, which should cause the integration test to fail if the library invocation failed. If you want to raise an Issue to make the sample pack.java also output human friendly messages, go ahead.. |
Ok. So we will in a separate PR improve the Spring Boot example or add a README to mention what is the purpose of this sample then |
Will do that :-) |
Tests are green, review comments have been met, and @cmoulliard has indicated via zulip that this is good to merge.. |
This PR represents a significant upgrade to the java buildpack client, which previously claimed support only for version 0.4 of the platform spec. This meant only builders that claimed compatibility with version 0.4 of the spec would run as expected. The spec has continued to evolve, most notably adding build image extension (0.10) and run image extension (0.12), and there are public builders (Paketo UBI Builder) using that spec, that will be unable to execute as intended with a 0.4 platform implementation.
An attempt has been made in this PR to tidy up the old code, splitting large executions across multiple classes, this included the configuration interface, which has been broken down into smaller composable units. As such this PR represents a breaking change to the api. We haven't released 1.0 yet, so this should be ok, and migration is relatively simple.
New to this version:
No support is currently planned for oci image layout (platform 0.12) or cache images.. which are implementation options for various revisions of the platform spec. Both are incompatible with extensions, and a primary goal for this updates was to ensure this library could build with builder images making use of extensions.