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

Provide context directory for extensions #1276

Merged

Conversation

modulo11
Copy link
Contributor

Summary

This PR implements the extension layer RFC by providing one of the following context directory to the kaniko build during the extension phase:

  • $CNB_OUTPUT_DIR/context
  • $CNB_OUTPUT_DIR/context.build and/or $CNB_OUTPUT_DIR/context.run

More details and examples can be found in the RFC.

Release notes

Provide and allow extensions to write into a shared or specific context directory.


Related


Context

@loewenstein
Copy link

@natalieparellano once the spec changes are in, this would be worth it a look. Or already now, if there is review feedback that requires changes.

@natalieparellano
Copy link
Member

@modulo11 @loewenstein thank you for this - apologies for being slow to review the changes. It looks like there might be a couple of tests to fix, but I'll take a deeper look tomorrow to see if I could offer more useful feedback.

buildpack/buildcontext.go Outdated Show resolved Hide resolved
phase/extender.go Outdated Show resolved Hide resolved
phase/generator.go Outdated Show resolved Hide resolved
buildpack/generate.go Outdated Show resolved Hide resolved
phase/extender.go Show resolved Hide resolved
@loewenstein
Copy link

@modulo11 @loewenstein thank you for this - apologies for being slow to review the changes. It looks like there might be a couple of tests to fix, but I'll take a deeper look tomorrow to see if I could offer more useful feedback.

@modulo11 did we have a look for what causes the test failures?

@natalieparellano natalieparellano added this to the lifecycle 0.19.0 milestone Feb 7, 2024
@modulo11 modulo11 force-pushed the extension-contexts branch 2 times, most recently from cbc65a2 to 63b1b73 Compare February 9, 2024 07:59
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (be5e24b) 64.47% compared to head (ca4f6db) 64.65%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1276      +/-   ##
==========================================
+ Coverage   64.47%   64.65%   +0.18%     
==========================================
  Files         100      101       +1     
  Lines        6873     6949      +76     
==========================================
+ Hits         4431     4492      +61     
- Misses       2041     2051      +10     
- Partials      401      406       +5     
Flag Coverage Δ
os_linux 64.12% <79.35%> (+0.18%) ⬆️
os_windows 56.91% <28.27%> (-0.26%) ⬇️
unit 64.12% <79.35%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

phase/generator.go Outdated Show resolved Hide resolved
phase/generator.go Outdated Show resolved Hide resolved
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

@modulo11 thank you for this. In general this looks good. I still need to go through the unit tests a bit more carefully to understand what is tested.

It would be awesome if we could add an acceptance test (by adding a new fixture or symlinking to the child fixtures here). I actually think (without updating the fixtures) the current acceptance tests are going to start failing when we add 0.13 to the list of supported Platform APIs here. You could go ahead and do that as part of this PR if you want. When we add an (unreleased) API version we typically add a row to the table in the README, as shown in this PR.

LMK if I can be of any assistance here, especially with the acceptance test. Running our suite can be a bit tricky.

pbusko and others added 5 commits February 13, 2024 09:44
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
Co-authored-by: Pavel Busko <pavel.busko@sap.com>
Signed-off-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Signed-off-by: Pavel Busko <pavel.busko@sap.com>
@pbusko
Copy link
Contributor

pbusko commented Feb 13, 2024

@natalieparellano we've addressed all the comments and introduced additional acceptance test with the extensions contexts

buildpack/generate.go Outdated Show resolved Hide resolved
t.Fatalf("Incorrect error: %s\n", err)
}
})
when("using the platform API 0.13", func() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's worth having a whole separate block for the 0.13 tests - it looks like if we run the 0.12 block with generator.PlatformAPI = api.MustParse("0.13") the only two tests that fail are:

  • it("copies Dockerfiles and extend-config.toml files to the correct locations", func() {
  • descCondition: "a run.Dockerfile declares a new base image (only) and no run.Dockerfiles follow", descResult: "returns the referenced base image, and sets extend to false",

It might make our future lives easier to just conditionally set a variable for "dockerfile parent dir" depending on the Platform API and use that in the tests

phase/generator.go Outdated Show resolved Hide resolved
desc := func(b bool) string {
if b {
return "rebasable"
when("using platform API 0.13", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to #1276 (comment), I wonder if we could just make vars for "A build.Dockerfile path", "A build BuildContext" (etc) and make this more of a table test.

@natalieparellano
Copy link
Member

@pbusko @modulo11 thank you for this - I left a few comments but they are mostly nits about test organization. LMK what is your bandwidth for addressing these as it would be nice to cut an 0.19.0 RC soon. I'd be okay leaving #1276 (comment) and #1276 (comment) for later refactoring if we want to merge this one soon.

Signed-off-by: Philipp Stehle <philipp.stehle@sap.com>
@phil9909
Copy link
Contributor

@natalieparellano I addressed the two small comments.
On the tests: I have some ideas how to change them, but there is no obviously superior way of doing this. So I would prefer to do this in a follow-up PR as you suggested, so we can start a fresh discussion there. Maybe even have two competing PRs.

@natalieparellano natalieparellano merged commit 7bdfb33 into buildpacks:main Feb 15, 2024
10 checks passed
Copy link

@loewenstein loewenstein left a comment

Choose a reason for hiding this comment

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

LGTM

@pbusko pbusko deleted the extension-contexts branch February 15, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants