Implement `register-buildpack` for github type #716

elbandito · 2020-06-29T20:12:44Z

Summary

**NOTE: In the RFC, there is mention of 2 types to support: github and git. This PR is ONLY for supporting github.

This PR adds functionality to the pack CLI to support registering a buildpack with github. A new command (register-buildpack) was introduced (hidden behind the experimental flag)

Register the buildpack to a registry

Usage:
  pack register-buildpack <image> [flags]

Flags:
  -R, --buildpack-registry string   Buildpack Registry reference
  -h, --help                        Help for 'register-buildpack'

Global Flags:
      --no-color     Disable color output
  -q, --quiet        Show less output
      --timestamps   Enable timestamps in output
  -v, --verbose      Show more output

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

For more details:
https://github.com/buildpacks/rfcs/blob/main/text/0022-client-side-buildpack-registry.md
buildpacks/rfcs#75

Documentation

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

This should probably have an acceptance test as well, given that it's a new command
There are quite a few sad paths (particularly in register_buildpack) that aren't tested, and should be simple to exercise
It would be nice if more of the errors were wrapped errors.Wrap to provide a bit more context to users in sad paths.
I assume one of the maintainers will need to create https://github.com/buildpacks/registry for this to work/test locally, which may be helpful.

internal/commands/register_buildpack_test.go

dfreilich · 2020-07-01T00:13:01Z

internal/commands/register_buildpack.go

+			return nil
+		}),
+	}
+	cmd.Flags().StringVarP(&flags.BuildpackRegistry, "buildpack-registry", "R", "", "Buildpack Registry name")


Can we use r instead of R? Seems like we only use uppercase shortened flags by pack build, when we have no other choice

@dfreilich I'm in support of following conventions, so happy to make the change if that is the case. 👍

@dfreilich I don't think that's the ongoing rule. At least during the initial registry PR I proposed capital R because:

A --buildpack-registry feels like it's of the same importance as --builder.

#540 (review)

I understand the importance, but as a cli user, I would find it annoying to capitalize something (a whole extra button press) which is supposed to make it easier; I find capitalized arguments to be more annoying to remember/use.

I thought the use of -B by pack build was more because we needed some way of distinguishing --buildpack from --builder, rather than to underscore the importance of the argument; and if so, we should follow that pattern in other commands (like create-builder, which only has lower case shortened ones)

@dfreilich I see your point. at the very minimum I would think we can agree that both build and register-buildpack should both use the same flag casing.

@jromero @dfreilich so what's the take away here? Do we want to change -R to be -r?

I'd still vote for -r over -R

internal/commands/register_buildpack.go

internal/config/config_test.go

register_buildpack.go

register_buildpack_test.go

elbandito · 2020-07-02T16:44:50Z

Overall, it's definitely looking good. Some broader suggestions:

This should probably have an acceptance test as well, given that it's a new command

There are quite a few sad paths (particularly in register_buildpack) that aren't tested, and should be simple to exercise

It would be nice if more of the errors were wrapped errors.Wrap to provide a bit more context to users in sad paths.

I assume one of the maintainers will need to create https://github.com/buildpacks/registry for this to work/test locally, which may be helpful.

@dfreilich Thanks again for the feedback!

As far as acceptance tests are concerned, I don't think it should be considered a blocker. Most of the logic for this command is centered on the ability to open a github issue via a browser. There's some parsing that takes place from the config.toml file, but we should be able to cover most of this surface area with unit tests. I did add more tests, but if there is anything else that I'm missing, please let me know. Perhaps later on we can look at doing something fancy with a headless browser to test the ability to open a new github issue.

Co-authored-by: Joe Kutner <jpkutner@gmail.com> Signed-off-by: Travis Longoria <longoria.public@gmail.com>

codecov · 2020-07-02T22:12:08Z

Codecov Report

Merging #716 into main will increase coverage by 1.01%.
The diff coverage is 82.14%.

@@            Coverage Diff             @@
##             main     #716      +/-   ##
==========================================
+ Coverage   72.51%   73.52%   +1.01%     
==========================================
  Files          67       75       +8     
  Lines        4896     5135     +239     
==========================================
+ Hits         3550     3775     +225     
- Misses       1023     1043      +20     
+ Partials      323      317       -6

Flag	Coverage Δ
#os_linux	`76.02% <83.74%> (+0.88%)`	⬆️
#os_macos	`71.91% <71.96%> (+0.63%)`	⬆️
#os_windows	`71.83% <72.16%> (+0.73%)`	⬆️
#unit	`73.52% <82.14%> (+1.01%)`	⬆️

elbandito · 2020-07-06T17:25:50Z

@jromero and @dfreilich are there any more action items on this PR that you'd like resolved?

dfreilich

overall LGTM!

register_buildpack_test.go

dfreilich · 2020-07-06T19:30:57Z

internal/commands/register_buildpack.go

+			return nil
+		}),
+	}
+	cmd.Flags().StringVarP(&flags.BuildpackRegistry, "buildpack-registry", "R", "", "Buildpack Registry name")


I'd still vote for -r over -R

register_buildpack.go

elbandito · 2020-07-08T00:36:45Z

@jromero any final thoughts on this?

jromero

This is looking great! I've marked a few requested changes. Feel free to reach out via slack or other ways if something is unclear.

internal/config/config.go

register_buildpack.go

register_buildpack_test.go

register_buildpack.go

register_buildpack_test.go

cmd/cmd.go

Signed-off-by: Travis Longoria <longoria.public@gmail.com>

Signed-off-by: Travis <longoria.public@gmail.com>

jromero · 2020-07-12T21:16:14Z

UA 1: TOML data format

$ ./out/pack register-buildpack cnbs/sample-package:hello-universe
Successfully registered cnbs/sample-package:hello-universe

