-
Notifications
You must be signed in to change notification settings - Fork 22
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
RHCLOUD-23119: Allow for on demand API discovery through hooks #217
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: florkbr 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 |
77f8842
to
838c0fc
Compare
838c0fc
to
500b182
Compare
if (!k8sModel && !inFlight && !batchesInFlight && retries < 3) { | ||
fetchModel(resource, dispatch); | ||
setRetries(retries + 1); | ||
} | ||
return modelsLoaded && !k8sModel && !batchesInFlight | ||
? [data, true, new NoModelError()] |
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.
We may need to prevent this return value from returning while we're still trying to fetch the model on demand above. While I was testing locally this didn't seem to be an issue, but I'll continue looking.
/assign @karelhala |
@karelhala FWIW I was able to test this change by commenting out API discovery in hac-core: diff --git a/frontend/src/sdk/InitializeSDK.tsx b/frontend/src/sdk/InitializeSDK.tsx
index 71860aa..a281f62 100644
--- a/frontend/src/sdk/InitializeSDK.tsx
+++ b/frontend/src/sdk/InitializeSDK.tsx
@@ -2,7 +2,7 @@ import * as React from 'react';
import { AppInitSDK } from '@openshift/dynamic-plugin-sdk-utils';
import Loader from '../Utils/Loader';
import useAppConfiguration from './useAppConfiguration';
-import { initAPIDiscovery } from '../Utils/api-discovery';
+// import { initAPIDiscovery } from '../Utils/api-discovery';
const InitializeSDK: React.FC = ({ children }) => {
const appConfigurations = useAppConfiguration();
@@ -14,7 +14,7 @@ const InitializeSDK: React.FC = ({ children }) => {
// TODO(SDK): Don't override children
// TODO(SDK): AppInitSDK doesn't work with no redux
// react_devtools_backend.js:4061 Error: could not find react-redux context value; please ensure the component is wrapped in a <Provider>
- return <AppInitSDK configurations={{ ...appConfigurations, apiDiscovery: initAPIDiscovery }}>{children as any}</AppInitSDK>;
+ return <AppInitSDK configurations={{ ...appConfigurations, apiDiscovery: () => {} }}>{children as any}</AppInitSDK>;
};
export default InitializeSDK; |
@florkbr: This pull request references RHCLOUD-23119 which is a valid jira issue. 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/test-infra repository. |
@florkbr: This pull request references RHCLOUD-23119 which is a valid jira issue. 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/test-infra repository. |
@@ -235,6 +236,35 @@ export const getGroupVersionKindForModel: GetGroupVersionKindForModel = ({ | |||
kind, | |||
}); | |||
|
|||
/** | |||
* Provides the K8sResourceIdentifier for the given K8sGroupVersionKind or K8sResourceKindReference. |
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.
FWIW this is not exposed to SDK consumers, just doc'ed internally for SDK devs.
@florkbr: all tests passed! Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
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.
Looking good! I wouldn't include onlyPreferenceList
in this PR. We should work on allowing plugins listing models to watch for and any other model can be on demand trough these new hooks.
@karelhala I'm not sure I follow. At some point won't we need to fallback on fetching the model if it's not in the list plugins provide? |
Yes, that's the aim of this PR isn't it? The idea is that plugins will provide default models we'll watch for. If extension would want to watch for another model they'll use these new hooks. |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
PR needs rebase. 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/test-infra repository. |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. 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/test-infra repository. |
Success case: demonstration showing fetch is only called once per model
on-demand-model-fetch-example.mp4
Failure case: demonstration showing fetch will be limited to only 3 calls for a given model
https://issues.redhat.com/browse/RHCLOUD-23119