Skip to content

Add more docker stats#14767

Merged
Jellyfrog merged 1 commit intolibrenms:masterfrom
hvanoch:docker
Mar 1, 2023
Merged

Add more docker stats#14767
Jellyfrog merged 1 commit intolibrenms:masterfrom
hvanoch:docker

Conversation

@hvanoch
Copy link
Copy Markdown
Contributor

@hvanoch hvanoch commented Jan 2, 2023

This pull requests add extra stats for docker.
This includes

  • container uptime
  • container size
  • container filesystem
  • Totals per status

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@hvanoch hvanoch force-pushed the docker branch 4 times, most recently from 6eb8177 to 1d121a3 Compare January 2, 2023 14:59
@hvanoch hvanoch force-pushed the docker branch 2 times, most recently from b954ad5 to 24f6d61 Compare January 2, 2023 15:29
@hvanoch hvanoch marked this pull request as ready for review January 3, 2023 11:31
@hvanoch
Copy link
Copy Markdown
Contributor Author

hvanoch commented Jan 3, 2023

Tests are failing. I believe the reasoning for the exception is that I have test for both v1 and v2 which probably include the same source file. not sure on how to get rid of that one.

@Jellyfrog
Copy link
Copy Markdown
Member

I think its because its includes docker.inc.php twice,

but this functions should be generic instead, and it probably exists one that already can do the work in some helper class

In docker.inc.php line 11:
                                                                               
  Cannot redeclare convertToBytes() (previously declared in /home/runner/work  
  /librenms/librenms/includes/polling/applications/docker.inc.php:11)      

@hvanoch hvanoch force-pushed the docker branch 7 times, most recently from 37b49cf to 6b42f14 Compare January 6, 2023 17:16
@hvanoch
Copy link
Copy Markdown
Contributor Author

hvanoch commented Jan 9, 2023

I moved the function to a separate helper class. Tests are also fixed so this MR is ready for review again.

@Jellyfrog
Copy link
Copy Markdown
Member

Would you mind moving your new class into a function in one of the existing helper classes? Number.php would be a good match I suppose. Also add a comment for the function what it does

@hvanoch
Copy link
Copy Markdown
Contributor Author

hvanoch commented Jan 13, 2023

I have moved the conversion function to Number.php class as requested.

@hvanoch
Copy link
Copy Markdown
Contributor Author

hvanoch commented Jan 25, 2023

@Jellyfrog are you able to give it an other review?
Thanks

$version = intval($version);

if ($version == 1) {
$output = 'LEGACY';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@murrant is this how we usually do this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Jellyfrog Jellyfrog merged commit 2140ff2 into librenms:master Mar 1, 2023
@librenms-bot
Copy link
Copy Markdown

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/23-4-0-changelog/21194/1

TheMysteriousX pushed a commit to TheMysteriousX/librenms that referenced this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants