Make a single constructor for lifecycle inputs #1009

natalieparellano · 2023-02-13T20:59:08Z

The logic to update the default path for TOML files was repeated across phases
In general it is safe to provide default values for inputs that might not be relevant to the current phase, as these will be ignored when constructing a new service for the phase; e.g., platform.LifecycleInputs.OrderPath will be ignored when constructing a lifecycle.Exporter
As more inputs are shared across phases (e.g., analyzed.toml is now an input to the detect phase), duplicating the logic for providing default values is becoming more cumbersome

platform/detect_inputs.go

natalieparellano · 2023-02-13T21:00:35Z

platform/lifecycle_inputs.go

@@ -55,6 +55,82 @@ type LifecycleInputs struct {
 	KanikoCacheTTL        time.Duration
 }

+func NewLifecycleInputs(platformAPI *api.Version) LifecycleInputs {
+	inputs := LifecycleInputs{
+		// Operator config


Mirrored the organization in platform/defaults.go

joe-kimmel-vmw

LGTM, fwiw :)

jabrown85

I like it!

natalieparellano · 2023-02-14T17:32:57Z

platform/lifecycle_inputs.go

-var (
-	ErrOutputImageRequired           = "image argument is required"
-	ErrRunImageRequiredWhenNoStackMD = "-run-image is required when there is no stack metadata available"
-	ErrRunImageRequiredWhenNoRunMD   = "-run-image is required when there is no run metadata available"
-	ErrSupplyOnlyOneRunImage         = "supply only one of -run-image or (deprecated) -image"
-	ErrRunImageUnsupported           = "-run-image is unsupported"
-	ErrImageUnsupported              = "-image is unsupported"
-	MsgIgnoringLaunchCache           = "Ignoring -launch-cache, only intended for use with -daemon"
-)


Moved this (and other "resolve" related functionality) to other file for readability

natalieparellano · 2023-02-14T17:34:39Z

Marking as ready, but @jjbustamante we can wait for the OCI layout PR to land first to avoid extra work on your side :)

natalieparellano · 2023-02-14T17:35:15Z

Look at all that deleted code!

- The logic to update the default path for TOML files was repeated across phases - In general it is safe to provide default values for inputs that might not be relevant to the current phase, as these will be ignored when constructing a new service for the phase; e.g., platform.LifecycleInputs.OrderPath will be ignored when constructing a lifecycle.Exporter - As more inputs are shared across phases (e.g., analyzed.toml is now an input to the detect phase), duplicating the logic for providing default values is becoming more cumbersome Signed-off-by: Natalie Arellano <narellano@vmware.com>

Signed-off-by: Natalie Arellano <narellano@vmware.com>

…ut that will be updated later Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano · 2023-02-27T23:06:03Z

I brought this up to date with the latest main and made a couple other changes, chiefly requiring the desired value of layers directory to be read before a new "platform" can be instantiated.

The old pattern was to provide a "placeholder" layers directory and then after-the-fact go back and update all the placeholder paths with the correct value now that the layers directory is known. This had the downside that when adding any new inputs that defaulted to a child of the layers directory (such as the newly added "generated" dir which defaults to <layers>/generated) we would need to remember to add it as a path to update later and failing to do this resulted in confusing errors.

natalieparellano · 2023-02-27T23:08:07Z

platform/lifecycle_inputs.go

-var (
-	ErrOutputImageRequired           = "image argument is required"
-	ErrRunImageRequiredWhenNoStackMD = "-run-image is required when there is no stack metadata available"
-	ErrRunImageRequiredWhenNoRunMD   = "-run-image is required when there is no run metadata available"
-	ErrSupplyOnlyOneRunImage         = "supply only one of -run-image or (deprecated) -image"
-	ErrRunImageUnsupported           = "-run-image is unsupported"
-	ErrImageUnsupported              = "-image is unsupported"
-	ErrMultipleTargetsUnsupported    = "exporting to multiple targets is unsupported"
-	ErrLayoutDirRequired             = "defining a layout directory is required when OCI Layout feature is enabled. Use -layout-dir flag or CNB_LAYOUT_DIR environment variable"
-	MsgIgnoringLaunchCache           = "Ignoring -launch-cache, only intended for use with -daemon"
-)


Moved to resolve_inputs.go

natalieparellano · 2023-02-27T23:10:19Z

platform/resolve_analyze_inputs_test.go

 						h.AssertNil(t, err)
 						h.AssertNoLogEntry(t, logHandler, `no stack metadata found at path ''`)
 						h.AssertNoLogEntry(t, logHandler, `Previous image with name "" not found`)
 					})
 				})
 			})
 		})
-


These are now tested in lifecycle_inputs_test.go

natalieparellano · 2023-02-27T23:10:40Z

platform/resolve_create_inputs_test.go

 						h.AssertNil(t, err)
 						h.AssertNoLogEntry(t, logHandler, `no stack metadata found at path ''`)
 						h.AssertNoLogEntry(t, logHandler, `Previous image with name "" not found`)
 					})
 				})
 			})
 		})
-


These are now tested in lifecycle_inputs_test.go

Signed-off-by: Natalie Arellano <narellano@vmware.com>

joe-kimmel-vmw

sorry i feel like i'm leaving comments on your sidequest and not the main change, feel free to ignore me

joe-kimmel-vmw · 2023-02-28T02:43:05Z

cmd/lifecycle/main.go

@@ -18,31 +18,41 @@ import (
 )

 func main() {
-	switch strings.TrimSuffix(filepath.Base(os.Args[0]), filepath.Ext(os.Args[0])) {
+	phase := strings.TrimSuffix(filepath.Base(os.Args[0]), filepath.Ext(os.Args[0]))


fwiw I'm a fan of explicitly naming this string phase instead of leaving it anonymous 👏

cmd/lifecycle/main.go

jabrown85

I love this type of work!

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano commented Feb 13, 2023

View reviewed changes

platform/detect_inputs.go Show resolved Hide resolved

natalieparellano commented Feb 13, 2023

View reviewed changes

joe-kimmel-vmw reviewed Feb 14, 2023

View reviewed changes

jabrown85 approved these changes Feb 14, 2023

View reviewed changes

natalieparellano force-pushed the refactor/lifecycle-inputs branch from ea6509e to d88c8cf Compare February 14, 2023 17:31

natalieparellano commented Feb 14, 2023

View reviewed changes

natalieparellano marked this pull request as ready for review February 14, 2023 17:34

natalieparellano requested a review from a team as a code owner February 14, 2023 17:34

natalieparellano force-pushed the refactor/lifecycle-inputs branch from d88c8cf to f22c4f9 Compare February 14, 2023 18:03

Read values from environment

433bf3c

Signed-off-by: Natalie Arellano <narellano@vmware.com>

jabrown85 approved these changes Feb 14, 2023

View reviewed changes

natalieparellano added 2 commits February 27, 2023 15:29

Merge branch 'main' into refactor/lifecycle-inputs

872ad8d

Pass in layers directory instead of defaulting to a "placeholder" inp…

e48cfaa

…ut that will be updated later Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano commented Feb 27, 2023

View reviewed changes

natalieparellano added 2 commits February 27, 2023 18:11

Assert layers directory

4459cd5

Signed-off-by: Natalie Arellano <narellano@vmware.com>

Fall back to default layers when directory not provided

54e862c

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano requested a review from jabrown85 February 27, 2023 23:14

joe-kimmel-vmw reviewed Feb 28, 2023

View reviewed changes

jabrown85 approved these changes Feb 28, 2023

View reviewed changes

Consolidate -layers parsing

ad3aeeb

Signed-off-by: Natalie Arellano <narellano@vmware.com>

natalieparellano merged commit cad291f into main Feb 28, 2023

natalieparellano deleted the refactor/lifecycle-inputs branch February 28, 2023 17:01

natalieparellano mentioned this pull request Mar 2, 2023

Fix CI #1027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make a single constructor for lifecycle inputs #1009

Make a single constructor for lifecycle inputs #1009

natalieparellano commented Feb 13, 2023

natalieparellano Feb 13, 2023

joe-kimmel-vmw left a comment

jabrown85 left a comment

natalieparellano Feb 14, 2023 •

edited

Loading

natalieparellano commented Feb 14, 2023

natalieparellano commented Feb 14, 2023

natalieparellano commented Feb 27, 2023

natalieparellano Feb 27, 2023

natalieparellano Feb 27, 2023

natalieparellano Feb 27, 2023

joe-kimmel-vmw left a comment

joe-kimmel-vmw Feb 28, 2023

jabrown85 left a comment

Make a single constructor for lifecycle inputs #1009

Make a single constructor for lifecycle inputs #1009

Conversation

natalieparellano commented Feb 13, 2023

natalieparellano Feb 13, 2023

Choose a reason for hiding this comment

joe-kimmel-vmw left a comment

Choose a reason for hiding this comment

jabrown85 left a comment

Choose a reason for hiding this comment

natalieparellano Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

natalieparellano commented Feb 14, 2023

natalieparellano commented Feb 14, 2023

natalieparellano commented Feb 27, 2023

natalieparellano Feb 27, 2023

Choose a reason for hiding this comment

natalieparellano Feb 27, 2023

Choose a reason for hiding this comment

natalieparellano Feb 27, 2023

Choose a reason for hiding this comment

joe-kimmel-vmw left a comment

Choose a reason for hiding this comment

joe-kimmel-vmw Feb 28, 2023

Choose a reason for hiding this comment

jabrown85 left a comment

Choose a reason for hiding this comment

natalieparellano Feb 14, 2023 •

edited

Loading