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

feat(archives): add non-target-specific "all-archives" view #550

Merged
merged 11 commits into from
Oct 14, 2022

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Oct 11, 2022

Fixes #546
Related https://github.com/cryostatio/cryostat/pull/1110
Depends on https://github.com/cryostatio/cryostat/pull/1110

Must be tested with the above back-end PR to work.

@maxcao13 maxcao13 added the feat New feature or request label Oct 11, 2022
@maxcao13 maxcao13 force-pushed the all-all-archives-view branch from 5639d1e to 1e21244 Compare October 13, 2022 04:11
@maxcao13
Copy link
Member Author

maxcao13 commented Oct 13, 2022

Thoughts before tests?

The new view works differently in that notifications refresh the entire page. This is done because the querying to the backend actually happens in the parent component AllArchivedRecordingsTable which contains multiple nested ArchivedRecordingsTables <- these normally update state by themselves but the new view has a entirely different set of api calls that don't depend on jvmIds and such so I would have to create a new ArchivedRecordingsTable-like component to properly handle notifications instead of just refreshing the page, which I haven't done. It is entirely possible to do, but I'm not sure if this the current state is okay enough since tomorrow is the deadline.

@maxcao13 maxcao13 requested review from andrewazores and tthvo and removed request for andrewazores October 13, 2022 23:13
@andrewazores
Copy link
Member

Sorry, I won't have time to review this fully tonight, but I will get it done tomorrow before making the upstream branches.

The full-view refresh on notification sounds fine for now, I don't expect this to be a popularly-used view in most scenarios, so it's OK if we leave a performance optimization on the table for later.

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks good overall to me. Just some questions :D

@tthvo
Copy link
Member

tthvo commented Oct 14, 2022

Thoughts before tests?

The new view works differently in that notifications refresh the entire page. This is done because the querying to the backend actually happens in the parent component AllArchivedRecordingsTable which contains multiple nested ArchivedRecordingsTables <- these normally update state by themselves but the new view has a entirely different set of api calls that don't depend on jvmIds and such so I would have to create a new ArchivedRecordingsTable-like component to properly handle notifications instead of just refreshing the page, which I haven't done. It is entirely possible to do, but I'm not sure if this the current state is okay enough since tomorrow is the deadline.

One way to go around this issue (i.e. similar to AllTargetArchive) is that the parent AllArchive can query only the list of directories and counts. Then, pass in an optional DirectoryOptions props including the directory info to the ArchiveTable. Then, the archive can check if this option is available, and make the api calls to fetch recordings for that directory. I think we should do the same for uploads. But this seems to require some refactoring and maybe backend supports :D

@maxcao13 maxcao13 marked this pull request as ready for review October 14, 2022 01:41
@andrewazores
Copy link
Member

@tthvo @maxcao13 please file an issue (or issues) to track these planned improvements to this view so we can put them on the backlog to be addressed after the release branchpoint.

@andrewazores
Copy link
Member

Just had one suggestion for the way the rows are rendered on the new table. PR opened against this feature branch here:

maxcao13#1

@github-actions
Copy link

This PR/issue depends on:

@andrewazores
Copy link
Member

@maxcao13 I updated your PR bodies for this and the backend PR to invert the depends-on relationship between them. Now that we have a CI hook to automatically update the submodule in the backend repo we can do it this more intuitive way.

@andrewazores
Copy link
Member

@tthvo thanks for taking the code review earlier, I've given it a brief look-over after your more thorough review and things look OK. It works well, too. Other than my suggestion PR to change how the rows are displayed I have no further requests.

@andrewazores
Copy link
Member

Oh, I didn't update the snapshot test along with that PR I made. Could you update that yourself @maxcao13 ? Good to go after that, I think.

@tthvo
Copy link
Member

tthvo commented Oct 14, 2022

@tthvo @maxcao13 please file an issue (or issues) to track these planned improvements to this view so we can put them on the backlog to be addressed after the release branchpoint.

Right, I filed the issue for refactoring at #552 :D

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks great now! Thanks for making this features :D

@andrewazores andrewazores merged commit b201606 into cryostatio:main Oct 14, 2022
@maxcao13 maxcao13 deleted the all-all-archives-view branch October 14, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Add a view for users to find all archives without target-specificity
4 participants