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 application dropdown to fetch resources based on extensions. #7034

Conversation

jeff-phillips-18
Copy link
Member

Fixes:
https://issues.redhat.com/browse/ODC-3613

Solution Description:
Use the topology plugin extensions to determine which resource kinds to fetch to then search for existing application groupings.

Browser conformance:

  • [x ] Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console labels Oct 29, 2020
@jeff-phillips-18 jeff-phillips-18 force-pushed the application-dropdown branch 3 times, most recently from 5df5c15 to d69f19c Compare October 30, 2020 15:29
@jeff-phillips-18
Copy link
Member Author

/retest

resources={resources}
loaded={loaded}
loadError={loadError}
placeholder={t('devconsole~Select application')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
placeholder={t('devconsole~Select application')}
placeholder={t('devconsole~Select an application')}

unsure if we need to take care of localization in this PR 'cause PR #6905 takes care of all the files under dev-console

Copy link
Member Author

Choose a reason for hiding this comment

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

afaik, the ask was to make sure any PRs do the i18n work as well in order to help ensure things are not missed.

@christianvogt
Copy link
Contributor

This works fine. However we are severely overfetching resources now.
There is no efficient way to properly identify all the applications within a namespace.
cc @serenamarie125 as this problem will continue to get worse.

Comment on lines 75 to 78
!kindsInFlight &&
Object.keys(watchResults).length > 0 &&
Object.keys(watchResults).every(
(key) => watchResults[key].loaded || watchResults[key].loadError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need kindsInFlight? Seems like you can rely on loaded of the watched resources. kindsInFlight will include every resource that's currently in flight from anywhere on the page.

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially loaded can return true with load errors for every resource fetched when kindsInFlight is true. This results in an error being shown briefly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've put a fix into useK8sWatchResources to not set the loaded flag to true when the model is not loaded and kinds are still in flight. @rawagner PTAL

@jeff-phillips-18 jeff-phillips-18 force-pushed the application-dropdown branch 2 times, most recently from be68ac3 to 6fc7a2d Compare November 4, 2020 21:42
@christianvogt
Copy link
Contributor

/approve

@rawagner please have a look at the change to k8s-watch-hook.ts

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
@@ -212,7 +212,7 @@ export const useK8sWatchResources = <R extends ResourcesObject>(
} else {
acc[key] = {
data: resources[key].isList ? [] : {},
loaded: true,
loaded: modelsLoaded,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that everytime we get here we can say that either all models are loaded or all requested models are loaded -

const hasAllModelsLoaded = React.useMemo(() => modelsLoaded || k8sModels.every((m) => !!m), [
. That means its safe to say resource is not being loaded anymore and the model is missing. Or I missed something, do you have a case which I can try that causes the issue ?

modelsLoaded is also missing in dependency array

}, [reduxIDs, JSON.stringify(resources), resourceK8s, noModels]);

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeff-phillips-18 I guess I see the problem. Can you change hasAllModelsLoaded to

const hasAllModelsLoaded = React.useMemo(
    () => modelsLoaded || Object.values(resources).every((r) => k8sModels.includes(r.kind)),
    [k8sModels, modelsLoaded, resources],
  );

and revert other changes ? That might help

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeff-phillips-18 I've updated my PR #6464 with some more changes which should resolve your issues. Can you try ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rawagner I'll give that a try. I definitely saw instances where all models returned loaded: true here while modelsLoaded was false.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeff-phillips-18 makes sense, hasAllModelsLoaded was broken because second part (after or operator) was just wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that fixes the issue I was seeing. Thanks, I will remove my changes to k8s-watch-hook.ts

@christianvogt
Copy link
Contributor

@jeff-phillips-18 do we wait for the related bug to be fixed or are we sticking with this small fix to the k8s-watch-hook?

@jeff-phillips-18
Copy link
Member Author

@jeff-phillips-18 do we wait for the related bug to be fixed or are we sticking with this small fix to the k8s-watch-hook?

No, we can go in without the change. Just can't update TopologyDataRetriever until the fix to the hook goes in.

@jeff-phillips-18
Copy link
Member Author

@christianvogt #6464 has merged. This PR should be ready now.

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 4e381de into openshift:master Nov 9, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 16, 2020
@jeff-phillips-18 jeff-phillips-18 deleted the application-dropdown branch December 2, 2020 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants