-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Cleanup dependentStores #4089
Cleanup dependentStores #4089
Conversation
Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>
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
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.
I am requesting changes so we can discuss this a bit more
@@ -79,7 +76,7 @@ export class Deployments extends React.Component<Props> { | |||
isConfigurable | |||
tableId="workload_deployments" | |||
className="Deployments" store={deploymentStore} | |||
dependentStores={[replicaSetStore, podsStore, nodesStore, eventStore]} |
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.
So looking at it, we actually do use podsStore
and replicaSetStore
in DeploymentDetails
(though that component calls reloadAll()
for some reason. I think it would be better to remove that reload calls instead.
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.
User can navigate to Deployment details elsewhere too.
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.
okay fair enough
This PR removes unnecessary dependent stores from workload list components.