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

Fix registry selector flag in build command and validation for the input #832

Merged
merged 4 commits into from Sep 30, 2020

Conversation

supra08
Copy link
Contributor

@supra08 supra08 commented Sep 4, 2020

Signed-off-by: Supratik Das rick.das08@gmail.com

Summary

This PR attempts to fix #747. It replaces the -R flag for selecting the registry in pack build to -r. It also puts a validation for rejecting git/https based URLs.

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #747

@supra08 supra08 requested a review from a team as a code owner September 4, 2020 23:28
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #832 into main will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #832      +/-   ##
==========================================
- Coverage   76.34%   76.23%   -0.11%     
==========================================
  Files          86       86              
  Lines        4416     4429      +13     
==========================================
+ Hits         3371     3376       +5     
- Misses        700      704       +4     
- Partials      345      349       +4     
Flag Coverage Δ
#os_linux 76.23% <ø> (-0.11%) ⬇️
#os_macos 72.82% <ø> (-0.10%) ⬇️
#os_windows 100.00% <ø> (ø)
#unit 76.23% <ø> (-0.11%) ⬇️

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

Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

Thanks for the additions! Would you be able to add tests, verifying that the regex works as expected?

build.go Outdated
Comment on lines 650 to 654
r := regexp.MustCompile(`^(https|git)(://|@)([^/:]+)[/:]([^/:]+)/(.+).git$`)
if r.MatchString(registry) {
return fetchedBPs, order, errors.New("registry should be only specified as named")
}

registryCache, err := c.getRegistry(c.logger, registry)
Copy link
Member

Choose a reason for hiding this comment

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

There may be some confusion on the original requirements. The flag should no longer accept a URI at all and instead only support names from the config.toml files.

This is taken from the latest PR changes:

By default, if no configuration values are defined in the config.toml file, buildpacks will be registered against:
https://github.com/buildpacks/registry. In addition, user can specify their own registries. For example:

[[registries]]
name="elbandito"
type="github"
url="https://github.com/elbandito/buildpack-registry"

[[registries]]
name="joe"
type="github"
url="https://github.com/jkutner/buildpack-registry/"

When there are multiple registries, the user can specify a specific registry via buildpack-registry, -R flag.

For example:
pack register-buildpack <buildpack/image> -r elbandito

A possible solution may be to change c.getRegistry to accept a "name" and then it can then look up the URL from the config.toml instead of it being passed in.

internal/commands/build.go Outdated Show resolved Hide resolved
@jromero
Copy link
Member

jromero commented Sep 17, 2020

Hi @supra08, just checking in. Is there anything I can clarify or you need from me/us?

@supra08
Copy link
Contributor Author

supra08 commented Sep 17, 2020

Oh hi @jromero, no there is nothing as of now. I was stuck with some university formalities so the delay. It will be good to go in a couple of days.

@supra08
Copy link
Contributor Author

supra08 commented Sep 30, 2020

One of the unit tests is failing, working on it.

@jromero jromero added experimental Issue or PR refers to an experimental feature. type/enhancement Issue that requests a new feature or improvement. labels Sep 30, 2020
Signed-off-by: Supratik Das <rick.das08@gmail.com>
Signed-off-by: Supratik Das <rick.das08@gmail.com>
Signed-off-by: Supratik Das <rick.das08@gmail.com>
Signed-off-by: Supratik Das <rick.das08@gmail.com>
@jromero
Copy link
Member

jromero commented Sep 30, 2020

✔️ Accepted

NOTE: Errors are expected since no publically accessible image is available on the registry yet.

$ cat ~/.pack/config.toml
default-registry = "official"
experimental = true

[[registries]]
  name = "my-registry-1"
  type = "github"
  url = "https://example.com/1"

[[registries]]
  name = "my-registry-2"
  type = "github"
  url = "https://example.com/2"

[[registries]]
  name = "a-registry-1"
  type = "github"
  url = "https://example.com/1"

