add command `skaffold inspect build-env add local` #5944

gsquared94 · 2021-06-04T09:41:49Z

Fixes: #5998
Related: #5758
Description

User facing changes (remove if N/A)

❯ skaffold inspect build-env add local --help
Add a new local build environment definition.
Without the '--profile' flag the new environment definition is added to the default pipeline. With the '--profile' flag
it will create a new profile with this build env definition. 
In these respective scenarios, it will fail if the build env definition for the default pipeline or the named profile
already exists. To override an existing definition use 'skaffold inspect build-env modify' command instead. 
Use the '--module' filter to specify the individual module to target. Otherwise, it'll be applied to all modules defined
in the target file. Also, with the '--profile' flag if the target config imports other configs as dependencies, then the
new profile will be recursively created in all the imported configs also.

Examples:
  # Add a new profile named 'local' targeting the local build environment with option to push images and using buildkit
  skaffold inspect build-env add local --profile local --push true --useBuildkit true -f skaffold.yaml

Options:
      --concurrency=-1: number of artifacts to build concurrently. 0 means "no-limit"
      --push=: Set to true to push images to a registry
      --tryImportMissing=: Set to true to to attempt importing artifacts from Docker (either a local or remote registry)
if not in the build cache
      --useBuildkit=: Set to true to use BuildKit to build Docker images
      --useDockerCLI=: Set to true to use 'docker' command-line interface instead of Docker Engine APIs

Usage:
  skaffold inspect build-env add local [flags] [options]

Use "skaffold options" for a list of global command-line options (applies to all commands).

codecov · 2021-06-04T09:58:59Z

Codecov Report

Merging #5944 (d9f8aec) into master (9647c41) will decrease coverage by 0.00%.
The diff coverage is 69.01%.

@@            Coverage Diff             @@
##           master    #5944      +/-   ##
==========================================
- Coverage   70.66%   70.65%   -0.01%     
==========================================
  Files         454      455       +1     
  Lines       17403    17471      +68     
==========================================
+ Hits        12297    12344      +47     
- Misses       4197     4214      +17     
- Partials      909      913       +4

Impacted Files	Coverage Δ
pkg/skaffold/inspect/types.go	`100.00% <ø> (ø)`
cmd/skaffold/app/cmd/cmd.go	`68.39% <50.00%> (ø)`
cmd/skaffold/app/cmd/inspect_build_env.go	`55.83% <56.66%> (-0.22%)`	⬇️
pkg/skaffold/inspect/buildEnv/add_local.go	`79.48% <79.48%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9647c41...d9f8aec. Read the comment docs.

cmd/skaffold/app/cmd/cmd.go

briandealwis · 2021-06-04T20:28:19Z

cmd/skaffold/app/cmd/inspect_build_env.go

+		WithDescription("Add a new Local build environment definition").
+		WithLongDescription(`Add a new Local build environment definition.


Do we use camel case elsewhere? Maybe a single quote would be better 'local' to reflect what's actually added?

briandealwis · 2021-06-04T20:31:08Z

cmd/skaffold/app/cmd/inspect_build_env.go

+		WithLongDescription(`Add a new Local build environment definition.
+Without the '--profile' flag the new environment definition is added to the default pipeline. With the '--profile' flag it will create a new profile with this build env definition. 
+In these respective scenarios, it will fail if the build env definition for the default pipeline or the named profile already exists. To override an existing definition use 'skaffold inspect build-env modify' command instead. 
+Use the '--module' filter to specify the individual module to target. Otherwise, it'll be applied to all modules defined in the target file. Also, with the '--profile' flag if the target config imports other configs as dependencies, then the new profile will be recursively created in all the imported configs also.`).


The target file isn't actually defined — and there's no option to provide (--filename).

The recursive --profile behaviour is surprising and feels like it should be controlled with a --recursive flag?

Suggested change

Use the '--module' filter to specify the individual module to target. Otherwise, it'll be applied to all modules defined in the target file. Also, with the '--profile' flag if the target config imports other configs as dependencies, then the new profile will be recursively created in all the imported configs also.`).

Use the '--module' filter to target specific modules. Otherwise, it will be applied to all modules defined in the target file. Also, with the '--profile' flag if the target config imports other configs as dependencies, then the new profile will be recursively created in all the imported configs also.`).

There actually is a --filename flag. It seems like a bug (or I missed it) that flags added via cobra.Command.PersistentFlags don't show up in the --help text.

briandealwis · 2021-06-04T20:52:23Z

pkg/skaffold/inspect/buildEnv/add_local.go

+
+func AddLocalBuildEnv(ctx context.Context, out io.Writer, opts inspect.Options) error {
+	formatter := inspect.OutputFormatter(out, opts.OutFormat)
+	cfgs, err := inspect.ConfigSetFunc(config.SkaffoldOptions{ConfigurationFile: opts.Filename, ConfigurationFilter: opts.Modules, SkipConfigDefaults: true, MakePathsAbsolute: util.BoolPtr(false)})


ConfigSetFunc() seems an odd name. Shouldn't it be ConfigSet?

briandealwis · 2021-06-04T20:54:08Z

cmd/skaffold/app/cmd/inspect_build_env.go

+
+func localBuildEnvOptions() inspect.Options {
+	return inspect.Options{
+		Filename:  inspectFlags.fileName,


Capitalization should be consistent: n vs N

briandealwis · 2021-06-04T20:58:24Z

pkg/skaffold/inspect/buildEnv/add_local.go

+		cfgs = cfgs.SelectRootConfigs()
+		for _, cfg := range cfgs {
+			if cfg.Build.LocalBuild != nil && (*cfg.Build.LocalBuild != latestV1.LocalBuild{}) {
+				return formatter.WriteErr(inspect.BuildEnvAlreadyExists(inspect.BuildEnvs.Local, cfg.SourceFile, ""))


Huh, I thought To override an existing definition use 'skaffold inspect build-env modify' command instead meant if there were GCB or Cluster builds set.

gsquared94 · 2021-06-07T14:05:47Z

The auto-merge bot merged it before I could look at the review comments. I'll make the changes in a follow-up PR. Also the auto-merge should probably wait on unresolved comments.

add command skaffold inspect build-env add local

d9f8aec

gsquared94 requested a review from a team as a code owner June 4, 2021 09:41

gsquared94 requested a review from tejal29 June 4, 2021 09:41

google-cla bot added the cla: yes label Jun 4, 2021

pull-request-size bot added the size/L label Jun 4, 2021

gsquared94 enabled auto-merge (squash) June 4, 2021 09:43

gsquared94 requested a review from MarlonGamez June 4, 2021 09:44

briandealwis approved these changes Jun 4, 2021

View reviewed changes

gsquared94 merged commit 6a7d2b9 into GoogleContainerTools:master Jun 4, 2021

gsquared94 mentioned this pull request Jun 10, 2021

Can't change build environment to local using skaffold inspect build-env add #5998

Closed

gsquared94 added the area/inspect label Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add command `skaffold inspect build-env add local` #5944

add command `skaffold inspect build-env add local` #5944

Uh oh!

gsquared94 commented Jun 4, 2021 •

edited

Loading

Uh oh!

codecov bot commented Jun 4, 2021 •

edited

Loading

Uh oh!

Uh oh!

briandealwis Jun 4, 2021

Uh oh!

briandealwis Jun 4, 2021

Uh oh!

gsquared94 Jun 7, 2021

Uh oh!

briandealwis Jun 4, 2021

Uh oh!

briandealwis Jun 4, 2021

Uh oh!

briandealwis Jun 4, 2021

Uh oh!

gsquared94 commented Jun 7, 2021

Uh oh!

Uh oh!

		WithDescription("Add a new Local build environment definition").
		WithLongDescription(`Add a new Local build environment definition.

	Use the '--module' filter to specify the individual module to target. Otherwise, it'll be applied to all modules defined in the target file. Also, with the '--profile' flag if the target config imports other configs as dependencies, then the new profile will be recursively created in all the imported configs also.`).
	Use the '--module' filter to target specific modules. Otherwise, it will be applied to all modules defined in the target file. Also, with the '--profile' flag if the target config imports other configs as dependencies, then the new profile will be recursively created in all the imported configs also.`).

add command skaffold inspect build-env add local #5944

add command skaffold inspect build-env add local #5944

Uh oh!

Conversation

gsquared94 commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

briandealwis Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

briandealwis Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

gsquared94 Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

briandealwis Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

briandealwis Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

briandealwis Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

gsquared94 commented Jun 7, 2021

Uh oh!

Uh oh!

add command `skaffold inspect build-env add local` #5944

add command `skaffold inspect build-env add local` #5944

gsquared94 commented Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 4, 2021 •

edited

Loading