update(uri): support for .cnb files in dependencies#420
update(uri): support for .cnb files in dependencies#420till wants to merge 2 commits intopaketo-buildpacks:mainfrom
Conversation
|
|
54c180e to
1590fdc
Compare
|
Hello @till ! Is on my todo list for reviewing it. I just need to find some time :S |
pacostas
left a comment
There was a problem hiding this comment.
Nice contribution! Could please rebase your branch and adjust your code to match the latest changes as the --no-cnb-registry flag has been deprecated.
I have also added two comments. Feel free to change your code accordingly or not as is mostly aesthetics :)
|
@till Ping |
d022aef to
991d4dc
Compare
991d4dc to
cd4de74
Compare
| @@ -37,16 +37,25 @@ func (d *PackageConfigDependency) UnmarshalTOML(v interface{}) error { | |||
| } | |||
|
|
|||
| if d.URI != "" { | |||
There was a problem hiding this comment.
Can you also please add some tests for covering the changes on the package_config file?
73db74b to
3f1e867
Compare
|
|
||
| d.URI = strings.TrimPrefix(uri.String(), "//") | ||
| // Static URIs with http/https scheme are left unchanged | ||
| if uri.Scheme == "http" || uri.Scheme == "https" { |
There was a problem hiding this comment.
I think this is the only change. And it should be covered by the integration test?
| } | ||
|
|
||
| if d.URI != "" { | ||
| if !strings.HasPrefix(d.URI, "urn:cnb:registry") { |
There was a problem hiding this comment.
This just removes the if wrapping and exists earlier.
|
@pacostas Can you add the label? I think minor is fine. |
Adds support for .cnb files and directories for dependencies, leaving them untouched during the update process. Supports cnb files from a local paths and from a http/https server alike. Extracted some of the check code (for suffix/prefix) into the uri file to be able to reuse it and make it a bit more readable (hopefully). LBNL, update the parser to keep http/https scheme around so the uri in the dependencies doesn't get the scheme stripped. Resolves: paketo-buildpacks#358
3f1e867 to
3eefac2
Compare
|
|
||
| uri.Scheme = "" | ||
| // Parse the URI to check its scheme | ||
| uri, err := url.Parse(d.URI) |
There was a problem hiding this comment.
This will not work, in case you have a ./hello/world.tgz
| for i, dependency := range config.Dependencies { | ||
| if !strings.HasPrefix(dependency.URI, "docker://") && !strings.HasPrefix(dependency.URI, "urn:cnb:registry") { | ||
| // Skip static URIs (local paths, HTTP URLs, etc.) - leave them unchanged | ||
| if IsStaticURI(dependency) { |
There was a problem hiding this comment.
This will not get inside the if statement, in case you have a ./hello/world.tgz uri
|
|
||
| d.URI = strings.TrimPrefix(uri.String(), "//") | ||
| // Static URIs with http/https scheme are left unchanged | ||
| if uri.Scheme == "http" || uri.Scheme == "https" { |
There was a problem hiding this comment.
I think here you can use the buildpack.GetLocatorType function
| if uri.Scheme == "http" || uri.Scheme == "https" { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
On this function, somewhere we should validate that local files that are referenced on the package.toml, actually exists locally
| }) | ||
| }) | ||
|
|
||
| context("when dependencies include .cnb archives", func() { |
There was a problem hiding this comment.
It seems like we dont need a new section just for the .cnb archives. Can you add the .cnb testing scenarios in an existing integration test? It would be nice not to increase the amount of integration tests if is not necessary.
| expected: true, | ||
| }, | ||
| { | ||
| name: "non-existent relative path", |
There was a problem hiding this comment.
I think this one is failing, not due to the file path does not exist, but because the getlocator function can not recognize it as a buildpack.URILocator. Extra code is necessary to ensure that is a file path.
| expected: true, | ||
| }, | ||
| { | ||
| name: "non-existent relative path", |
There was a problem hiding this comment.
Can you also add a scenario when the file exists?
| expected: false, | ||
| }, | ||
| { | ||
| name: "non-existent absolute path", |
There was a problem hiding this comment.
I think this one is failing, not due to the file path does not exist, but because the getlocator function can not recognize it as a buildpack.URILocator. Extra code is necessary to ensure that is a file path.
| expected: false, | ||
| }, | ||
| { | ||
| name: "non-existent absolute path", |
There was a problem hiding this comment.
Can you also add a scenario when the file exists?
Summary
Adds support for .cnb files for dependencies, leaving them untouched
during an update process. Supports both local paths, but an archive
from a http/https server alike.
I extracted the code from commands into its own file, to be able to
add a bit of test coverage.
Also extracted some of the check code (for suffix/prefix) into the uri
file to be able to reuse it and make it a bit more readable (hopefully).
LBNL, update the parser to keep http/https scheme around so the uri in
the dependencies doesn't get the scheme stripped.
Resolves: #358
Use Cases
A cnb file in the local file system or on a remote http/https server.
Checklist