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

Feature/extra summary #666

Merged
merged 9 commits into from
Mar 17, 2021

Conversation

i-hate-nicknames
Copy link
Contributor

Did you run make format && make check?
Yes

Fixes #646

Changes:

  • Add GET /api//visors-summary endpoint, allowing to fetch detailed information about each visor

How to test this PR:
Run a hypervisor with some visors and query GET /api//visors-summary.

wg.Add(len(hv.visors))

i := 0
if hv.visor != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Is there currently any possiblity of this code executing when hv.Visor would be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed there isn't. I took the code for getting visors and modified it to have additional info in response. However, we never start hypervisor without visor, except in tests. However, some tests rely on nil response on an "empty" hypervisor. Not sure if it's worth the trouble to change the tests and remove nil-checks everywhere.

@Senyoret1
Copy link
Contributor

This is working well. I would just remove the dmsg field, because it appears to always return null and the data needed is in the dmsg_stats field, and the routes field, because it is not needed for the visor list and I think it could sometimes contain a lot of data, which would just unnecessarily degrade the performance. However, if there are reasons for keeping them there, then there is no problem

@i-hate-nicknames
Copy link
Contributor Author

@Senyoret1 no other reasons besides just composing existing structures instead of declaring new ones. If you don't need that info it's no problem to avoid sending it.

@Senyoret1
Copy link
Contributor

I checked and #699 contines working after the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants