Move "printer column" options under the same generator config struct #82
Conversation
|
@jaypipes @RedbackThomson i'm aware that this is a breaking change, but TBH this is necessary to minimize damage (very verbose and non-understandable config files) in the future. |
| name: DeploymentConfig | ||
| Description: | ||
| is_printable: true | ||
| print: {} |
There was a problem hiding this comment.
I'm not a huge fan of this, as I feel it is overly verbose and easy to mess up :)
How about we just remove the IsPrintable field and keep the PrintName field, but make it a *string instead of string and just require any printable column to have PrintName set to a string value (even if that string value is the same name as the field).
There was a problem hiding this comment.
The problem is that there are other configuration we want to add the PrinterColumn generator. Like order, priority, type and probably more in the future.
So long term, instead of having something like:
resources:
Deployment:
fields:
DeploymentGroupName:
is_printable: true
print_name: DeploymentGroup
print_order: 3
print_priority: 1
DeploymentConfigName:
is_printable: true
print_name: DeploymentConfig
print_order: 2
print_priority: 1
print_type: string
we can have something like:
resources:
Deployment:
fields:
DeploymentGroupName:
print:
name: DeploymentGroup
order: 3
priority: 1
DeploymentConfigName:
print:
name: DeploymentConfig
order: 2
priority: 1
type: stringThe second option is way less verbose and easier/clearer to read/understand IMO.
Initially we used to have one config field under "is_printable" under `ackconfig.FieldConfig` to instruct the code generator to generate kubebuilder marker comments in order to include the field in `kubectl get` response. Time passed and we are in a situation where want to tune more the way we print these fields and extend the `kubebuilder:printcolumns` comment markers to include options like `priority`, `order` and default columns. This patch reworks the field configuration structure and prepare the laying ground for future print config fields. This patch will simplify the realization of the following issues: * aws-controllers-k8s/community#821 * aws-controllers-k8s/community#822 * aws-controllers-k8s/community#823
| name: DeploymentConfig | ||
| Description: | ||
| is_printable: true | ||
| print: {} |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes 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:
Approvers can indicate their approval by writing |
Recently we merged a PR that changed `is_printable` field types to struct types, mainly to allow users to set other printing parameters: like type, priority and order. Ref: aws-controllers-k8s/code-generator#82
3 participants
Issue N/A
Initially we used to have one config field ("is_printable") under
ackconfig.FieldConfigto instruct the code generator to generatekubebuilder marker comments in order to include the field in
kubectl getresponse.
Time passed and we are in a situation where want to tune more the way
we print these fields and extend the
kubebuilder:printcolumnscommentmarkers to include options like
priority,orderand default columns.This patch reworks the field configuration structure and prepare the
laying ground for future print config fields.
This patch will simplify the realization of the following issues:
ack-generateto overridekubebuilder:printcolumndefault priority community#821ack-generateto generate an extra comment marker for anAGEcolumn community#822+kubebuilder:printcolumn:name"sort by" function community#823By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.