always sort additional printer columns

When returning AdditionalPrinterColumns for a CRD, we now always sort
the list of PrinterColumn structs by their Name. This prevents the
non-deterministic behaviour we've seen in Issue #720.

Also, the `pkg/generate:Generator.GetCRDs()` method was incorrectly only
adding PrinterColumns to a CRD when the field was specified as being
sourced from a different shape than the Create input or output shape (in
other words, only when the `pkg/generate/config.FieldConfig.From` field
was non-nil). This patch corrects this so that any field in a resource
may be marked as a printer column.

Fixes Issue #720

Author: jaypipes

pkg/generate/codedeploy_test.go

@@ -110,4 +110,21 @@ func TestCodeDeploy_Deployment(t *testing.T) {
 	// But sadly, has no Update or Delete operation :(
 	assert.Nil(crd.Ops.Update)
 	assert.Nil(crd.Ops.Delete)
+
+	// We marked the fields, "ApplicationName", "DeploymentGroupName",
+	// "DeploymentConfigName and "Description" as printer columns in the
+	// generator.yaml. Let's make sure that they are always returned in sorted
+	// order.
+	expPrinterColNames := []string{
+		"ApplicationName",
+		"DeploymentConfigName",
+		"DeploymentGroupName",
+		"Description",
+	}
+	gotPrinterCols := crd.AdditionalPrinterColumns()
+	gotPrinterColNames := []string{}
+	for _, pc := range gotPrinterCols {
+		gotPrinterColNames = append(gotPrinterColNames, pc.Name)
+	}
+	assert.Equal(expPrinterColNames, gotPrinterColNames)
 }

pkg/generate/generator.go

@@ -141,11 +141,7 @@ func (g *Generator) GetCRDs() ([]*ackmodel.CRD, error) {
 			)
 			if found {
 				memberNames := names.New(targetFieldName)
-				field := crd.AddSpecField(memberNames, memberShapeRef)
-
-				if fieldConfig.IsPrintable {
-					crd.AddSpecPrintableColumn(field)
-				}
+				crd.AddSpecField(memberNames, memberShapeRef)
 			} else {
 				// This is a compile-time failure, just bomb out...
 				msg := fmt.Sprintf(
@@ -214,11 +210,7 @@ func (g *Generator) GetCRDs() ([]*ackmodel.CRD, error) {
 			)
 			if found {
 				memberNames := names.New(targetFieldName)
-				field := crd.AddStatusField(memberNames, memberShapeRef)
-
-				if fieldConfig.IsPrintable {
-					crd.AddStatusPrintableColumn(field)
-				}
+				crd.AddStatusField(memberNames, memberShapeRef)
 			} else {
 				// This is a compile-time failure, just bomb out...
 				msg := fmt.Sprintf(

pkg/generate/testdata/models/apis/codedeploy/0000-00-00/generator.yaml

@@ -4,3 +4,13 @@ resources:
       errors:
         404:
           code: DeploymentDoesNotExistException
+    # below, we're testing printer columns end up sorted properly in the CRD
+    fields:
+      DeploymentGroupName:
+        is_printable: true
+      ApplicationName:
+        is_printable: true
+      DeploymentConfigName:
+        is_printable: true
+      Description:
+        is_printable: true

pkg/model/crd.go

@@ -14,7 +14,6 @@
 package model
 
 import (
-	"fmt"
 	"sort"
 	"strings"
 
@@ -58,14 +57,6 @@ func (ops Ops) IterOps() []*awssdkmodel.Operation {
 	return res
 }
 
-// PrinterColumn represents a single field in the CRD's Spec or Status objects
-type PrinterColumn struct {
-	CRD      *CRD
-	Name     string
-	Type     string
-	JSONPath string
-}
-
 // CRD describes a single top-level resource in an AWS service API
 type CRD struct {
 	sdkAPI *SDKAPI
@@ -75,10 +66,9 @@ type CRD struct {
 	Plural string
 	// Ops are the CRUD operations controlling this resource
 	Ops Ops
-	// AdditionalPrinterColumns is an array of PrinterColumn objects
+	// additionalPrinterColumns is an array of PrinterColumn objects
 	// representing the printer column settings for the CRD
-	// AdditionalPrinterColumns field.
-	AdditionalPrinterColumns []*PrinterColumn
+	additionalPrinterColumns []*PrinterColumn
 	// SpecFields is a map, keyed by the **original SDK member name** of
 	// Field objects representing those fields in the CRD's Spec struct
 	// field.
@@ -178,22 +168,29 @@ func (r *CRD) InputFieldRename(
 func (r *CRD) AddSpecField(
 	memberNames names.Names,
 	shapeRef *awssdkmodel.ShapeRef,
-) *Field {
-	fieldConfigs := r.cfg.ResourceFields(r.Names.Original)
-	f := newField(r, memberNames, shapeRef, fieldConfigs[memberNames.Original])
+) {
+	fConfigs := r.cfg.ResourceFields(r.Names.Original)
+	fConfig := fConfigs[memberNames.Original]
+	f := newField(r, memberNames, shapeRef, fConfig)
+	if fConfig != nil && fConfig.IsPrintable {
+		r.addSpecPrintableColumn(f)
+	}
 	r.SpecFields[memberNames.Original] = f
-	return f
 }
 
 // AddStatusField adds a new Field of a given name and shape into the Status
 // field of a CRD
 func (r *CRD) AddStatusField(
 	memberNames names.Names,
 	shapeRef *awssdkmodel.ShapeRef,
-) *Field {
-	f := newField(r, memberNames, shapeRef, nil)
+) {
+	fConfigs := r.cfg.ResourceFields(r.Names.Original)
+	fConfig := fConfigs[memberNames.Original]
+	f := newField(r, memberNames, shapeRef, fConfig)
+	if fConfig != nil && fConfig.IsPrintable {
+		r.addStatusPrintableColumn(f)
+	}
 	r.StatusFields[memberNames.Original] = f
-	return f
 }
 
 // AddTypeImport adds an entry in the CRD's TypeImports map for an import line
@@ -218,80 +215,6 @@ func (r *CRD) SpecFieldNames() []string {
 	return res
 }
 
-// AddPrintableColumn adds an entry to the list of additional printer columns
-// using the given path and field types.
-func (r *CRD) AddPrintableColumn(
-	field *Field,
-	jsonPath string,
-) *PrinterColumn {
-	fieldColumnType := field.GoTypeElem
-
-	// Printable columns must be primitives supported by the OpenAPI list of data
-	// types as defined by
-	// https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types
-	// This maps Go type to OpenAPI type.
-	acceptableColumnMaps := map[string]string{
-		"string":  "string",
-		"boolean": "boolean",
-		"int":     "integer",
-		"int8":    "integer",
-		"int16":   "integer",
-		"int32":   "integer",
-		"int64":   "integer",
-		"uint":    "integer",
-		"uint8":   "integer",
-		"uint16":  "integer",
-		"uint32":  "integer",
-		"uint64":  "integer",
-		"uintptr": "integer",
-		"float32": "number",
-		"float64": "number",
-	}
-	printColumnType, exists := acceptableColumnMaps[fieldColumnType]
-
-	if !exists {
-		msg := fmt.Sprintf(
-			"GENERATION FAILURE! Unable to generate a printer column for the field %s that has type %s.",
-			field.Names.Camel, fieldColumnType,
-		)
-		panic(msg)
-		return nil
-	}
-
-	column := &PrinterColumn{
-		CRD:      r,
-		Name:     field.Names.Camel,
-		Type:     printColumnType,
-		JSONPath: jsonPath,
-	}
-	r.AdditionalPrinterColumns = append(r.AdditionalPrinterColumns, column)
-	return column
-}
-
-// AddSpecPrintableColumn adds an entry to the list of additional printer columns
-// using the path of the given spec field.
-func (r *CRD) AddSpecPrintableColumn(
-	field *Field,
-) *PrinterColumn {
-	return r.AddPrintableColumn(
-		field,
-		//TODO(nithomso): Ideally we'd use `r.cfg.PrefixConfig.SpecField` but it uses uppercase
-		fmt.Sprintf("%s.%s", ".spec", field.Names.CamelLower),
-	)
-}
-
-// AddStatusPrintableColumn adds an entry to the list of additional printer columns
-// using the path of the given status field.
-func (r *CRD) AddStatusPrintableColumn(
-	field *Field,
-) *PrinterColumn {
-	return r.AddPrintableColumn(
-		field,
-		//TODO(nithomso): Ideally we'd use `r.cfg.PrefixConfig.StatusField` but it uses uppercase
-		fmt.Sprintf("%s.%s", ".status", field.Names.CamelLower),
-	)
-}
-
 // UnpacksAttributesMap returns true if the underlying API has
 // Get{Resource}Attributes/Set{Resource}Attributes API calls that map real,
 // schema'd fields to a raw `map[string]*string` for this resource (see SNS and
@@ -470,7 +393,7 @@ func NewCRD(
 		Kind:                     kind,
 		Plural:                   plural,
 		Ops:                      ops,
-		AdditionalPrinterColumns: make([]*PrinterColumn, 0),
+		additionalPrinterColumns: make([]*PrinterColumn, 0),
 		SpecFields:               map[string]*Field{},
 		StatusFields:             map[string]*Field{},
 		ShortNames:               cfg.ResourceShortNames(kind),

pkg/model/printer_column.go

@@ -0,0 +1,142 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//     http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package model
+
+import (
+	"fmt"
+	"sort"
+)
+
+// PrinterColumn represents a single field in the CRD's Spec or Status objects
+type PrinterColumn struct {
+	CRD      *CRD
+	Name     string
+	Type     string
+	JSONPath string
+}
+
+// By can sort two PrinterColumns
+type By func(a, b *PrinterColumn) bool
+
+// Sort does an in-place sort of the supplied printer columns
+func (by By) Sort(subject []*PrinterColumn) {
+	pcs := printerColumnSorter{
+		cols: subject,
+		by:   by,
+	}
+	sort.Sort(pcs)
+}
+
+// printerColumnSorter sorts printer columns by name
+type printerColumnSorter struct {
+	cols []*PrinterColumn
+	by   By
+}
+
+// Len implements sort.Interface.Len
+func (pcs printerColumnSorter) Len() int {
+	return len(pcs.cols)
+}
+
+// Swap implements sort.Interface.Swap
+func (pcs printerColumnSorter) Swap(i, j int) {
+	pcs.cols[i], pcs.cols[j] = pcs.cols[j], pcs.cols[i]
+}
+
+// Less implements sort.Interface.Less
+func (pcs printerColumnSorter) Less(i, j int) bool {
+	return pcs.by(pcs.cols[i], pcs.cols[j])
+}
+
+// AdditionalPrinterColumns returns a sorted list of PrinterColumn structs for
+// the resource
+func (r CRD) AdditionalPrinterColumns() []*PrinterColumn {
+	byName := func(a, b *PrinterColumn) bool {
+		return a.Name < b.Name
+	}
+	By(byName).Sort(r.additionalPrinterColumns)
+	return r.additionalPrinterColumns
+}
+
+// addPrintableColumn adds an entry to the list of additional printer columns
+// using the given path and field types.
+func (r *CRD) addPrintableColumn(
+	field *Field,
+	jsonPath string,
+) {
+	fieldColumnType := field.GoTypeElem
+
+	// Printable columns must be primitives supported by the OpenAPI list of data
+	// types as defined by
+	// https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types
+	// This maps Go type to OpenAPI type.
+	acceptableColumnMaps := map[string]string{
+		"string":  "string",
+		"boolean": "boolean",
+		"int":     "integer",
+		"int8":    "integer",
+		"int16":   "integer",
+		"int32":   "integer",
+		"int64":   "integer",
+		"uint":    "integer",
+		"uint8":   "integer",
+		"uint16":  "integer",
+		"uint32":  "integer",
+		"uint64":  "integer",
+		"uintptr": "integer",
+		"float32": "number",
+		"float64": "number",
+	}
+	printColumnType, exists := acceptableColumnMaps[fieldColumnType]
+
+	if !exists {
+		msg := fmt.Sprintf(
+			"GENERATION FAILURE! Unable to generate a printer column for the field %s that has type %s.",
+			field.Names.Camel, fieldColumnType,
+		)
+		panic(msg)
+	}
+
+	column := &PrinterColumn{
+		CRD:      r,
+		Name:     field.Names.Camel,
+		Type:     printColumnType,
+		JSONPath: jsonPath,
+	}
+	r.additionalPrinterColumns = append(r.additionalPrinterColumns, column)
+}
+
+// addSpecPrintableColumn adds an entry to the list of additional printer columns
+// using the path of the given spec field.
+func (r *CRD) addSpecPrintableColumn(
+	field *Field,
+) {
+	r.addPrintableColumn(
+		field,
+		//TODO(nithomso): Ideally we'd use `r.cfg.PrefixConfig.SpecField` but it uses uppercase
+		fmt.Sprintf("%s.%s", ".spec", field.Names.CamelLower),
+	)
+}
+
+// addStatusPrintableColumn adds an entry to the list of additional printer columns
+// using the path of the given status field.
+func (r *CRD) addStatusPrintableColumn(
+	field *Field,
+) {
+	r.addPrintableColumn(
+		field,
+		//TODO(nithomso): Ideally we'd use `r.cfg.PrefixConfig.StatusField` but it uses uppercase
+		fmt.Sprintf("%s.%s", ".status", field.Names.CamelLower),
+	)
+}