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

Fix ClusterSet output when status is empty #4174

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

luolanzone
Copy link
Contributor

@luolanzone luolanzone commented Aug 30, 2022

When a ClusterSet is created in the leader cluster, there might be
no status yet, then antctl mc get clustersets -A will skip print the
ClusterSet when the status is empty.
Fix it by passing an empty status and condition.

The old output:

CLUSTER-ID        NAMESPACE                CLUSTER-SET-ID  TYPE     STATUS REASON
test-cluster-east kube-system              test-clusterset IsLeader True   <NONE>
test-cluster-east kube-system              test-clusterset Ready    True   <NONE>

The new output:

CLUSTER-ID        NAMESPACE                CLUSTER-SET-ID  TYPE     STATUS REASON
<NONE>            antrea-multicluster-test test-clusterset <NONE>   <NONE> <NONE>
test-cluster-east kube-system              test-clusterset IsLeader True   <NONE>
test-cluster-east kube-system              test-clusterset Ready    True   <NONE>

Signed-off-by: Lan Luo [email protected]

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #4174 (66c90f2) into main (3714619) will increase coverage by 4.32%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4174      +/-   ##
==========================================
+ Coverage   56.09%   60.42%   +4.32%     
==========================================
  Files         371      383      +12     
  Lines       53327    54261     +934     
==========================================
+ Hits        29913    32785    +2872     
+ Misses      21071    19028    -2043     
- Partials     2343     2448     +105     
Flag Coverage Δ
integration-tests 34.98% <ø> (+<0.01%) ⬆️
kind-e2e-tests 48.25% <ø> (+5.20%) ⬆️
unit-tests 41.66% <81.81%> (+1.39%) ⬆️
Impacted Files Coverage Δ
pkg/antctl/raw/multicluster/get/resourceexport.go 22.72% <0.00%> (ø)
pkg/antctl/raw/multicluster/get/resourceimport.go 21.83% <0.00%> (ø)
pkg/antctl/raw/multicluster/get/clusterset.go 83.11% <71.42%> (+63.76%) ⬆️
pkg/antctl/raw/multicluster/get/output.go 85.00% <100.00%> (+85.00%) ⬆️
pkg/antctl/transform/clusterset/transform.go 100.00% <100.00%> (+100.00%) ⬆️
pkg/agent/flowexporter/exporter/exporter.go 67.82% <0.00%> (-10.15%) ⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go 73.59% <0.00%> (-9.13%) ⬇️
pkg/flowaggregator/options/options.go 31.25% <0.00%> (-5.49%) ⬇️
pkg/controller/networkpolicy/tier.go 50.00% <0.00%> (-5.00%) ⬇️
pkg/flowaggregator/flowaggregator.go 65.99% <0.00%> (-4.06%) ⬇️
... and 109 more

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Aug 30, 2022
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.

@luolanzone : could you provide an example output (with both empty and non-empty Status)?

@luolanzone
Copy link
Contributor Author

@jianjuns the old and new output are added in the summary.

@jianjuns
Copy link
Contributor

@luolanzone : "Empty_ClusterSet" looks strange. Could we not set it and just show "NONE"?

@luolanzone
Copy link
Contributor Author

Sure, let me change it to NONE.

@jianjuns
Copy link
Contributor

Do we need to set the filed to "NONE", or antctl will print "NONE" when a filed is not set?

@luolanzone
Copy link
Contributor Author

luolanzone commented Aug 31, 2022

Antctl will print "NONE" by default when a field is not set.

@luolanzone
Copy link
Contributor Author

@jianjuns, the code is updated, the new output will be like:

CLUSTER-ID        NAMESPACE                CLUSTER-SET-ID  TYPE     STATUS REASON
<NONE>            antrea-multicluster-test test-clusterset <NONE>   <NONE> <NONE>
test-cluster-east kube-system              test-clusterset IsLeader True   <NONE>
test-cluster-east kube-system              test-clusterset Ready    True   <NONE>

jianjuns
jianjuns previously approved these changes Aug 31, 2022
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.

LGTM

@jianjuns
Copy link
Contributor

/skip-all

@jianjuns
Copy link
Contributor

@luolanzone : this PR does not meet the 70% coverage target. Could you check any way to improve coverage?

@luolanzone
Copy link
Contributor Author

Hi @jianjuns I am trying to improve the unit test for antctl mc in another PR #4183. For this specific change, I haven't figured out an efficient way to test antctl mc commands. Do you think it's Ok to merge this first?

@luolanzone
Copy link
Contributor Author

Hi @jianjuns after refactored a few codes, I added some unit tests for the antctl get clusterset. Could you help to check? thanks.

jianjuns
jianjuns previously approved these changes Sep 1, 2022
@jianjuns
Copy link
Contributor

jianjuns commented Sep 1, 2022

@luolanzone : this PR does not reach the target coverage still.

When a ClusterSet is created in the leader cluster, there might be
no status yet, then `antctl mc get clustersets -A` will skip print the
ClusterSet when the status is empty.
Fix it by passing an empty status and condition.

Signed-off-by: Lan Luo <[email protected]>
@jianjuns
Copy link
Contributor

jianjuns commented Sep 2, 2022

/skip-all

@jianjuns jianjuns merged commit 53bb0c3 into antrea-io:main Sep 2, 2022
@luolanzone luolanzone deleted the fix-mc-clustersets branch September 2, 2022 05:50
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
After a ClusterSet is just created in the leader cluster, it might have
no status yet, then `antctl mc get clustersets -A` will skip print the
ClusterSet when its status is empty.
Fix it by passing an empty status and condition.

Signed-off-by: Lan Luo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants