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

Introduce incus top #817

Merged
merged 2 commits into from
May 6, 2024
Merged

Introduce incus top #817

merged 2 commits into from
May 6, 2024

Conversation

CharanSriramUni
Copy link
Contributor

@CharanSriramUni CharanSriramUni commented May 2, 2024

Closes #701

@stgraber
Copy link
Member

stgraber commented May 5, 2024

Looking into this one now

@CharanSriramUni
Copy link
Contributor Author

Awesome; just added the updated translations as well.

@stgraber
Copy link
Member

stgraber commented May 5, 2024

Did some simple fixes:

  • Rebased everything down to one commit
  • Added missing Signed-off-by line
  • Added missing bug reference
  • Generated translations
  • Ran make static-analysis and fixed what it found

After that looked at the logic itself and did:

  • Made all user visible strings translated
  • Made most errors in handleKeystrokes return out of the function to avoid infinite error loops

TODO:

  • Remove dependency on internal/server/metrics
  • Use the metrics as the source of instance names (save the GetInstanceNames call) which will also make stopped instances disappear

@stgraber stgraber changed the title cmd/top: Final draft Introduce incus top May 5, 2024
@CharanSriramUni
Copy link
Contributor Author

@stgraber - Should be good now I think!

Removed the dependency and removed the extraneous call to GetInstanceNames. Also, reran translations and checked static-analysis.

Thanks for all the help with this issue! Really made contributing to my first open source project a ton of fun :)

@stgraber
Copy link
Member

stgraber commented May 6, 2024

Getting back to working on this one now.

@stgraber
Copy link
Member

stgraber commented May 6, 2024

I did:

  • Rebase on main
  • Cleaned up commits
  • Ran make static-analysis and fixed what it reported
  • Changed to show absolute values instead of percentages
  • Changed sorting to show largest first

@CharanSriramUni
Copy link
Contributor Author

Do I need to worry about Tests / System (1.21.x, cluster, dir) (pull_request) ? Seems to be a massive test from the logs but not sure if it's correct.

@stgraber
Copy link
Member

stgraber commented May 6, 2024

Nope, that's fine, a random failure can happen every so often

@stgraber stgraber merged commit 525c23f into lxc:main May 6, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add an incus top command
2 participants