fix: Fixes #256, added ability to associate all fields after processi…

…ng (#263)

* fix: Fixes #236 glob pattern for doublestar

Signed-off-by: Dustin Scott <[email protected]>

* fix: fix panic when workload references collectionField

This commit addresses a panic situation when the workload itself references a
collection field, however the workload itself is not using a collection field
as an input to any of its fields.

Signed-off-by: Dustin Scott <[email protected]>

* test: add functional test for panic situation

Signed-off-by: Dustin Scott <[email protected]>

* fix: Fixes #256, added ability to associate all fields after processing

Signed-off-by: Dustin Scott <[email protected]>

* fix: handle array to interface conversion more intelligently

Signed-off-by: Dustin Scott <[email protected]>

* fix: handle case of comments below markers being seen as markers

Signed-off-by: Dustin Scott <[email protected]>

* test: use namespaced resource instead of cluster scoped resource

Signed-off-by: Dustin Scott <[email protected]>

* chore: fix linter error by removing linting item for TODO

Signed-off-by: Dustin Scott <[email protected]>

* chore: prepare test case for #271

This simply is a reminder to prepare for a test case for #271.  It does not
include an empty line, as an empty line would break the test case.  Once
in order to produce our functional test to account for this.

Signed-off-by: Dustin Scott <[email protected]>

* fix: Fixes #272, unmarshal struct with omitted fields

This adds the 'omitempty' tag to all fields within the API.  This allows the
unmarshal to unmarshal with empty fields.  Without the 'omitempty' tag, the
unmarshal event results in zero values before it can be processed by the CRD
which is there to set the default values.

Signed-off-by: Dustin Scott <[email protected]>

* test: remove conflict between quota and deployments without resources

Signed-off-by: Dustin Scott <[email protected]>

* test: fix logging test when controller is not deployed in cluster

Signed-off-by: Dustin Scott <[email protected]>

* test: fix missing os path in generated file

Signed-off-by: Dustin Scott <[email protected]>

* test: adjusted resources for resource quota

Signed-off-by: Dustin Scott <[email protected]>

internal/plugins/config/v1/api.go

@@ -46,9 +46,7 @@ func (p *createAPISubcommand) InjectConfig(c config.Config) error {
 }
 
 func (p *createAPISubcommand) InjectResource(res *resource.Resource) error {
-	workload, err := workloadv1.ProcessAPIConfig(
-		p.workloadConfigPath,
-	)
+	workload, err := workloadv1.ProcessAPIConfig(p.workloadConfigPath)
 	if err != nil {
 		return fmt.Errorf("unable to inject resource into %s, %w", p.workloadConfigPath, err)
 	}

internal/plugins/workload/v1/api.go

@@ -57,9 +57,7 @@ func (p *createAPISubcommand) InjectResource(res *resource.Resource) error {
 
 func (p *createAPISubcommand) PreScaffold(machinery.Filesystem) error {
 	// load the workload config
-	workload, err := workloadv1.ProcessAPIConfig(
-		p.workloadConfigPath,
-	)
+	workload, err := workloadv1.ProcessAPIConfig(p.workloadConfigPath)
 	if err != nil {
 		return fmt.Errorf("unable to process api config for %s, %w", p.workloadConfigPath, err)
 	}

internal/plugins/workload/v1/scaffolds/templates/test/e2e/workloads.go

@@ -76,6 +76,7 @@ package e2e_test
 
 import (
 	"fmt"
+	"os"
 
 	"github.com/stretchr/testify/require"
 
@@ -147,7 +148,9 @@ func (tester *E2ETest) {{ .TesterName }}Test(testSuite *E2EComponentTestSuite) {
 	// see https://github.com/vmware-tanzu-labs/operator-builder/issues/67
 
 	// test that controller logs do not contain errors
-	require.NoErrorf(testSuite.T(), testControllerLogsNoErrors(tester.suiteConfig, tester.logSyntax), "found errors in controller logs")
+	if os.Getenv("DEPLOY_IN_CLUSTER") == "true" {
+		require.NoErrorf(testSuite.T(), testControllerLogsNoErrors(tester.suiteConfig, tester.logSyntax), "found errors in controller logs")
+	}
 }
 
 {{ if .Builder.IsCollection -}}

internal/workload/v1/api.go

@@ -277,7 +277,7 @@ func (api *APIFields) newChild(name string, fieldType FieldType, sample interfac
 		Name:         strings.Title(name),
 		manifestName: name,
 		Type:         fieldType,
-		Tags:         fmt.Sprintf("`json:%q`", name),
+		Tags:         fmt.Sprintf("`json:%q`", fmt.Sprintf("%s,%s", name, "omitempty")),
 		Comments:     []string{},
 		Markers:      []string{},
 	}

internal/workload/v1/api_internal_test.go

@@ -616,7 +616,7 @@ func TestAPIFields_newChild(t *testing.T) {
 				manifestName: "string",
 				Type:         FieldString,
 				Sample:       "string: \"string\"",
-				Tags:         "`json:\"string\"`",
+				Tags:         "`json:\"string,omitempty\"`",
 				Comments:     []string{},
 				Markers:      []string{},
 			},
@@ -634,7 +634,7 @@ func TestAPIFields_newChild(t *testing.T) {
 				manifestName: "unknown",
 				Type:         FieldUnknownType,
 				Sample:       "unknown: unknown",
-				Tags:         "`json:\"unknown\"`",
+				Tags:         "`json:\"unknown,omitempty\"`",
 				Comments:     []string{},
 				Markers:      []string{},
 			},
@@ -652,7 +652,7 @@ func TestAPIFields_newChild(t *testing.T) {
 				manifestName: "int",
 				Type:         FieldInt,
 				Sample:       "int: int",
-				Tags:         "`json:\"int\"`",
+				Tags:         "`json:\"int,omitempty\"`",
 				Comments:     []string{},
 				Markers:      []string{},
 			},
@@ -670,7 +670,7 @@ func TestAPIFields_newChild(t *testing.T) {
 				manifestName: "bool",
 				Type:         FieldBool,
 				Sample:       "bool: bool",
-				Tags:         "`json:\"bool\"`",
+				Tags:         "`json:\"bool,omitempty\"`",
 				Comments:     []string{},
 				Markers:      []string{},
 			},
@@ -688,7 +688,7 @@ func TestAPIFields_newChild(t *testing.T) {
 				manifestName: "struct",
 				Type:         FieldStruct,
 				Sample:       "struct:",
-				Tags:         "`json:\"struct\"`",
+				Tags:         "`json:\"struct,omitempty\"`",
 				Comments:     []string{},
 				Markers:      []string{},
 			},

internal/workload/v1/config.go

@@ -58,6 +58,7 @@ func ProcessInitConfig(workloadConfig string) (WorkloadInitializer, error) {
 	return workload, nil
 }
 
+//nolint:gocyclo //needs refactor
 func ProcessAPIConfig(workloadConfig string) (WorkloadAPIBuilder, error) {
 	workloads, err := parseConfig(workloadConfig)
 	if err != nil {
@@ -112,6 +113,11 @@ func ProcessAPIConfig(workloadConfig string) (WorkloadAPIBuilder, error) {
 
 	workload.SetNames()
 
+	markers := &markerCollection{
+		fieldMarkers:           []*FieldMarker{},
+		collectionFieldMarkers: []*CollectionFieldMarker{},
+	}
+
 	for _, component := range components {
 		// assign values related to the collection
 		component.Spec.Collection = collection
@@ -121,6 +127,24 @@ func ProcessAPIConfig(workloadConfig string) (WorkloadAPIBuilder, error) {
 		}
 
 		component.SetNames()
+
+		markers.fieldMarkers = append(markers.fieldMarkers, component.Spec.FieldMarkers...)
+		markers.collectionFieldMarkers = append(markers.collectionFieldMarkers, component.Spec.CollectionFieldMarkers...)
+	}
+
+	if collection != nil {
+		markers.fieldMarkers = append(markers.fieldMarkers, collection.Spec.FieldMarkers...)
+		markers.collectionFieldMarkers = append(markers.collectionFieldMarkers, collection.Spec.CollectionFieldMarkers...)
+
+		if err := collection.Spec.processResourceMarkers(markers); err != nil {
+			return nil, err
+		}
+	}
+
+	for _, component := range components {
+		if err := component.Spec.processResourceMarkers(markers); err != nil {
+			return nil, err
+		}
 	}
 
 	return workload, nil

internal/workload/v1/markers.go

@@ -16,7 +16,9 @@ import (
 )
 
 var (
-	ErrReservedFieldMarker = errors.New("field marker cannot be used and is reserved for internal purposes")
+	ErrReservedFieldMarker     = errors.New("field marker cannot be used and is reserved for internal purposes")
+	ErrNumberResourceMarkers   = errors.New("expected only 1 resource marker")
+	ErrAssociateResourceMarker = errors.New("unable to associate resource marker with field marker")
 )
 
 const (
@@ -40,11 +42,13 @@ const (
 type MarkerType int
 
 type FieldMarker struct {
-	Name          string
-	Type          FieldType
-	Description   *string
-	Default       interface{} `marker:",optional"`
-	Replace       *string
+	Name        string
+	Type        FieldType
+	Description *string
+	Default     interface{} `marker:",optional"`
+	Replace     *string
+
+	forCollection bool
 	originalValue interface{}
 }
 
@@ -59,6 +63,11 @@ type ResourceMarker struct {
 	fieldMarker     interface{}
 }
 
+type markerCollection struct {
+	fieldMarkers           []*FieldMarker
+	collectionFieldMarkers []*CollectionFieldMarker
+}
+
 var (
 	ErrMismatchedMarkerTypes            = errors.New("resource marker and field marker have mismatched types")
 	ErrResourceMarkerUnknownValueType   = errors.New("resource marker 'value' is of unknown type")
@@ -68,6 +77,7 @@ var (
 	ErrFieldMarkerInvalidType           = errors.New("field marker type is invalid")
 )
 
+//nolint:gocritic //needed to implement string interface
 func (fm FieldMarker) String() string {
 	return fmt.Sprintf("FieldMarker{Name: %s Type: %v Description: %q Default: %v}",
 		fm.Name,
@@ -90,6 +100,7 @@ func defineFieldMarker(registry *marker.Registry) error {
 
 type CollectionFieldMarker FieldMarker
 
+//nolint:gocritic //needed to implement string interface
 func (cfm CollectionFieldMarker) String() string {
 	return fmt.Sprintf("CollectionFieldMarker{Name: %s Type: %v Description: %q Default: %v}",
 		cfm.Name,
@@ -328,25 +339,35 @@ func (rm *ResourceMarker) hasValue() bool {
 	return rm.Value != nil
 }
 
-func (rm *ResourceMarker) associateFieldMarker(spec *WorkloadSpec) {
+func (rm *ResourceMarker) associateFieldMarker(markers *markerCollection) {
 	// return immediately if our entire workload spec has no field markers
-	if len(spec.CollectionFieldMarkers) == 0 && len(spec.FieldMarkers) == 0 {
+	if len(markers.collectionFieldMarkers) == 0 && len(markers.fieldMarkers) == 0 {
 		return
 	}
 
 	// associate first relevant field marker with this marker
-	for _, fm := range spec.FieldMarkers {
+	for _, fm := range markers.fieldMarkers {
 		if rm.Field != nil {
 			if fm.Name == *rm.Field {
 				rm.fieldMarker = fm
 
 				return
 			}
 		}
+
+		if fm.forCollection {
+			if rm.CollectionField != nil {
+				if fm.Name == *rm.CollectionField {
+					rm.fieldMarker = fm
+
+					return
+				}
+			}
+		}
 	}
 
 	// associate first relevant collection field marker with this marker
-	for _, cm := range spec.CollectionFieldMarkers {
+	for _, cm := range markers.collectionFieldMarkers {
 		if rm.CollectionField != nil {
 			if cm.Name == *rm.CollectionField {
 				rm.fieldMarker = cm
@@ -425,3 +446,20 @@ func (rm *ResourceMarker) process() error {
 
 	return nil
 }
+
+// filterResourceMarkers removes all comments from the results as resource markers
+// are required to exist on the same line.
+func filterResourceMarkers(results []*inspect.YAMLResult) []*ResourceMarker {
+	var markers []*ResourceMarker
+
+	for _, marker := range results {
+		switch marker := marker.Object.(type) {
+		case ResourceMarker:
+			markers = append(markers, &marker)
+		default:
+			continue
+		}
+	}
+
+	return markers
+}

internal/workload/v1/markers_internal_test.go

@@ -307,8 +307,8 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 	fieldOne := "fieldOne"
 	fieldTwo := "fieldTwo"
 
-	testWorkloadSpec := &WorkloadSpec{
-		CollectionFieldMarkers: []*CollectionFieldMarker{
+	testMarkers := &markerCollection{
+		collectionFieldMarkers: []*CollectionFieldMarker{
 			{
 				Name: collectionFieldOne,
 				Type: FieldString,
@@ -318,7 +318,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 				Type: FieldString,
 			},
 		},
-		FieldMarkers: []*FieldMarker{
+		fieldMarkers: []*FieldMarker{
 			{
 				Name: fieldOne,
 				Type: FieldString,
@@ -341,7 +341,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 	}
 
 	type args struct {
-		spec *WorkloadSpec
+		markers *markerCollection
 	}
 
 	tests := []struct {
@@ -353,7 +353,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 		{
 			name: "resource marker with non-collection field first",
 			args: args{
-				spec: testWorkloadSpec,
+				markers: testMarkers,
 			},
 			fields: fields{
 				Field: &fieldOne,
@@ -369,7 +369,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 		{
 			name: "resource marker with non-collection field second",
 			args: args{
-				spec: testWorkloadSpec,
+				markers: testMarkers,
 			},
 			fields: fields{
 				Field: &fieldTwo,
@@ -385,7 +385,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 		{
 			name: "resource marker with collection field first",
 			args: args{
-				spec: testWorkloadSpec,
+				markers: testMarkers,
 			},
 			fields: fields{
 				CollectionField: &collectionFieldOne,
@@ -401,7 +401,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 		{
 			name: "resource marker with collection field second",
 			args: args{
-				spec: testWorkloadSpec,
+				markers: testMarkers,
 			},
 			fields: fields{
 				CollectionField: &collectionFieldTwo,
@@ -417,7 +417,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 		{
 			name: "resource marker with no related fields",
 			args: args{
-				spec: testWorkloadSpec,
+				markers: testMarkers,
 			},
 			fields: fields{
 				CollectionField: &testMissing,
@@ -443,7 +443,7 @@ func TestResourceMarker_associateFieldMarker(t *testing.T) {
 				sourceCodeValue: tt.fields.sourceCodeValue,
 				fieldMarker:     tt.fields.fieldMarker,
 			}
-			rm.associateFieldMarker(tt.args.spec)
+			rm.associateFieldMarker(tt.args.markers)
 			assert.Equal(t, tt.want, rm)
 		})
 	}