UI: ignore error when list public ips for CKS clusters on Shared network#8489
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.18 #8489 +/- ##
=========================================
Coverage 13.12% 13.12%
Complexity 9142 9142
=========================================
Files 2720 2720
Lines 257744 257744
Branches 40182 40182
=========================================
Hits 33839 33839
+ Misses 219615 219614 -1
- Partials 4290 4291 +1 ☔ View full report in Codecov by Sentry. |
| }).catch(() => { | ||
| this.publicIpAddress = null |
There was a problem hiding this comment.
Instead of ignoring the exception, why not check if the user has permission to use the API?
There was a problem hiding this comment.
Instead of ignoring the exception, why not check if the user has permission to use the API?
The issue is not caused by permission of API. The users have permission to the api, but they cannot list public ips of a shared network which has scope=domain
There was a problem hiding this comment.
How do you know that @MejdiB is using shared networks with domain scope in their CKS clusters? Were you able to reproduce the error reported on #7681?
In any case, shouldn't we test if the user is trying to list the public IPs of a shared network with domain scope and not call the API in this case? It's better than ignoring any error that might happen on the API call.
Also, a description of the tests done would be great.
There was a problem hiding this comment.
How do you know that @MejdiB is using shared networks with domain scope in their CKS clusters? Were you able to reproduce the error reported on #7681?
Yes, I am able to reproduce the issue in advanced zone with SG.
In any case, shouldn't we test if the user is trying to list the public IPs of a shared network with domain scope and not call the API in this case? It's better than ignoring any error that might happen on the API call.
this PR is the simplest way.
The better solution might be (1) listNetworks; (2) skip if network type is Shared.
but it did not work in my testing ,may require some investigation.
Also, a description of the tests done would be great.
There was a problem hiding this comment.
I too would like to see a test description.
I agree that preventing a call to the server is good, but a working solution is as well ;)
I could live with a comment in the code that explains the better solution and why it was not implemented.
There was a problem hiding this comment.
test steps have been added
@JoaoJandre @DaanHoogland
|
@shwstppr a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
@rohityadavcloud a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
|
thanks for review @DaanHoogland @JoaoJandre moved this to draft. I will revisit later |
|
@weizhouapache will this make it for 4.18.2? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.18 #8489 +/- ##
=========================================
Coverage 13.12% 13.12%
Complexity 9142 9142
=========================================
Files 2720 2720
Lines 257744 257744
Branches 40182 40182
=========================================
Hits 33839 33839
+ Misses 219615 219614 -1
- Partials 4290 4291 +1 ☔ View full report in Codecov by Sentry. |
|
@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
weizhouapache
changed the title
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9307 |
|
@blueorangutan test alma9 kvm-alma9 keepEnv |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-9903)
|
|
@blueorangutan test alma9 kvm-alma9 keepEnv securityGroups |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-9935)
|
note that this is a normal rate for security group zones :( |
|
@weizhouapache , as admin user I can not register an iso : FYI, haven't investigated this yet: |
|
[SF] Trillian Build Failed (tid-9955) |
|
[SF] Trillian Build Failed (tid-9956) |
|
verified manually in a test env with security groups; a cluster can be seen without error messages. |
|
reports in #8489 (comment) are serious but not related to this code (no proper error handling when no zone is ready and enabled. |
cool, thanks @DaanHoogland |
|
@JoaoJandre are you ok with this one? |
|
@JoaoJandre @MejdiB can you test / review this? |
JoaoJandre
left a comment
JoaoJandre
left a comment
There was a problem hiding this comment.
CLGTM, I don't have an env with security groups, I could deploy one but it might take me a while. It would be good if @MejdiB could validate this, otherwise, I can try to test it within a few weeks.
ok thnks, I'll merge as I have tested it. @MejdiB can always create a new issues if he has additional configurations/requirements |
* 4.18: UI: ignore error when list public ips for CKS clusters on Shared network (#8489)
* 4.18: UI: ignore error when list public ips for CKS clusters on Shared network (apache#8489)
* 4.19: UI: ignore error when list public ips for CKS clusters on Shared network (apache#8489) Infra25725 add codecov token to workflow (apache#8960)
7 participants
Description
This PR fixes #7681
steps to reproduce the issue
Without this PR. same issue #7681
With this PR, the errors are gone.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?