fix: image name derivation and flag precidence #1185

lkingland · 2022-08-24T06:05:13Z

🧹 clean up image name derivation logic
🐛 image flag now correctly respected
🎁 tests in commands and core codifying registry vs image treatement
🎁 prompts on deploy should now correctly calculate image default
🧹 clean up namespace derivation logic and add a test

Clean up logic used to derive and update image name based on registry, function name etc.
Adds requisite tests to both deploy and build.

/kind cleanup

cmd/deploy.go

function.go

codecov · 2022-08-31T19:41:46Z

Codecov Report

Merging #1185 (53d2c3e) into main (d8bcf96) will increase coverage by 0.12%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #1185      +/-   ##
==========================================
+ Coverage   45.20%   45.32%   +0.12%     
==========================================
  Files          68       68              
  Lines        9745     9699      -46     
==========================================
- Hits         4405     4396       -9     
+ Misses       4940     4914      -26     
+ Partials      400      389      -11

Impacted Files	Coverage Δ
cmd/root.go	`71.72% <0.00%> (-3.47%)`	⬇️
cmd/run.go	`72.80% <0.00%> (ø)`
client.go	`62.99% <44.44%> (+0.04%)`	⬆️
cmd/build.go	`67.23% <47.72%> (-1.40%)`	⬇️
cmd/deploy.go	`65.30% <65.51%> (+6.03%)`	⬆️
function.go	`64.31% <85.00%> (+5.32%)`	⬆️
s2i/builder.go	`61.04% <0.00%> (-1.21%)`	⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lkingland · 2022-08-31T19:44:29Z

cmd/build.go

 	if !interactiveTerminal() || !c.Confirm {
 		return c, nil
 	}

-	bc := buildConfig{Verbose: c.Verbose}
+	imageName := deriveImage(c.Image, c.Registry, c.Path)


Two things happen here: The current state of the config is allowed to flow through as the defaults to the prompts, and we move down the line which is calculating the default image name to after preconditions.

cmd/build.go

lkingland · 2022-08-31T19:48:28Z

cmd/build_test.go

@@ -223,119 +181,3 @@ created: 2009-11-10 23:00:00`,
 		})
 	}
 }
-
-func testBuilderPersistence(t *testing.T, testRegistry string, cmdBuilder func(ClientFactory) *cobra.Command) {


The implementations of the shared tests are moved into deploy_test.go since it is the superset of deploy+build

lkingland · 2022-08-31T19:57:44Z

cmd/build_test.go

 }

-// TestBuild_registryConfigurationInYaml tests that a build will execute successfully
-// when there is no registry provided on the command line, but one exists in func.yaml
-func TestBuild_registryConfigurationInYaml(t *testing.T) {


Most of the tests are just moved/shared. This was more heavily refactored to use the client API rather than a func.yaml as a sub-assertion in TestBuild_RegistryLoads

lkingland · 2022-08-31T20:02:28Z

cmd/deploy.go

-	if function.Image == "" && currentBuildType != "disabled" {
-		//  AND a --registry was not provided, then we need to
-		// prompt for a registry from which we can derive an image name.
-		if config.Registry == "" {
-			fmt.Println("A registry for function images is required. For example, 'docker.io/tigerteam'.")
-
-			err = survey.AskOne(
-				&survey.Input{Message: "Registry for function images:"},
-				&config.Registry, survey.WithValidator(survey.Required))
-			if err != nil {
-				if err == terminal.InterruptErr {
-					return nil
-				}
-				return
-			}
-		}
-
-		// We have the registry, so let's use it to derive the function image name
-		config.Image = deriveImage(config.Image, config.Registry, config.Path)
-		function.Image = config.Image


This prompt block is part of the flow initiated by --confirm (config.Prompt), with normal execution mode usually expected to result in an immediate non-zero exit code and error message printed to stderr.

zroubalik

/lgtm great improvement :)

/hold for others

knative-prow · 2022-09-02T09:13:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, zroubalik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lkingland,zroubalik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lkingland · 2022-09-04T19:36:44Z

Since parent PR (#1079) already got a few approvals when these changes were originally included therein, I am presuming this derivitave is GTG with the single approval

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2022

lkingland commented Aug 26, 2022

View reviewed changes

cmd/deploy.go Outdated Show resolved Hide resolved

lkingland force-pushed the lkingland/image-name branch from c01945d to 57c2529 Compare August 26, 2022 19:40

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2022

lkingland force-pushed the lkingland/image-name branch from 57c2529 to 0a16525 Compare August 26, 2022 20:40

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 26, 2022

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

lkingland added 7 commits August 31, 2022 12:59

fix: image name derivation and flag precidence

653896e

cleanup

bb43e31

fix image prompt, reorganize and cleanup

b35b9cd

client deploy image test

8eedee1

be explicit about registry treatment in tests

a8d98e8

ineffassign

0c9c06e

fix: k8s env not required

279c390

lkingland force-pushed the lkingland/image-name branch from 1913e8c to 279c390 Compare August 31, 2022 19:12

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

lkingland commented Aug 31, 2022

View reviewed changes

function.go Show resolved Hide resolved

misspell

0e57515

lkingland commented Aug 31, 2022

View reviewed changes

cmd/build.go Show resolved Hide resolved

lkingland commented Aug 31, 2022

View reviewed changes

minor cleanup

e5d4514

lkingland commented Aug 31, 2022

View reviewed changes

lkingland added 2 commits August 31, 2022 14:13

recombine build and deploy configs

d83f780

test namesace choosing logic

53d2c3e

lkingland added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/bug Categorizes issue or PR as related to a bug. labels Aug 31, 2022

lkingland marked this pull request as ready for review August 31, 2022 20:56

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022

lkingland requested review from lance, zroubalik and matejvasek September 1, 2022 23:08

zroubalik approved these changes Sep 2, 2022

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2022

knative-prow bot assigned zroubalik Sep 2, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022

lkingland removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2022

knative-prow bot merged commit ae75e5e into knative:main Sep 4, 2022

lkingland deleted the lkingland/image-name branch September 20, 2022 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: image name derivation and flag precidence #1185

fix: image name derivation and flag precidence #1185

lkingland commented Aug 24, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

lkingland Aug 31, 2022 •

edited

Loading

lkingland Aug 31, 2022

lkingland Aug 31, 2022 •

edited

Loading

lkingland Aug 31, 2022

zroubalik left a comment

knative-prow bot commented Sep 2, 2022

lkingland commented Sep 4, 2022

fix: image name derivation and flag precidence #1185

fix: image name derivation and flag precidence #1185

Conversation

lkingland commented Aug 24, 2022 • edited Loading

codecov bot commented Aug 31, 2022 • edited Loading

Codecov Report

lkingland Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

lkingland Aug 31, 2022

Choose a reason for hiding this comment

lkingland Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

lkingland Aug 31, 2022

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

knative-prow bot commented Sep 2, 2022

lkingland commented Sep 4, 2022

lkingland commented Aug 24, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

lkingland Aug 31, 2022 •

edited

Loading

lkingland Aug 31, 2022 •

edited

Loading