fix: adds an additional printer column annotation #6390

shawkins · 2024-09-30T14:59:56Z

Description

closes: #3069

I believe the is the most straight-foward appraoch to adding general additional printer columns.

Other thoughts:

we could reuse the existing PrinterColumn annotation. However I didn't like about that is that it mixes the usage models, so the javadocs / validations would need to be more extensive.
allow the annotation to appear in other places. I think we'd at least need to auto-prefix the paths with the current path within the structure - we do have that as we're navigating over the object model. This can be easily added later if needed.
it's not being extensively validated. For example we could check that the path provided is valid / actually exists.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

baloo42 · 2024-09-30T18:00:08Z

👍 worked on the same last weekend, but no tests yet: baloo42#10

Looks very similar, but I used enums for format and type. I think using enums is better here, because not all formats and types are allowed to be used here. What do you think on enums?

we could reuse the existing PrinterColumn annotation. However I didn't like about that is that it mixes the usage models, so the javadocs / validations would need to be more extensive.

I also wouldn't like it if we reuse the PrinterColumn. Too many differences to explain.

allow the annotation to appear in other places. I think we'd at least need to auto-prefix the paths with the current path within the structure - we do have that as we're navigating over the object model. This can be easily added later if needed.

I think there are pro's and con's about this. It is possible and might be explainable
but if we don't implement it, the difference would be better:

@PrinterColumn Uses magic to find the path
@AdditionalPrinterColumn No magic involved, you as user are fully responsible.

👍 to postpone this to next iteration at least.

it's not being extensively validated. For example we could check that the path provided is valid / actually exists.

👍 same, as a above. If we validate it, it must support all cases because CRDGenerator is now fully responsible instead of the user.

shawkins · 2024-09-30T18:36:59Z

worked on the same last weekend, but no tests yet: baloo42#10

Whoops, sorry for the duplication of effort.

What do you think on enums?

My only thought is the mismatch with the PrinterColumn annotation. To evolve the other annotation we'd have to accept Object for a major release before restricting back to an enum.

I've added your enums to the this pr.

to postpone this to next iteration at least.

I'm guessing that it may never be needed - what are the chances that someone wants the same set of printer columns in two different CRDs based upon a common object?

baloo42 · 2024-09-30T19:21:33Z

worked on the same last weekend, but no tests yet: baloo42#10

Whoops, sorry for the duplication of effort.

no worries, I havn't published or noted it somewhere. My fault ^^

What do you think on enums?

My only thought is the mismatch with the PrinterColumn annotation. To evolve the other annotation we'd have to accept Object for a major release before restricting back to an enum.

I've added your enums to the this pr.

👍
For @PrinterColumn we could also phase it out:

Add additional enum param formatStrict, deprecate format
Replace format with enum param, deprecate formatStrict
Remove formatStrict

This would require the same amount of major releases but avoids Object or do I miss something?

to postpone this to next iteration at least.

I'm guessing that it may never be needed - what are the chances that someone wants the same set of printer columns in two different CRDs based upon a common object?

Full ack. There is no need for additional ways. If someone wants to configure a printer column deep in the hierarchy and maybe shared between CRDs, he should and can already use PrinterColumn.

baloo42 · 2024-09-30T19:31:20Z

...r-annotations/src/main/java/io/fabric8/crd/generator/annotation/AdditionalPrinterColumn.java

+    FLOAT("float"),
+    DOUBLE("double"),
+    BYTE("byte"),
+    BINARY("binary"),


Sorry, havn't checked it twice: binary is not allowed. Only byte.

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#format

Updated. Altenatively, I'm fine if you want to take over ownership of this feature from your branch.

If you want, you can assign it to me but it doesn't make a lot sense too me as yours is almost ready.
Your approach is able collect the path, my approach tries to distinguesh between top level annotations and annotations in the hierarchy a little bit more but without the ability to collect the path.
So it depends on which direction we want to go.

shawkins · 2024-10-01T15:13:14Z

This would require the same amount of major releases but avoids Object or do I miss something?

Actually Object won't work at all for an annotation property. So it either needs to be a breaking change for 7.0 or done over 3 major releases with the additional field. I'd opt for just doing the breaking change instead.

...r-annotations/src/main/java/io/fabric8/crd/generator/annotation/AdditionalPrinterColumn.java

baloo42

FYI: I also tried to add an approval test (only for @PrinterColumns) to compare if something breaks with this change.

Your change in this PR looks good, but it seems there is a bug in api-v1 which blocks us to add an approval test at the moment:

@Data
public class PrinterColumnSpec {
  @PrinterColumn
  private String fromLevel0;

  private DeepLevel1 deepLevel1;

  @Data
  public static class DeepLevel1 {

    @PrinterColumn
    private Integer fromLevel1;
  }
}

Result with api-v2 (this MR):

    - jsonPath: ".spec.deepLevel1.fromLevel1"
      name: "FROMLEVEL1"
      priority: 0
      type: "integer"
    - jsonPath: ".spec.fromLevel0"
      name: "FROMLEVEL0"
      priority: 0
      type: "string"

Result with api-v1:

    - jsonPath: ".spec.deepLevel1.fromLevel1"
      name: "FROMLEVEL1"
      priority: 0
      type: "integer"
    - jsonPath: ".spec.fromLevel0"
      name: "FROMLEVEL0"
      priority: 0
      type: "string"
    - jsonPath: ".spec.fromLevel1"
      name: "FROMLEVEL1"
      priority: 0
      type: "integer"

Should we fix this within this PR or create another one and add an approval test later?

...r-annotations/src/main/java/io/fabric8/crd/generator/annotation/AdditionalPrinterColumn.java

baloo42 · 2024-10-15T22:05:50Z

...generator/api-v2/src/main/java/io/fabric8/crdv2/generator/AbstractCustomResourceHandler.java

