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.

Sign up for GitHub

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

Jump to bottom

OpenMediaVault widget: Part 2 #1817

Closed
wants to merge 8 commits into from
Closed

OpenMediaVault widget: Part 2 #1817

wants to merge 8 commits into from

Conversation

userXinos
Copy link
Contributor

@userXinos userXinos commented Aug 16, 2023 •
edited
Loading

Proposed change

Sequel for #1807

изображение

# services.yaml

- OpenMediaVault:
#...
    - ZFS Hits/Misses:
        widget:
          type: openmediavault
          url: http://
          username: admin
          password: yes
          method: zfs.getStats
# widgets.yaml

- openmediavault:
    url: http://
    username: admin
    password: yes
    updates: true
    expanded: true

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 here: Openmediavault docs 2 benphelps/homepage-docs#141
  • 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 16, 2023 •
edited
Loading

http.zip

@userXinos userXinos marked this pull request as draft August 16, 2023 11:46
@shamoon
Copy link
Collaborator

shamoon commented Aug 16, 2023

Since there havent been requests specifically for either of these:

Feels like updates in an info widget is weird, I think of those for persistently-relevant data. I wonder if would be better as another service widget option.

ZFS charts one is cool because charts are cool but I wonder how meaningful it is, again there was no discussion on these.

Overall Im a little concerned because it feels like we already do / would have even more of a large amount of OMV-related code to maintain, imbalanced with most other services we support, for something that I suspect is not used by the vast majority of [our] users based off votes etc

Welcome your thoughts

@userXinos
Copy link
Contributor Author

The update widget will then look like this, is it really necessary in this form? As an info widget, I thought it would be more accurate.
Screenshot_20230817_133105

ZFS, these charts are also in TrueNas, it seems there is some demand, why not port them

I'm not sure that the implementation of endpoints will be expensive, because so far this is a copy-paste to a large extent.

If there is some kind of off-limit on the number of endpoints, then ok, I just implemented what I'm interested in and in this part, what's interesting is secondary. I thought to keep the first (then this) PR open, so as not to be accepted and used as a draft.

@shamoon
Copy link
Collaborator

shamoon commented Aug 17, 2023

There may be demand for ZFS data on a truenas dashboard which is a dedicated space for that but not necessarily the same in homepage... Homepage does not aim to be an "everything" command center, more a very nice dashboard with some useful info.

My biggest concern is code maintenance, etc. This is a lot of code for just one ecosystem used by a minority of users (I think).

My suggestion would be to move the updates info into an (option) of the service widget and drop the graph widget.

@userXinos
Copy link
Contributor Author

Oh, this ?
изображение

shamoon
shamoon requested changes Aug 17, 2023
View reviewed changes
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.

Yea thats great, thanks.

Small thing is to remove these IDE comments.

If youre ready you can mark this as such and Im happy to test it out / merge

src/widgets/openmediavault/proxy.js
Comment on lines +1 to +2
// noinspection JSUnresolvedVariable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection JSUnresolvedVariable

src/widgets/openmediavault/proxy.js
@@ -11,6 +13,7 @@ const BG_POLL_PERIOD = 500;

const logger = createLogger(PROXY_NAME);

// noinspection DuplicatedCode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection DuplicatedCode

src/widgets/openmediavault/methods/smart_get_list.jsx
Comment on lines +1 to +2
// noinspection JSUnresolvedVariable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection JSUnresolvedVariable

src/widgets/openmediavault/methods/downloader_get_downloadlist.jsx
Comment on lines +1 to +2
// noinspection JSUnresolvedVariable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection JSUnresolvedVariable

src/widgets/openmediavault/methods/apt_enumerateUpgraded.jsx
Comment on lines +1 to +2
// noinspection JSUnresolvedVariable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection JSUnresolvedVariable

src/widgets/openmediavault/methods/downloader_get_downloadlist.jsx
];

// noinspection DuplicatedCode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection DuplicatedCode

src/widgets/openmediavault/methods/services_get_status.jsx
];

// noinspection DuplicatedCode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection DuplicatedCode

src/widgets/openmediavault/methods/apt_enumerateUpgraded.jsx
},
];

// noinspection DuplicatedCode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// noinspection DuplicatedCode

@shamoon
Copy link
Collaborator

shamoon commented Aug 26, 2023

@userXinos Im not clear how much demand there is for these options, of course lets please re-address if there is more interest from the discussion(s).

Thanks again

@shamoon shamoon closed this Aug 26, 2023
@userXinos
Copy link
Contributor Author

oh, alright

@github-actions GitHub Actions
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.
Reviewers

@shamoon shamoon shamoon requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@userXinos @shamoon