feat: add AdditionalPrinterColumnPolicy to control CRD printer column… by yashpal2104 · Pull Request #860 · kubernetes-sigs/kro

yashpal2104 · 2025-11-24T20:18:51Z

… merging

Add AdditionalPrinterColumnPolicy field to Schema with two modes:

Replace (default): User columns replace defaults; falls back to defaults if empty
Add: Merges user columns with defaults; user columns override on Name/JSONPath match

This allows users to either fully control printer columns or extend the defaults, maintaining backward compatibility with existing RGDs.

Includes comprehensive unit and e2e tests for both policy modes.

k8s-ci-robot · 2025-11-24T20:18:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yashpal2104
Once this PR has been reviewed and has the lgtm label, please assign nicslatts for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2025-11-24T20:19:01Z

Hi @yashpal2104. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

yashpal2104 · 2025-11-25T13:24:59Z

/ok-to-test

k8s-ci-robot · 2025-11-25T13:25:15Z

@yashpal2104: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jakobmoellerdev · 2025-11-25T18:50:46Z

                      the SimpleSchema spec.
                    type: object
                    x-kubernetes-preserve-unknown-fields: true
+                  validation:


this is a leftover. you really need to rebase properly.

jakobmoellerdev

TLDR: please stop recreating prs and rebasing incorrectly.

this is the last review i will make on a fresh pr. please stop recreating pull requests or I will start closing them.

You already created #790 and #653 even though on the second one I offered you to collaborate. you make it really hard for us maintainers to review and keep control of the pull request list otherwise. thanks!

yashpal2104 · 2025-11-25T18:56:01Z

I am sorry but I don't know how can I add you as a collaborator. Didn't mean to create problems for you, but I thought creating fesh PR and cherry picking will solve the problem.
Please do let me know how do I add you as a collaborator.
Sorry for the trouble caused

Copilot

Pull request overview

This PR adds the AdditionalPrinterColumnPolicy field to the Schema API to control how custom printer columns are merged with default columns in generated CRDs. Currently only "Add" mode is implemented, which merges user-provided columns with the defaults (State, Ready, Age), with user columns overriding defaults when they share the same Name.

Introduces AdditionalPrinterColumnPolicy enum type with "Add" as the only supported and default policy
Implements column merging logic that overrides defaults by Name and appends new columns
Includes comprehensive unit tests and e2e tests validating the merge behavior

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
api/v1alpha1/resourcegraphdefinition_types.go	Adds `AdditionalPrinterColumnPolicy` type and `Validation` struct; updates Schema with new fields
api/v1alpha1/zz_generated.deepcopy.go	Generated deep copy methods for new Validation struct
helm/crds/kro.run_resourcegraphdefinitions.yaml	Updates CRD schema to include new printer column policy and validation fields with documentation
pkg/graph/builder.go	Updates SynthesizeCRD call to pass the new policy parameter
pkg/graph/crd/crd.go	Implements printer column merging logic and updates SynthesizeCRD signature
pkg/graph/crd/crd_test.go	Adds comprehensive unit tests for printer column merging behavior
test/e2e/chainsaw/check-additional-printer-columns/*.yaml	E2e test files validating default and Add policy behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-29T19:35:23Z

+// AdditionalPrinterColumnPolicy defines how additional printer columns are applied to the generated CustomResourceDefinition (CRD).
+type AdditionalPrinterColumnPolicy string
+
+const (
+	// AdditionalPrinterColumnPolicyAdd specifies that the provided additional
+	// printer columns should be appended to the default columns. When there are
+	// conflicts (columns with the same Name), the user-provided columns take
+	// precedence over the defaults.
+	AdditionalPrinterColumnPolicyAdd AdditionalPrinterColumnPolicy = "Add"


The description states "Replace (default): User columns replace defaults" but the API definition shows "Add" as the only supported policy with +kubebuilder:default=Add. The PR description and CRD documentation are inconsistent with the actual implementation. Either "Replace" mode should be implemented or the PR description should be updated to clarify that only "Add" mode is currently supported.

Copilot · 2025-12-29T19:35:23Z

+	// Use defaults if no columns provided
+	printerColumns := additionalPrinterColumns
+	if len(printerColumns) == 0 {
+		printerColumns = defaultAdditionalPrinterColumns
+	}


The newCRD function has redundant logic. It checks if len(printerColumns) == 0 and assigns defaults, but SynthesizeCRD already calls newCRDAdditionalPrinterColumns which handles empty columns by returning defaults. This check on lines 41-45 is now redundant and creates duplicate logic that could lead to inconsistencies if only one location is updated in the future.

Copilot · 2025-12-29T19:35:24Z

+      jsonPath: .spec.name
+      name: State
+      type: string
+    # Default Synced column should remain


The comment says "Default Synced column should remain" but the column name is actually "Ready" (line 26). This is inconsistent and misleading - the comment should say "Default Ready column should remain" to match the actual column name.

Suggested change

# Default Synced column should remain

# Default Ready column should remain

Copilot · 2025-12-29T19:35:24Z

+	// Validation is a list of validation rules that are applied to the
+	// resourcegraphdefinition.
+	Validation []Validation `json:"validation,omitempty"`


The Validation field and its related changes appear unrelated to the printer column policy feature described in the PR. This should either be part of a separate PR or explicitly mentioned in the PR description as an additional feature being added.

Suggested change

// Validation is a list of validation rules that are applied to the

// resourcegraphdefinition.

Validation []Validation `json:"validation,omitempty"`

+1 to that. please get rid of validation

pls check the changes made

Copilot · 2025-12-29T19:35:24Z

 )

 // SynthesizeCRD generates a CustomResourceDefinition for a given API version and kind
 // with the provided spec and status schemas~


The comment has a trailing tilde character (~) at the end of line 28 which appears to be a typo. This should be removed.

Suggested change

// with the provided spec and status schemas~

// with the provided spec and status schemas

Copilot · 2025-12-29T19:35:25Z

-				"state": {
+				stateProperty: {
 					Type:        "string",
 					Description: "Custom state filed",


Typo: "filed" should be "field" in the description text.

Suggested change

Description: "Custom state filed",

Description: "Custom state field",

Copilot · 2025-12-29T19:35:25Z

+// User columns override defaults when they share the same Name.
+// New user columns are appended after the defaults.
+func newCRDAdditionalPrinterColumns(
+	policy v1alpha1.AdditionalPrinterColumnPolicy,


The policy parameter is declared but never used in the function logic. The comment on line 118 acknowledges this ("The policy parameter is currently unused as only 'Add' behavior is supported"), but this creates dead code and misleads callers into thinking the policy will be respected. Either implement the policy logic or remove the parameter if "Add" is the only behavior currently supported.

jakobmoellerdev · 2025-12-30T08:09:38Z

+)
+
+// AdditionalPrinterColumnPolicy defines how additional printer columns are applied to the generated CustomResourceDefinition (CRD).
+type AdditionalPrinterColumnPolicy string


leftover from spec change I believe

is the policy not needed I thought as you said ( or ig amine said) this needs to be implemented as a policy can you pls confirm?

#629 (comment)

jakobmoellerdev · 2025-12-30T08:09:54Z

+	//
+	// +kubebuilder:validation:Enum=Add
+	// +kubebuilder:default=Add
+	AdditionalPrinterColumnPolicy AdditionalPrinterColumnPolicy `json:"additionalPrinterColumnPolicy,omitempty"`


should be removed as discussed in comm call

sorry was not present in the call so the policy needs to be removed?

#629 (comment)

yes correct, if we only have one policy there is no need for a field anymore!

so will we be using string instead of policy ("Add")? and also then we have to use string literals in tests instead of custom types

Oh now I get it I think

Add AdditionalPrinterColumnPolicy field to control how custom printer columns are merged with defaults. Currently only "Add" policy is supported, which merges user columns with default columns (State, Ready, Age), with user columns overriding defaults by Name. - Add AdditionalPrinterColumnPolicy type with "Add" enum value - Implement merge logic in newCRDAdditionalPrinterColumns - User columns override defaults when Name matches - New columns are appended after defaults - Default behavior: always merge (no replace option) Includes comprehensive unit tests (87.4% coverage) and e2e tests. Signed-off-by: Yash Pal <yashpal2104@gmail.com>

k8s-ci-robot · 2026-01-23T02:03:58Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

a-hilaly · 2026-03-03T09:03:32Z

thanks for your contribution! we decided we no longer want to expose this parameter in the RGD. at least in v1alpha1.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 24, 2025

k8s-ci-robot requested review from jakobmoellerdev and jlbutler November 24, 2025 20:19

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 24, 2025

jakobmoellerdev reviewed Nov 25, 2025

View reviewed changes

jakobmoellerdev suggested changes Nov 25, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2025

Copilot AI review requested due to automatic review settings December 29, 2025 19:28

yashpal2104 force-pushed the add-printer-column-policy-clean branch from a835783 to cb2eb79 Compare December 29, 2025 19:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 29, 2025

Copilot started reviewing on behalf of yashpal2104 December 29, 2025 19:29 View session

Copilot AI reviewed Dec 29, 2025

View reviewed changes

yashpal2104 force-pushed the add-printer-column-policy-clean branch 3 times, most recently from 2250f0a to f738e6d Compare December 30, 2025 07:34

jakobmoellerdev suggested changes Dec 30, 2025

View reviewed changes

yashpal2104 force-pushed the add-printer-column-policy-clean branch 2 times, most recently from e2f5116 to e5593bb Compare January 2, 2026 11:30

yashpal2104 force-pushed the add-printer-column-policy-clean branch from e5593bb to d3249c2 Compare January 4, 2026 07:45

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2026

a-hilaly closed this Mar 3, 2026

	# Default Synced column should remain
	# Default Ready column should remain

	// Validation is a list of validation rules that are applied to the
	// resourcegraphdefinition.
	Validation []Validation `json:"validation,omitempty"`

	// with the provided spec and status schemas~
	// with the provided spec and status schemas

	Description: "Custom state filed",
	Description: "Custom state field",

Conversation

yashpal2104 commented Nov 24, 2025

Uh oh!

k8s-ci-robot commented Nov 24, 2025

Uh oh!

k8s-ci-robot commented Nov 24, 2025

Uh oh!

yashpal2104 commented Nov 25, 2025

Uh oh!

k8s-ci-robot commented Nov 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobmoellerdev left a comment

Choose a reason for hiding this comment

Uh oh!

yashpal2104 commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashpal2104 Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 23, 2026

Uh oh!

a-hilaly commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

yashpal2104 commented Nov 25, 2025 •

edited

Loading

yashpal2104 Dec 30, 2025 •

edited

Loading