$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b test/node-function
...
Status: Downloaded newer image for heroku/pack:18
ERROR: failed to build: creating from buildpackage test/node-function: fetching image: image test/node-function does not exist on the daemon: not found

$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b test/node-function@0.6.2
...
Status: Image is up to date for heroku/pack:18
ERROR: failed to build: invalid buildpack string test/node-function@0.6.2

$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b urn:cnb:registry:test/node-function@0.6.2
...
Status: Image is up to date for heroku/pack:18
ERROR: failed to build: extracting from registry urn:cnb:registry:test/node-function@0.6.2: fetching image: connect to repo store 'gcr.io/test/node-function@sha256:9d88250dfd77dbf5a535f1358c6a05dc2c0d3a22defbdcd72bb8f5e24b84e21d': GET https://gcr.io/v2/token?scope=repository%3Atest%2Fnode-function%3Apull&service=gcr.io: UNKNOWN: Project 'project:test' not found or deleted.

$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b urn:cnb:registry:test/node-function
...
Status: Image is up to date for heroku/pack:18
ERROR: failed to build: extracting from registry urn:cnb:registry:test/node-function: fetching image: connect to repo store 'gcr.io/test/node-function@sha256:9d88250dfd77dbf5a535f1358c6a05dc2c0d3a22defbdcd72bb8f5e24b84e21d': GET https://gcr.io/v2/token?scope=repository%3Atest%2Fnode-function%3Apull&service=gcr.io: UNKNOWN: Project 'project:test' not found or deleted.

$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b urn:cnb:registry:test/node-function -r my-registry-1
...
Status: Image is up to date for heroku/pack:18
ERROR: failed to build: locating in registry urn:cnb:registry:test/node-function: refreshing cache: initializing (/Users/javier.romero/.pack/registry-f2f9784142e4d11e32b550593e53fa187718c7c3435936d7845bf0300d28391c): creating registry cache: cloning remote registry: repository not found

$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b urn:cnb:registry:test/node-function -r unset-registry
...
Status: Image is up to date for heroku/pack:18
ERROR: failed to build: invalid registry 'unset-registry': registry unset-registry is not defined in your config file

$ vim ~/.pack/config.toml
$ cat ~/.pack/config.toml
default-registry = "my-registry-1"
experimental = true

[[registries]]
  name = "my-registry-1"
  type = "github"
  url = "https://example.com/1"

[[registries]]
  name = "my-registry-2"
  type = "github"
  url = "https://example.com/2"

[[registries]]
  name = "a-registry-1"
  type = "github"
  url = "https://example.com/1"


$ ./out/pack build my-app -p ../samples/apps/bash-script/ -B heroku/buildpacks:18 -b urn:cnb:registry:test/node-function
...
Status: Image is up to date for heroku/pack:18
ERROR: failed to build: locating in registry urn:cnb:registry:test/node-function: refreshing cache: initializing (/Users/javier.romero/.pack/registry-f2f9784142e4d11e32b550593e53fa187718c7c3435936d7845bf0300d28391c): creating registry cache: cloning remote registry: repository not found

@jromero jromero added this to the 0.14.0 milestone Sep 30, 2020
@jromero jromero changed the base branch from main to release/0.14.0 September 30, 2020 22:22
@jromero
Copy link
Member

jromero commented Sep 30, 2020

NOTE: Documentation deferred until the buildpacks registry workflow is more complete.

@jromero jromero merged commit a0a6e81 into buildpacks:release/0.14.0 Sep 30, 2020
@jromero
Copy link
Member

jromero commented Sep 30, 2020

A huge thanks @supra08! 👏 👏 👏 Thank you for putting in the work and coordinating with us in order to be able to 🚢 this today.

@elbandito
Copy link
Member

Good job getting this over the finish line @supra08! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issue or PR refers to an experimental feature. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update pack build -R to use named buildpack registries
4 participants