Feedback

It looks like the output doesn't match exactly what the RFC proposes. There is a missing field for the digest whereas this PR has the digest included as part of the addr. I can't really see the benefit of one vs the other. @jkutner do you have some insight about whether it's worth breaking out or not?

Error cases

$ ./out/pack register-buildpack cnbs/sample-package:no-exist
ERROR: image cnbs/sample-package:no-exist does not exist in registry: not found

$  ./out/pack register-buildpack gcr.io/cnbs/sample-package:no-exist
ERROR: connect to repo store 'gcr.io/cnbs/sample-package:no-exist': GET https://gcr.io/v2/token?scope=repository%3Acnbs%2Fsample-package%3Apull&service=gcr.io: UNKNOWN: Project 'project:cnbs' not found or deleted.

$ ./out/pack register-buildpack -r no-exist cnbs/sample-package:hello-universe
ERROR: registry no-exist is not defined in your config file

elbandito · 2020-07-13T15:23:51Z

@jromero I spoke with @jkutner and the intention is to have the digest as part of the addr field.
https://github.com/buildpacks/rfcs/blob/main/text/0032-update-json-cnb-registry.md

jromero · 2020-07-13T16:10:43Z

@jromero I spoke with @jkutner and the intention is to have the digest as part of the addr field.
https://github.com/buildpacks/rfcs/blob/main/text/0032-update-json-cnb-registry.md

Sounds good! ✨ Approved! ✨

elbandito force-pushed the register-buildpack branch 3 times, most recently from 2740165 to 7b4853b Compare June 30, 2020 23:40

elbandito marked this pull request as ready for review June 30, 2020 23:41

elbandito requested a review from a team as a code owner June 30, 2020 23:41

dfreilich requested changes Jul 1, 2020

View reviewed changes

elbandito force-pushed the register-buildpack branch 2 times, most recently from 484590a to d1a0ce4 Compare July 1, 2020 17:44

elbandito closed this Jul 1, 2020

elbandito reopened this Jul 1, 2020

dfreilich mentioned this pull request Jul 1, 2020

Add analyzer acceptance tests buildpacks/lifecycle#319

Merged

elbandito force-pushed the register-buildpack branch 2 times, most recently from f24e942 to d639776 Compare July 2, 2020 21:40

create register-buildpack command

83de8b5

Co-authored-by: Joe Kutner <jpkutner@gmail.com> Signed-off-by: Travis Longoria <longoria.public@gmail.com>

elbandito force-pushed the register-buildpack branch from d639776 to 83de8b5 Compare July 2, 2020 22:05

dfreilich approved these changes Jul 6, 2020

View reviewed changes

Merge branch 'main' into register-buildpack

0ae4930

elbandito force-pushed the register-buildpack branch from a2192f0 to 0ae4930 Compare July 8, 2020 00:02

jromero suggested changes Jul 8, 2020

View reviewed changes

jromero reviewed Jul 8, 2020

View reviewed changes

cmd/cmd.go Outdated Show resolved Hide resolved

clean-up registry-buildpack

3487ca7

Signed-off-by: Travis Longoria <longoria.public@gmail.com>

elbandito force-pushed the register-buildpack branch 4 times, most recently from aec5c7b to cd01c95 Compare July 10, 2020 01:09

Merge branch 'main' into register-buildpack

3854992

Signed-off-by: Travis <longoria.public@gmail.com>

elbandito force-pushed the register-buildpack branch from cd01c95 to 3854992 Compare July 10, 2020 01:17

jromero added this to the 0.13.0 milestone Jul 13, 2020

jromero added experimental Issue or PR refers to an experimental feature. type/enhancement Issue that requests a new feature or improvement. status/user-accepted Pull Request that has passed user acceptance. labels Jul 13, 2020

jromero approved these changes Jul 13, 2020

View reviewed changes

jromero merged commit 90e3412 into main Jul 13, 2020

jromero deleted the register-buildpack branch July 13, 2020 16:11

jromero changed the title ~~Register buildpack~~ Implement register-buildpack for github type Aug 18, 2020

jromero mentioned this pull request Sep 8, 2020

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

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `register-buildpack` for github type #716

Implement `register-buildpack` for github type #716

elbandito commented Jun 29, 2020 •

edited

dfreilich left a comment

dfreilich Jul 1, 2020

elbandito Jul 1, 2020

jromero Jul 1, 2020

dfreilich Jul 1, 2020

jromero Jul 1, 2020

elbandito Jul 2, 2020

dfreilich Jul 6, 2020

elbandito commented Jul 2, 2020

codecov bot commented Jul 2, 2020 •

edited

elbandito commented Jul 6, 2020

dfreilich left a comment

dfreilich Jul 6, 2020

elbandito commented Jul 8, 2020

jromero left a comment

jromero commented Jul 12, 2020

elbandito commented Jul 13, 2020

jromero commented Jul 13, 2020

Implement register-buildpack for github type #716

Implement register-buildpack for github type #716

Conversation

elbandito commented Jun 29, 2020 • edited

Summary

Documentation

Related

dfreilich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbandito commented Jul 2, 2020

codecov bot commented Jul 2, 2020 • edited

Codecov Report

elbandito commented Jul 6, 2020

dfreilich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbandito commented Jul 8, 2020

jromero left a comment

Choose a reason for hiding this comment

jromero commented Jul 12, 2020

UA 1: TOML data format

Feedback

Error cases

elbandito commented Jul 13, 2020

jromero commented Jul 13, 2020

Implement `register-buildpack` for github type #716

Implement `register-buildpack` for github type #716

elbandito commented Jun 29, 2020 •

edited

codecov bot commented Jul 2, 2020 •

edited