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 nil error issue #4154

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Fix nil error issue #4154

merged 1 commit into from
Aug 29, 2022

Conversation

luolanzone
Copy link
Contributor

@luolanzone luolanzone commented Aug 26, 2022

admission.Errored doesn't handle nil error case, it will raise a nil
pointer error when there is no ClusterSet found.
Fix it with a new error with error message.
Refactor unit test and add a few more test cases.

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

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Aug 26, 2022
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

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

We should back-port this to the 1.8 patch release.

@luolanzone
Copy link
Contributor Author

sure, I will backport it after it's merged. thanks.

@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #4154 (bb89c75) into main (68d274a) will decrease coverage by 1.81%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4154      +/-   ##
==========================================
- Coverage   65.08%   63.27%   -1.82%     
==========================================
  Files         303      304       +1     
  Lines       46561    46626      +65     
==========================================
- Hits        30304    29501     -803     
- Misses      13903    14774     +871     
+ Partials     2354     2351       -3     
Flag Coverage Δ
integration-tests 34.94% <ø> (+<0.01%) ⬆️
kind-e2e-tests 42.83% <ø> (-5.80%) ⬇️
unit-tests 45.03% <100.00%> (+0.06%) ⬆️
Impacted Files Coverage Δ
...luster-controller/memberclusterannounce_webhook.go 71.66% <100.00%> (+16.66%) ⬆️
pkg/agent/proxy/endpointslicecache.go 0.00% <0.00%> (-83.60%) ⬇️
pkg/agent/secondarynetwork/cnipodcache/cache.go 0.00% <0.00%> (-77.56%) ⬇️
pkg/controller/egress/store/egressgroup.go 1.72% <0.00%> (-54.32%) ⬇️
...g/agent/controller/serviceexternalip/controller.go 35.83% <0.00%> (-45.74%) ⬇️
pkg/agent/secondarynetwork/podwatch/controller.go 0.00% <0.00%> (-39.02%) ⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go 29.62% <0.00%> (-22.23%) ⬇️
pkg/agent/util/net.go 25.46% <0.00%> (-20.98%) ⬇️
pkg/agent/flowexporter/connections/connections.go 55.55% <0.00%> (-20.21%) ⬇️
pkg/agent/route/route_linux.go 35.48% <0.00%> (-17.71%) ⬇️
... and 60 more

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.

could you take the chance to add unit test for this case?

@tnqn tnqn added kind/bug Categorizes issue or PR as related to a bug. action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Aug 26, 2022
@luolanzone
Copy link
Contributor Author

Hi @jianjuns @tnqn I have added a new test for this case and also refactor unit test with structured tests. Could you please review again? thanks.

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

admission.Errored doesn't handle nil error case, it will raise a nil
pointer error when there is no ClusterSet found.
Fix it with a new error with error message.
Refactor unit test and add a few more test cases.

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

tnqn commented Aug 29, 2022

/skip-networkpolicy
/skip-conformance
/skip-e2e

@tnqn tnqn merged commit e6d0a2b into antrea-io:main Aug 29, 2022
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
admission.Errored doesn't handle nil error case, it will raise a nil
pointer error when there is no ClusterSet found.
Fix it with a new error with error message.
Refactor unit test and add a few more test cases.

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
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. area/multi-cluster Issues or PRs related to multi cluster. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants