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

Update ClusterGroup singular name #2484

Merged
merged 1 commit into from
Aug 9, 2021
Merged

Update ClusterGroup singular name #2484

merged 1 commit into from
Aug 9, 2021

Conversation

abhiraut
Copy link
Contributor

@abhiraut abhiraut commented Jul 28, 2021 •
edited
Loading

This PR updates the singular name of the ClusterGroup CRD from group to clustergroup. After this update, the kubectl get group command will not return any ClusterGroups. However, the most common way and the documented way to return
ClusterGroups using kubectl have been to use kubectl get cg or kubectl get clustergroups.

Signed-off-by: abhiraut rauta@vmware.com

@abhiraut
Update ClusterGroup singular name
960ece2 
This PR updates the singular name of the ClusterGroup
CRD from "group" to "clustergroup". After this update,
the "kubectl get group" command will not return any
ClusterGroups. However, the most common way and the
documented way to return ClusterGroups using kubectl
have been to use "kubectl get cg" or "kubectl get clustergroups".

Signed-off-by: abhiraut <rauta@vmware.com>
@abhiraut
Copy link
Contributor Author

i ack that this PR may be rejected.

Confirming whether aliasing is the only use for "singular" field. Even though the documented way and most used way is to use "clustergroups" or "cg" to return ClusterGroups, astute users of CRDs may be using "group" to return ClusterGroups. This is bad, but how bad?

Alternative is to continue with this term and use a new term for Namespaced Group CRD

@abhiraut abhiraut requested a review from Dyanngg July 28, 2021 00:51
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021 •
edited
Loading

Codecov Report

Merging #2484 (960ece2) into main (6f33da3) will increase coverage by 22.99%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2484       +/-   ##
===========================================
+ Coverage   42.02%   65.02%   +22.99%     
===========================================
  Files         148      284      +136     
  Lines       18131    26068     +7937     
===========================================
+ Hits         7620    16951     +9331     
+ Misses       9828     7530     -2298     
- Partials      683     1587      +904
Flag Coverage Δ
e2e-tests 55.98% <ø> (?)
kind-e2e-tests 47.02% <ø> (?)
unit-tests 42.01% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/egress/ipallocator/allocator.go 67.82% <0.00%> (-15.16%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 77.64% <0.00%> (-13.79%) ⬇️
pkg/apis/controlplane/v1beta1/conversion.go 72.44% <0.00%> (-11.09%) ⬇️
pkg/controller/egress/controller.go 76.76% <0.00%> (-10.44%) ⬇️
pkg/apiserver/handlers/endpoint/handler.go 61.11% <0.00%> (-9.48%) ⬇️
pkg/agent/config/traffic_encap_mode.go 91.30% <0.00%> (-8.70%) ⬇️
pkg/ovs/ovsconfig/errors.go 7.14% <0.00%> (-7.15%) ⬇️
pkg/agent/util/iptables/lock.go 75.00% <0.00%> (-6.82%) ⬇️
pkg/agent/apiserver/handlers/ovsflows/handler.go 67.80% <0.00%> (-4.85%) ⬇️
pkg/cni/client.go 71.15% <0.00%> (-4.36%) ⬇️
... and 268 more

jianjuns
jianjuns reviewed Jul 28, 2021
View reviewed changes
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

I feel not a big problem to drop "kubectl get group".

@abhiraut
Copy link
Contributor Author

abhiraut commented Jul 28, 2021 •
edited
Loading

i ack that this PR may be rejected.

Confirming whether aliasing is the only use for "singular" field.

Based on conversation I had with folks savvy with CRD code, this should be the only drawback. ie kubectl get with singular name will stop working.

Even though the documented way and most used way is to use "clustergroups" or "cg" to return ClusterGroups, astute users of CRDs may be using "group" to return ClusterGroups. This is bad, but how bad?

Alternative is to continue with this term and use a new term for Namespaced Group CRD

@abhiraut
Copy link
Contributor Author

/test-all

@abhiraut
Copy link
Contributor Author

abhiraut commented Aug 6, 2021

@antoninbas @tnqn any feedback?

@abhiraut abhiraut added this to the Antrea v1.3 release milestone Aug 6, 2021
tnqn
tnqn approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas antoninbas merged commit fe4457f into antrea-io:main Aug 9, 2021
annakhm pushed a commit to annakhm/antrea that referenced this pull request Aug 16, 2021
@abhiraut @annakhm
Update ClusterGroup singular name (antrea-io#2484)
9db5d53 
This PR updates the singular name of the ClusterGroup
CRD from "group" to "clustergroup". After this update,
the "kubectl get group" command will not return any
ClusterGroups. However, the most common way and the
documented way to return ClusterGroups using kubectl
have been to use "kubectl get cg" or "kubectl get clustergroups".

Signed-off-by: abhiraut <rauta@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@jianjuns jianjuns jianjuns approved these changes

@antoninbas antoninbas Awaiting requested review from antoninbas

@Dyanngg Dyanngg Awaiting requested review from Dyanngg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Antrea v1.3 release
Development

Successfully merging this pull request may close these issues.
5 participants
@abhiraut @codecov-commenter @tnqn @jianjuns @antoninbas