Add a configuration for `additional_columns` at the Resource level #378

jljaco · 2022-12-05T02:19:51Z

Issue #, if available: 1281

Description of changes:

We want to have the option to include additional printer columns in the output of kubectl. This change allow the author to specify an arbitrary number of those per Resource, for each specifying the name, type, and JSON path of the field to produce.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot · 2022-12-05T02:19:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

a-hilaly

Great stuff! Thank you @jljaco.
I left few comments and suggestions below

a-hilaly · 2022-12-05T17:06:59Z

pkg/config/resource.go

+
+	// AdditionalColumns can be used to add arbitrary extra columns to a Resource's output
+	// if present, should be a list of objects, each containing: name, json_path, and type
+	AdditionalColumns []AdditionalColumnConfig `json:"additional_columns,omitempty"`


Shall we make this a pointers array? if we do so GetAdditionalColumns will not have to allocate a new AdditionalColumnConfig every time there an error, or something missing.

Suggested change

AdditionalColumns []AdditionalColumnConfig `json:"additional_columns,omitempty"`

AdditionalColumns []*AdditionalColumnConfig `json:"additional_columns,omitempty"`

a-hilaly · 2022-12-05T17:11:43Z

pkg/model/model.go

+			printerColumn := &PrinterColumn{}
+			printerColumn.Name = additionalColumn.Name
+			printerColumn.JSONPath = additionalColumn.JSONPath
+			printerColumn.Type = additionalColumn.Type


Ideally, we should also set Priority and Index

a-hilaly · 2022-12-05T17:13:12Z

pkg/config/config.go

+	}
+
+	resourceConfig, ok := c.Resources[resourceName]
+	if !ok || resourceConfig.Print == nil || resourceConfig.Print.AdditionalColumns == nil {


nil checks on slices are not super reliable in Go. The len built-in function is the most accurate one for this use case (it checks the nility first then size of the slice)

Suggested change

if !ok || resourceConfig.Print == nil || resourceConfig.Print.AdditionalColumns == nil {

if !ok || resourceConfig.Print == nil || len(resourceConfig.Print.AdditionalColumns) == 0 {

a-hilaly · 2022-12-05T17:17:36Z

pkg/config/resource.go

+type AdditionalColumnConfig struct {
+	Name string `json:"name"`
+	JSONPath string `json:"json_path"`
+	Type string `json:"type"`
+}


nit: Please include tiny comments for the struct and its fields. Those will be included in our generated GoDoc documentation

a-hilaly · 2022-12-05T17:18:23Z

pkg/model/model.go

@@ -284,6 +284,16 @@ func (m *Model) GetCRDs() ([]*CRD, error) {
 			crd.AddStatusField(memberNames, memberShapeRef)
 		}

+		// Now add the additional printer columns that have been defined explicitly
+		// in additional_columns
+		for _, additionalColumn := range m.cfg.GetAdditionalColumns(crdName) {


tiny suggestion: Shall we make this a private method? thinking crd.setAdditionalPrinterColumns()

pkg/config/config.go

a-hilaly · 2022-12-07T22:39:13Z

pkg/config/config.go

-	if !ok || resourceConfig.Print == nil || resourceConfig.Print.AdditionalColumns == nil {
-		return []AdditionalColumnConfig{}
+	if !ok || resourceConfig.Print == nil || len(resourceConfig.Print.AdditionalColumns) == 0 {
+		return []*AdditionalColumnConfig{}


a-hilaly · 2022-12-07T22:40:22Z

pkg/config/resource.go

+	// the name to display in the column's output
 	Name string `json:"name"`
+	// the JSONPath definining the source of the output
 	JSONPath string `json:"json_path"`
+	// the OpenAPI type of the output
+	// c.f., https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#data-types
 	Type string `json:"type"`
+	// the priority of the column in the resource's output
+	Priority int `json:"priority,omitempty"`
+	// the zero-based index of the position at which to display the column in output
+	Index int `json:"index,omitempty"`


Quotting from https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences:

Comments should begin with the name of the thing being described and end in a period

a-hilaly · 2022-12-07T22:43:25Z

pkg/model/crd.go

+func (r *CRD) setAdditionalPrinterColumns(additionalColumns []*ackgenconfig.AdditionalColumnConfig) {
+	r.additionalPrinterColumns = []*PrinterColumn{}
+
+	for _, additionalColumn := range additionalColumns {
+		printerColumn := &PrinterColumn{}
+		printerColumn.Name = additionalColumn.Name
+		printerColumn.JSONPath = additionalColumn.JSONPath
+		printerColumn.Type = additionalColumn.Type
+		printerColumn.Priority = additionalColumn.Priority
+		printerColumn.Index = additionalColumn.Index
+		r.additionalPrinterColumns = append(r.additionalPrinterColumns, printerColumn)
+	}
+}
+


The allocation on L570 overrides the existing printerColumns previously added in model.go L284 and L236. Removing L570 should fix the code and the unit tests

Ahh, good point. In that case, I'm going to rename this addAdditionalPrinterColumns to avoid ambiguity.

Co-authored-by: Amine <hilalyamine@gmail.com>

a-hilaly

Top! Merci beaucoup :)
/lgtm

ack-bot · 2022-12-07T22:57:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jljaco

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~~ [A-Hilaly,jljaco]

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

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2022

ack-bot requested review from a-hilaly and RedbackThomson December 5, 2022 02:19

ack-bot added the approved label Dec 5, 2022

Add a configuration for "additionalColumns" at the Resource level

a474316

jljaco force-pushed the jljaco/1281_additional_printer_columns branch from 95fba7d to a474316 Compare December 5, 2022 02:22

a-hilaly reviewed Dec 5, 2022

View reviewed changes

Feedback from @a-hilaly

44ccccd

jljaco requested a review from a-hilaly December 7, 2022 22:24

jljaco marked this pull request as ready for review December 7, 2022 22:28

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2022

a-hilaly reviewed Dec 7, 2022

View reviewed changes

Update pkg/config/config.go

0255fa5

Co-authored-by: Amine <hilalyamine@gmail.com>

jljaco force-pushed the jljaco/1281_additional_printer_columns branch from 247b895 to 0255fa5 Compare December 7, 2022 22:46

More feedback from @a-hilaly

8ca54e3

jljaco requested a review from a-hilaly December 7, 2022 22:50

a-hilaly approved these changes Dec 7, 2022

View reviewed changes

ack-bot assigned a-hilaly Dec 7, 2022

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2022

ack-bot merged commit 638a172 into aws-controllers-k8s:main Dec 7, 2022

jljaco deleted the jljaco/1281_additional_printer_columns branch December 7, 2022 22:58

jljaco mentioned this pull request Dec 8, 2022

Prow does not wait for all tests to pass before merging following a /lgtm aws-controllers-k8s/community#1588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a configuration for `additional_columns` at the Resource level #378

Add a configuration for `additional_columns` at the Resource level #378

jljaco commented Dec 5, 2022 •

edited

Loading

ack-bot commented Dec 5, 2022

a-hilaly left a comment

a-hilaly Dec 5, 2022

jljaco Dec 7, 2022

a-hilaly Dec 5, 2022

jljaco Dec 7, 2022

a-hilaly Dec 5, 2022

jljaco Dec 7, 2022

a-hilaly Dec 5, 2022

jljaco Dec 7, 2022

a-hilaly Dec 5, 2022

jljaco Dec 7, 2022

a-hilaly Dec 7, 2022

jljaco Dec 7, 2022

a-hilaly Dec 7, 2022

jljaco Dec 7, 2022

a-hilaly Dec 7, 2022

jljaco Dec 7, 2022

jljaco Dec 7, 2022

a-hilaly left a comment

ack-bot commented Dec 7, 2022

	AdditionalColumns []AdditionalColumnConfig `json:"additional_columns,omitempty"`
	AdditionalColumns []*AdditionalColumnConfig `json:"additional_columns,omitempty"`

	if !ok \|\| resourceConfig.Print == nil \|\| resourceConfig.Print.AdditionalColumns == nil {
	if !ok \|\| resourceConfig.Print == nil \|\| len(resourceConfig.Print.AdditionalColumns) == 0 {

Add a configuration for additional_columns at the Resource level #378

Add a configuration for additional_columns at the Resource level #378

Conversation

jljaco commented Dec 5, 2022 • edited Loading

ack-bot commented Dec 5, 2022

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly left a comment

Choose a reason for hiding this comment

ack-bot commented Dec 7, 2022

Add a configuration for `additional_columns` at the Resource level #378

Add a configuration for `additional_columns` at the Resource level #378

jljaco commented Dec 5, 2022 •

edited

Loading