-
Notifications
You must be signed in to change notification settings - Fork 23
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: update tree, query cluster info and send telemetry in backgroud (#912) #915
Conversation
9d7b3ae
to
68ee7f2
Compare
9f99d1d
to
e83ba11
Compare
@@ -418,7 +415,7 @@ public ComponentInfo getComponentInfo(String project, String component, String p | |||
|
|||
private ComponentInfo parseComponentInfo(String json, ComponentKind kind) throws IOException { | |||
JSonParser parser = new JSonParser(Serialization.json().readTree(json)); | |||
return parser.parseDescribeComponentInfo(kind, isPodmanPresent); | |||
return parser.parseDescribeComponentInfo(kind, isPodmanPresent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why modify this variable to a method call each time a compoenent is parsed ? this is time consuming for no reason, has podman is present or not for the whole time the extension is instanciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason is the following:
the check was executed in the constructor of OdoCli
and it's a heavy operation. All the heavy operations that are in the constructor of OdoCli were executed when the tree structure requested the odo binary. This then lead to the tree displaying "IndicatorCancelled" when the UI was waiting for these operations and it took too long.
I changed the current impl so that it would lazy determine if podman is present in the background and store the result once it's determined.
108e796
to
ce65129
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. heavy check is now perfomed only once, in a lazy manner. good one !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbouchet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…edhat-developer#912) Signed-off-by: Andre Dietisheim <adietish@redhat.com>
New changes are detected. LGTM label has been removed. |
/override ci/prow/e2e-openshift |
@adietish: Overrode contexts on behalf of adietish: ci/prow/e2e-openshift In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Quality Gate failedFailed conditions |
37bdd93
into
redhat-developer:main
fixes #912