-
Notifications
You must be signed in to change notification settings - Fork 706
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
Add 'Refresh All App Repositories' button #1091
Conversation
dashboard/src/components/Config/AppRepoList/AppRepoRefreshAllButton.tsx
Outdated
Show resolved
Hide resolved
Close vmware-tanzu#582. Add a button next to 'Add App Repository' to refresh all app repositories.
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.
Thanks for the PR! I have sent some comments.
dashboard/src/actions/repos.ts
Outdated
// TODO: Do something to show progress | ||
const repos = await AppRepository.list(namespace); | ||
if (repos.items.length > 0) { | ||
repos.items.forEach(repo => { |
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.
here it's better to dispatch calls to the other method: dispatch(resyncRepo(repo.metadata.name))
to avoid repeating code.
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.
+1, dispatching it will also make each resync request happen in parallel
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.
resyncRepo(name) function will make a GET and UPDATE API call, then a call to AppRepository.list(namespace)
, so using it here will make many duplicate AppRepository.list
calls. Any more idea?
const repo = await AppRepository.get(name, namespace);
repo.spec.resyncRequests = repo.spec.resyncRequests || 0;
repo.spec.resyncRequests++;
await AppRepository.update(name, namespace, repo);
// TODO: Do something to show progress
dispatch(requestRepos());
const repos = await AppRepository.list(namespace);
dispatch(receiveRepos(repos.items));
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.
To be honest, I don't know why we are listing the repositories in the resyncRepo
method, it's not doing anything. You can remove the lines 95-97 so we don't list them several times.
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 guess it is to reload the updated repos list?
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.
Hmm, good catch @jessehu, we definitely don't want to be listing multiple times. I think what we could do here is remove the last few lines from resyncRepo and reuse fetchRepos when calling this action. We can update RepoListContainer to do this:
resyncRepo: async (name: string) => {
await dispatch(actions.repos.resyncRepo(name));
return dispatch(actions.repos.fetchRepos());
},
and then do the same for resyncAllRepos. What do you think?
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.
@prydonius With the new code above, the logic will be resyncAllRepos
-> dispatch(resyncRepo(name));
-> dispatch(actions.repos.fetchRepos());
, so seems it stills calls fetchRepos() multiple times ? For now, is fetchRepos() needed after a resyncRepo(name)? We only show repo name and URL on the UI, so I think it won't make any changes on UI unless there is a new app repo added/deleted in the backend by another kubeapps web UI client, but this is not expected goal of refreshing a single repo.
dashboard/src/components/Config/AppRepoList/AppRepoRefreshAllButton.tsx
Outdated
Show resolved
Hide resolved
dashboard/src/components/Config/AppRepoList/AppRepoRefreshAllButton.tsx
Outdated
Show resolved
Hide resolved
dashboard/src/actions/repos.ts
Outdated
// TODO: Do something to show progress | ||
const repos = await AppRepository.list(namespace); | ||
if (repos.items.length > 0) { | ||
repos.items.forEach(repo => { |
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.
+1, dispatching it will also make each resync request happen in parallel
This is looking great @jessehu, just had a few thoughts |
@@ -43,7 +43,7 @@ export class AppRepoAddButton extends React.Component< | |||
public render() { | |||
const { redirectTo } = this.props; | |||
return ( | |||
<div className="AppRepoAddButton"> | |||
<div> |
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 meant to use React.Fragment
here (instead of div) not in the AppRepoList
kubeappsNamespace={kubeappsNamespace} | ||
/> | ||
<React.Fragment> | ||
<td> |
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.
You should not need React.Fragment
and td
here.
dashboard/src/components/Config/AppRepoList/AppRepoRefreshAllButton.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for the changes!
Close #582. Add a button next to 'Add App Repository'
to refresh all app repositories.