-      } else if ("string".equals(type) && "date".equals(property.schema.getFormat())) {
-        type = "date";
-      }
+      if (property.annotation instanceof AdditionalPrinterColumn) {


At the moment the @AdditionalPrinterColum is collected from everywhere in the spec or status type. Is this intended?

We should either clarify this in the description of the annotation or pick it only from the custom resource class.

At the moment the @AdditionalPrinterColum is collected from everywhere in the spec or status type.

It should not be: https://github.com/fabric8io/kubernetes-client/pull/6390/files#diff-e5491c474236ab12f8e6f5d9895464ecf77754b17ddb0e182682add6db8620ceR146

We should either clarify this in the description of the annotation or pick it only from the custom resource class.

It is documented as intended for the root: https://github.com/fabric8io/kubernetes-client/pull/6390/files#diff-669d381122146d717c360f17fcfe5eeaab88b5c84dd4bfcf81b6de71aa5a75c1R25

Sorry, seems like it was a problem on my side. I have checked out your branch again and could not reproduce it. Looks fine now!

...generator/api-v2/src/main/java/io/fabric8/crdv2/generator/AbstractCustomResourceHandler.java

shawkins · 2024-10-16T00:50:17Z

Should we fix this within this PR or create another one and add an approval test later?

Do you mean fix the v1 implementation? I don't think we need to bother doing that.

closes: fabric8io#3069 Signed-off-by: Steve Hawkins <[email protected]>

based upon Bernhard Strähle's review Signed-off-by: Steve Hawkins <[email protected]>

Signed-off-by: Steve Hawkins <[email protected]>

baloo42 · 2024-10-16T16:06:36Z

Should we fix this within this PR or create another one and add an approval test later?

Do you mean fix the v1 implementation? I don't think we need to bother doing that.

Yes I mean fixing v1 because of differences in the approval tests. If we don't want to fix v1, then we should start splitting the approval tests so that we can add tests for new or breaking features. (not in this PR)

Quality Gate failed

Failed conditions
78.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

shawkins requested review from andreaTP, metacosm, manusa, oscerd and rohanKanojia as code owners September 30, 2024 14:59

baloo42 suggested changes Sep 30, 2024

View reviewed changes

shawkins force-pushed the iss3069 branch from f5b2c89 to e9383e0 Compare October 1, 2024 15:14

baloo42 mentioned this pull request Oct 1, 2024

CRD generator implementation that supports Jakarta and Swagger #6315

Open

11 tasks

metacosm reviewed Oct 1, 2024

View reviewed changes

...r-annotations/src/main/java/io/fabric8/crd/generator/annotation/AdditionalPrinterColumn.java Show resolved Hide resolved

metacosm reviewed Oct 1, 2024

View reviewed changes

...r-annotations/src/main/java/io/fabric8/crd/generator/annotation/AdditionalPrinterColumn.java Show resolved Hide resolved

baloo42 mentioned this pull request Oct 5, 2024

CRD-Generator: Selectable fields #6392

Open

MichaelMorrisEst mentioned this pull request Oct 14, 2024

Define additional printer columns in the KafkaAccess CRD strimzi/kafka-access-operator#53

Open

manusa requested review from metacosm and baloo42 October 15, 2024 10:59

baloo42 suggested changes Oct 15, 2024

View reviewed changes

baloo42 mentioned this pull request Oct 15, 2024

CRD-Generator: Change format in @PrinterColum to enum #6455

Open

shawkins added 3 commits October 16, 2024 08:12

fix: adds an additional printer column annotation

0fab4f7

closes: fabric8io#3069 Signed-off-by: Steve Hawkins <[email protected]>

switching to enums for format and type

bd559a8

based upon Bernhard Strähle's review Signed-off-by: Steve Hawkins <[email protected]>

updating to jsonPath

5b79029

Signed-off-by: Steve Hawkins <[email protected]>

shawkins force-pushed the iss3069 branch from e9383e0 to 5b79029 Compare October 16, 2024 12:14

baloo42 approved these changes Oct 16, 2024

View reviewed changes

manusa added this to the 7.0.0 milestone Oct 18, 2024 — with automated-tasks

manusa approved these changes Oct 18, 2024

View reviewed changes

manusa merged commit 66dbab1 into fabric8io:main Oct 18, 2024
19 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: adds an additional printer column annotation #6390

fix: adds an additional printer column annotation #6390

shawkins commented Sep 30, 2024

baloo42 commented Sep 30, 2024

shawkins commented Sep 30, 2024

baloo42 commented Sep 30, 2024

baloo42 Sep 30, 2024

shawkins Oct 1, 2024

baloo42 Oct 1, 2024

shawkins commented Oct 1, 2024

baloo42 left a comment

baloo42 Oct 15, 2024

shawkins Oct 16, 2024

baloo42 Oct 16, 2024

shawkins commented Oct 16, 2024

baloo42 commented Oct 16, 2024

shawkins commented Oct 16, 2024

manusa left a comment

sonarcloud bot commented Oct 18, 2024

fix: adds an additional printer column annotation #6390

fix: adds an additional printer column annotation #6390

Conversation

shawkins commented Sep 30, 2024

Description

Type of change

Checklist

baloo42 commented Sep 30, 2024

shawkins commented Sep 30, 2024

baloo42 commented Sep 30, 2024

baloo42 Sep 30, 2024

Choose a reason for hiding this comment

shawkins Oct 1, 2024

Choose a reason for hiding this comment

baloo42 Oct 1, 2024

Choose a reason for hiding this comment

shawkins commented Oct 1, 2024

baloo42 left a comment

Choose a reason for hiding this comment

baloo42 Oct 15, 2024

Choose a reason for hiding this comment

shawkins Oct 16, 2024

Choose a reason for hiding this comment

baloo42 Oct 16, 2024

Choose a reason for hiding this comment

shawkins commented Oct 16, 2024

baloo42 commented Oct 16, 2024

shawkins commented Oct 16, 2024

manusa left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 18, 2024

Quality Gate failed