Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: #5339 include priority field in the PrinterColumn #5340

Merged
merged 5 commits into from
Jul 20, 2023
Merged

feat: #5339 include priority field in the PrinterColumn #5340

merged 5 commits into from
Jul 20, 2023

Conversation

bachmanity1
Copy link
Contributor

Description

resolves #5339

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

@bachmanity1 bachmanity1 changed the title add priority field to the PrinterColumn feat: #5339 include priority field in the PrinterColumn Jul 19, 2023
andreaTP
andreaTP approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fast! LGTM as soon as we have a green CI 🙂

@rohanKanojia
Copy link
Member

@bachmanity1 : Could you please run mvn spotless:apply to fix style errors?

@rohanKanojia
Copy link
Member

@bachmanity1 : I think we also need to update documentation in https://github.com/fabric8io/kubernetes-client/blob/main/doc/CRD-generator.md

andreaTP
andreaTP reviewed Jul 19, 2023
View reviewed changes
doc/CRD-generator.md
@@ -444,6 +465,7 @@ The CRD generator will add the additional `labels`:
| `io.fabric8.crd.generator.annotation.SchemaSwap` | Similar to SchemaFrom, but can be applied at any point in the class hierarchy |
| `io.fabric8.crd.generator.annotation.Annotations` | Additional `annotations` in `metadata` |
| `io.fabric8.crd.generator.annotation.Labels` | Additional `labels` in `metadata` |
| `io.fabric8.kubernetes.model.annotation.PrinterColumn` | Customize columns shown by the `kubectl get` command |
Copy link
Member

@andreaTP andreaTP Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for adding this entry!
I actually think that this is a leftover and we should move this to the crd-generator module.
(we can tackle this issue outside the scope of this PR)

cc. @manusa @shawkins @metacosm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should go through something like this:
bachmanity1#1

to actually remove the annotation.
Since it was completely undocumented, I would get this change in before we release the next 6.8 if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I handle it in a separate PR?

@bachmanity1
Copy link
Contributor Author

@andreaTP How can I resolved failing sonarcloud tests? Can you please have a look?

@andreaTP
Copy link
Member

andreaTP commented Jul 19, 2023 •
edited
Loading

@bachmanity1 the comments from Sonarcloud are not really interesting, ignore them unless someone comes up with a better advise

@bachmanity1
Copy link
Contributor Author

@bachmanity1 the comments from Sonarcloud are not really interesting, ignore them unless someone comes up with a better advise

also, it looks like you've canceled build checks, can I ask why?

@andreaTP
Copy link
Member

it looks like you've canceled build checks, can I ask why?

I have attempted to re-run them, something acted-up ... anyhow, re-triggered now 👍

andreaTP
andreaTP reviewed Jul 19, 2023
View reviewed changes
Copy link
Member

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A last note on the implementation and lgtm.
Let's coordinate with @manusa on how to roll out the deprecation.

...i/src/main/java/io/fabric8/crd/generator/v1/decorator/AddAdditionPrinterColumnDecorator.java Outdated
@@ -113,11 +117,12 @@ public boolean equals(Object obj) {
return false;
} else if (!type.equals(other.type))
return false;
return true;
return priority != other.priority;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this before, I think this should be ==

@bachmanity1 bachmanity1 force-pushed the extend-printer-column branch from 32dc5fc to 5bee175 Compare July 19, 2023 21:27
@rohanKanojia
Copy link
Member

@bachmanity1 : tests seem to be failing, could you please take a look into this?

@bachmanity1
Copy link
Contributor Author

@bachmanity1 : tests seem to be failing, could you please take a look into this?

@rohanKanojia it's weird because all tests run successfully on my machine (Java 11). The failing assertion doesn't look to be related to the changes I've made, could you please try to re-run failing tests?

@bachmanity1 bachmanity1 force-pushed the extend-printer-column branch from 5bee175 to b3b09f1 Compare July 20, 2023 05:23
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 2 Code Smells

50.0% 50.0% Coverage
23.5% 23.5% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@manusa
Copy link
Member

manusa commented Jul 20, 2023

A last note on the implementation and lgtm. Let's coordinate with @manusa on how to roll out the deprecation.

As agreed internally, let's merge this in first (currently reviewing), and then @andreaTP will create a new PR for the deprecation.

@manusa manusa added this to the 6.8.0 milestone Jul 20, 2023
@manusa manusa merged commit 0e7a32c into fabric8io:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohanKanojia rohanKanojia rohanKanojia approved these changes

@andreaTP andreaTP andreaTP approved these changes

@oscerd oscerd oscerd approved these changes

@manusa manusa manusa approved these changes

@shawkins shawkins Awaiting requested review from shawkins shawkins is a code owner

@sunix sunix Awaiting requested review from sunix

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.8.0
Development

Successfully merging this pull request may close these issues.

Include priority field in the PrinterColumn annotation
5 participants
@bachmanity1 @rohanKanojia @andreaTP @manusa @oscerd