Add config trusted-builders subcommands #959
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dfreilich
force-pushed
the
921-config-t-builders
branch
2 times, most recently
from
d48010b to
b5162dc
Compare
jromero
suggested changes
Nov 17, 2020
internal/commands/config/config.go
Outdated
| "github.com/buildpacks/pack/logging" | ||
| ) | ||
|
|
||
| func Config(logger logging.Logger, cfg config.Config, cfgPath string) *cobra.Command { |
There was a problem hiding this comment.
Suggested change
| func Config(logger logging.Logger, cfg config.Config, cfgPath string) *cobra.Command { | |
| func NewConfigCommand(logger logging.Logger, cfg config.Config, cfgPath string) *cobra.Command { |
There was a problem hiding this comment.
I'm happy to change that, but in my initial comment
to the stack cmd PR (#937 (comment)), I had urged away from doing that.
Our command pattern is typically to do the command name itself (e.g. command.Build), so I think this would fit the pattern a bit more.
Just want to confirm whether this should be the new pattern. It does make more sense (because then it will be used as rootCmd.AddCommand(config.NewCommand(...))
dfreilich
added
the
type/enhancement
dfreilich
force-pushed
the
921-config-t-builders
branch
from
e0d9378 to
03975ae
Compare
1 task
dfreilich
force-pushed
the
921-config-t-builders
branch
3 times, most recently
from
2d18b9c to
8f17f9c
Compare
|
Blocked on #964 |
dfreilich
force-pushed
the
921-config-t-builders
branch
from
8f17f9c to
8d10749
Compare
Codecov Report
@@ Coverage Diff @@
## main #959 +/- ##
==========================================
+ Coverage 77.63% 77.92% +0.29%
==========================================
Files 105 107 +2
Lines 5060 5116 +56
==========================================
+ Hits 3928 3986 +58
+ Misses 731 730 -1
+ Partials 401 400 -1
Flags with carried forward coverage won't be shown. Click here to find out more. |
dfreilich
force-pushed
the
921-config-t-builders
branch
from
8d10749 to
9023079
Compare
dfreilich
changed the title
dfreilich
commented
Dec 1, 2020
| } | ||
|
|
||
| // Technically the creator is supported as of platform API version 0.3 (lifecycle version 0.7.0+) but earlier versions | ||
| // have bugs that make using the creator problematic. | ||
| creatorSupported := lifecycle.SupportsFeature(config.CreatorInLifecycle) && | ||
| pack.SupportsFeature(invoke.CreatorInPack) | ||
|
|
||
| usingCreator = creatorSupported && trustBuilder | ||
| usingCreator = creatorSupported |
There was a problem hiding this comment.
trust-builder is supported by all versions tested
dfreilich
force-pushed
the
921-config-t-builders
branch
from
9023079 to
48483b6
Compare
…ng to subcommands Signed-off-by: David Freilich <dfreilich@vmware.com>
Signed-off-by: David Freilich <dfreilich@vmware.com>
* Create config command generators, and replace list-trusted/trust/untrust builder funcs Signed-off-by: David Freilich <dfreilich@vmware.com>
…mands Signed-off-by: David Freilich <dfreilich@vmware.com>
* Add acceptance test for deprecation warning * Fix linting errors * Add command to rootCmd Signed-off-by: David Freilich <dfreilich@vmware.com>
Signed-off-by: David Freilich <dfreilich@vmware.com>
command Signed-off-by: David Freilich <dfreilich@vmware.com> Signed-off-by: David Freilich <dfreilich@vmware.com>
* Rename trusted-builder to trusted-builders, to follow RFC * Change Config to NewConfigCommand Signed-off-by: David Freilich <dfreilich@vmware.com>
Signed-off-by: David Freilich <dfreilich@vmware.com>
dfreilich
force-pushed
the
921-config-t-builders
branch
from
48483b6 to
c67b0fb
Compare
jromero
reviewed
Dec 3, 2020
| @@ -73,13 +72,17 @@ func NewPackCommand(logger ConfigurableLogger) (*cobra.Command, error) { | |||
| rootCmd.AddCommand(commands.SetDefaultBuilder(logger, cfg, &packClient)) | |||
| rootCmd.AddCommand(commands.InspectBuilder(logger, cfg, &packClient, writer.NewFactory())) | |||
| rootCmd.AddCommand(commands.SuggestBuilders(logger, &packClient)) | |||
| //nolint:staticcheck | |||
There was a problem hiding this comment.
Out of curiosity, what lint warning are we suppressing?
There was a problem hiding this comment.
Using a deprecated command.
jromero
reviewed
Dec 3, 2020
✅ Accepted |
jromero
added
the
status/user-accepted
jromero
approved these changes
Dec 3, 2020
Signed-off-by: David Freilich <dfreilich@vmware.com> Co-authored-by: Javier Romero <rjavier@vmware.com>
Signed-off-by: David Freilich <dfreilich@vmware.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds 4
pack config trusted-builderssubcommands:pack config trusted-builders→ Lists trusted builderspack config trusted-builders listpack config trusted-builders add <builder>pack config trusted-builders remove <builder>As part of it, it also hid the commands
pack trust-builder,pack untrust-builder, andpack list-trusted-builders, and aliased them to the respective new commands.In order to accomplish this cleanly, I also made some of the funcs/items in the
commandspackage public, which changed a fair number of files.Output
Before
After
Documentation
Related
Resolves #921
Relates to (and partially accomplishes) #597