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

OpenMediaVault widget #1807

Merged
merged 1 commit into from
Aug 14, 2023
Merged

OpenMediaVault widget #1807

merged 1 commit into from
Aug 14, 2023

Conversation

userXinos
Copy link
Contributor

@userXinos userXinos commented Aug 12, 2023

Proposed change

Widget for integration with OpenMediaVault (#1204)

изображение

- OpenMediaVault:
    - Services:
        widget:
          type: openmediavault
          url: http://
          username: admin
          password: pass
          method: services.getStatus
    - S.M.A.R.T.:
        widget:
          type: openmediavault
          url: http://
          username: admin
          password: pass
          method: smart.getListBg
    - Downloader Plugin:
        widget:
          type: openmediavault
          url: http://
          username: admin
          password: pass
          method: downloader.getDownloadList

OMV RPC docs: https://docs.openmediavault.org/en/6.x/development/tools/omv_rpc.html
Real docs: Source Code on GH

Type of change

  • New service widget
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (please explain)

Checklist:

  • If adding a service widget or a change that requires it, I have added a corresponding PR to the documentation:Openmediavault docs benphelps/homepage-docs#140
  • If adding a new widget I have reviewed the guidelines.
  • If applicable, I have checked that all tests pass with e.g. pnpm lint.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

@userXinos
Copy link
Contributor Author

userXinos commented Aug 12, 2023

I would like to discuss which widget methods should be implemented next. I don’t see any reason to implement CPU Utilization, Memory from the dashboard, since the glances widget provides more functions and an excellent implementation, plus both widgets return one method from the backend, i.e. if you could dine them into a group so as not to spam with a FULL_SYS_INFO request ... .

Network Developer Tools: [POST] {"service":"System","method":"getInformation"...}
изображение

@userXinos

This comment was marked as off-topic.

@benphelps
Copy link
Member

When Homepage TypeScript

When someone does all the work and makes a PR for it, and has enough interest. We'd have to come to some agreement on what required engagement from the community to constitute a "majority" looks like.

Then given a "majority" of the community is interested in converting to TS; give the members a few weeks to review it, test it, etc., which would require a notice to freeze development until the review is complete and merged or otherwise.

It would be a tremendous amount of effort, esp. given the number of widgets currently under maintenance.

@userXinos
Copy link
Contributor Author

I just asked in the plan if there were any plans for this or something like that without pretensions.

Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A couple things:

  • The widget should use normal "blocks", not implement its own style. We've come upon this issue before, but we prioritize consistency.
  • Please revert the linting changes, it sorta muddies this PR

Also, since OMV setup isn't so trivial, if you could please provide some samples of API endpoints for testing, thanks.

@shamoon shamoon marked this pull request as draft August 12, 2023 14:00
@userXinos
Copy link
Contributor Author

изображение

  1. I just copied the style from the OMV dashboard, which widget should I pay attention to to implement this functionality? Or to do the normal text list ?

@userXinos
Copy link
Contributor Author

userXinos commented Aug 12, 2023

http.zip

Note: smart.getListBg returns the name of the file that starts executing on the server, need to execute the exec.getOutput request until the desired response is geted

@shamoon
Copy link
Collaborator

shamoon commented Aug 12, 2023

Thanks. Yea the text "block" widgets.

@userXinos
Copy link
Contributor Author

изображение

Current state. Not so much fun

@shamoon
Copy link
Collaborator

shamoon commented Aug 12, 2023

Sorry, to clarify, they should be some version of e.g:, just like the downloader one

    <Container service={service}>
       <Block label=..." value={...} />
       <Block label="..." value={...} />
     </Container>

This is how fields filtering works and its the standard for all service widgets.

I'll leave it up to you how to handle that, if you list the number of drives with SMART OK, services up / total, or whatever. Personally I would merge the widgets into one and support different fields (with a default limited to max 4 as in the guidelines). An example like this is the gamedig widget: https://github.com/benphelps/homepage/pull/1717/files

@userXinos
Copy link
Contributor Author

alright

@userXinos
Copy link
Contributor Author

userXinos commented Aug 12, 2023

изображение

Personally I would merge the widgets into one

yes, probably

I do not plan to dwell on these 3 widgets, I think something else is being implemented. Maybe someone wants some more numbers/data to withdraw from OMV?

@shamoon shamoon marked this pull request as ready for review August 13, 2023 15:25
Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

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

Thanks for your flexibility, this is great overall. I made some relatively minor changes (some needed, like placing error handling before the items manipulation, unique translation strings, etc). Please let me know if you have any concerns

Screenshot 2023-08-13 at 8 19 22 AM Screenshot 2023-08-13 at 8 18 46 AM Screenshot 2023-08-13 at 8 09 47 AM

@userXinos
Copy link
Contributor Author

I was hoping that the members of the discussion would like to see some of the methods released. I will implement a few later. Can I use the graphics widget style from Glances Widget for this ?
изображение

@shamoon
Copy link
Collaborator

shamoon commented Aug 13, 2023

Yes you can use the charts or the standard ones. Im happy to merge this in the meantime if you’re good with it

@shamoon shamoon merged commit b0f2970 into gethomepage:main Aug 14, 2023
1 check passed
@userXinos userXinos mentioned this pull request Aug 16, 2023
8 tasks
@Mbarmem
Copy link
Contributor

Mbarmem commented Sep 20, 2023

I would like to discuss which widget methods should be implemented next. I don’t see any reason to implement CPU Utilization, Memory from the dashboard, since the glances widget provides more functions and an excellent implementation, plus both widgets return one method from the backend, i.e. if you could dine them into a group so as not to spam with a FULL_SYS_INFO request ... .

Network Developer Tools: [POST] {"service":"System","method":"getInformation"...} изображение

Hi @userXinos , would you mind sharing the code for pulling CPU & Memory usage.

I am using OMV on a different system and would love to implement it.

@userXinos
Copy link
Contributor Author

@Mbarmem

I don't have such code, can you see something similar in the glances widget?

Copy link
Contributor

github-actions bot commented Feb 5, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants