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

Refactor Summaries #759

Merged
merged 11 commits into from
May 13, 2021
Merged

Refactor Summaries #759

merged 11 commits into from
May 13, 2021

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented May 6, 2021

Did you run make format && make check?
yes

Fixes #757

Changes:

  • Changed Summary to Overview and ExtraSummary to Summary.
  • Factored out TCPAddr and Online fields into its own struct NetworkStats.
  • Removed extraSummaryWithDmsgResp and updated the new Summary accordingly.
  • Renamed getVisorsExtraSummary to getAllVisorsSummary.
  • Removed TCPAddr field from makeSummaryResp

How to test this PR:

  1. Run make build-ui; make move-built-frontend ; make build; make install
  2. Start Hypervisor and check ui.

@ersonp
Copy link
Contributor Author

ersonp commented May 6, 2021

dmsg stats is needed by both getVisorSummary and getAllVisorsSummary.

Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

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

Seems to me like TCP address is not actually completely removed. If it is, then we can probably get rid of the whole network stats structure because

  1. TCPAddr is gone
  2. Online field is not actually filled by visor iirc so it does not need to be part of the summary.

pkg/visor/api.go Outdated Show resolved Hide resolved
@ersonp
Copy link
Contributor Author

ersonp commented May 7, 2021

"/visors", getVisors() and "/visors/{pk}", getVisor() uses NetworkStat but i do not know where the endpoints are used. I removed NetworkStat from them and everything still works but I will need more info on them. Online is used by getAllVisorsSummary() and the port in TCPAddr is used by getVisorSummary so I replaced TCPAddr with Port. Both getAllVisorsSummary() and getVisorSummary user Overview so Online and Port can't be taken out.

pkg/visor/api.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Senyoret1 Senyoret1 left a comment

Choose a reason for hiding this comment

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

Working well, just 2 things:

  • With the changes made, some parts of the UI code are no longer needed, but I checked and added them to a list, so I can make the changes after this PR is merged, to avoid delays with this one.
  • I'm not a fan of updating the compiled UI files in PRs that were made for adding other changes, but if nobody else has problems with this, there is no problem.

ersonp added 2 commits May 13, 2021 01:43
This reverts commit 155d9fb
@jdknives jdknives merged commit 08f03ce into skycoin:develop May 13, 2021
@ersonp ersonp deleted the refactor/hypervisor branch April 11, 2022 15:37
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.

3 participants