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.

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

Improve CLI user experience for "kubectl get ciliumendpoints" #28744

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

Iiqbal2000
Copy link
Contributor

@Iiqbal2000 Iiqbal2000 commented Oct 23, 2023

Fix the empty columns by adding priority fields to columns that may be empty.

Fixes: #27459

Hide empty columns by default in "kubectl get ciliumendpoints" output

@Iiqbal2000 Iiqbal2000 requested review from a team as code owners October 23, 2023 14:14
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 23, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 23, 2023
@aanm aanm requested review from joestringer and aanm and removed request for nathanjsweet October 23, 2023 21:37
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

In terms of hiding columns, I think this makes sense. I'll leave the review of how this becomes reality to Andre; this is all magic to me, and I don't understand how changing a comment makes the code different :)

pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 23, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 23, 2023
@joestringer
Copy link
Member

Ah CI gives the next steps, see the failed job for Go related checks.

@Iiqbal2000
Copy link
Contributor Author

Ah CI gives the next steps, see the failed job for Go related checks.

It seems I missed a step

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

@Iiqbal2000 please run `make generate-k8s-api && make manifests' and submit your changes

@aanm
Copy link
Member

aanm commented Nov 3, 2023

I've marked the Pull Request as draft. Once you have finished up working on it feel free to click on the "Ready for review" button.

image

Thank you

@aanm aanm marked this pull request as draft November 3, 2023 08:32
@Iiqbal2000
Copy link
Contributor Author

I've marked the Pull Request as draft. Once you have finished up working on it feel free to click on the "Ready for review" button.

image

Thank you

Thanks for the note! I'm a bit swamped lately, I'll get back to the PR and ping you once I've hit 'Ready for review'

@Iiqbal2000 Iiqbal2000 marked this pull request as ready for review November 14, 2023 00:46
@lmb lmb requested a review from joestringer November 22, 2023 12:24
@joestringer joestringer force-pushed the pr/Iiqbal2000/fix-empty-columns branch from 1ebc6d9 to e0b19af Compare November 27, 2023 23:05
Signed-off-by: Iiqbal2000 <iqbalhafizh2000@gmail.com>
@joestringer joestringer force-pushed the pr/Iiqbal2000/fix-empty-columns branch from e0b19af to a1e673f Compare November 27, 2023 23:25
@joestringer
Copy link
Member

I rebased & squashed the commits to tidy it up and to address one of the CI failures. Will trigger full testsuite after the smoke tests go through.

@joestringer
Copy link
Member

/test

@Iiqbal2000
Copy link
Contributor Author

Thank you @joestringer
I forgot to rebase it. is there any action I need to take?

@joestringer
Copy link
Member

No action is needed at this time, just looking for the CI tests to complete. I don't expect that this change should impact CI at all, hopefully CI doesn't rely on the textual output of this command.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Change LGTM. Assuming it passes CI this should be good to merge.

@joestringer joestringer changed the title fix empty colomns on CNP CRD Improve CLI user experience for "kubectl get ciliumendpoints" Nov 27, 2023
@youngnick
Copy link
Contributor

Looks like the external workloads test failed, I can't see any errors aside from not being able to find a specific pod in the connectivity tests, I'll rerun.

@youngnick
Copy link
Contributor

/ci-external-workloads

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 28, 2023
@aanm aanm enabled auto-merge November 28, 2023 08:53
@aanm aanm added this pull request to the merge queue Nov 28, 2023
Merged via the queue into cilium:main with commit 8072f60 Nov 28, 2023
61 checks passed
@joestringer
Copy link
Member

Thanks for your contribution @Iiqbal2000 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFP: Recreate CNP CRDs based on feature enablement
4 participants