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

Improve proxmox #1676

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Improve proxmox #1676

merged 1 commit into from
Jul 20, 2023

Conversation

markwalet
Copy link
Contributor

Proposed change

The following changes have been done to the proxmox widget:

  • Calculate average load across nodes
  • Add node filter to service definition.
  • Improve empty state when node not found.
  • Swap display from Block to Resource for cpu and ram metrics.

Without the node filter, the averages across all nodes will be calculated. I've considered the original behaviour a bug and because of that, not a breaking change. I can however make the new display optional by introducing another config value, but I didn't want to over-complicate the configuration without a good reason.

References #1290

proxmox-demo

Type of change

  • New service widget (not new, but an update)
  • 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: Update proxmox.md benphelps/homepage-docs#123
  • 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.

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, but service widgets should only use the block style, please remove that change.

I cant test as I dont have proxmox, @JazzFisch does but not sure if he's available to test

@markwalet
Copy link
Contributor Author

Alright, I'll remove that then. Is there another way to customise the appearance of these kind of blocks?

@shamoon
Copy link
Collaborator

shamoon commented Jul 13, 2023

No, at the moment consistency > fanciness :)

@markwalet
Copy link
Contributor Author

Alright, I've removed the resource displays. Screenshot:
image

Maybe we can add it back in in the future, when there are more tested display modes for blocks or something. I personally like a bit of freedom here

@shamoon
Copy link
Collaborator

shamoon commented Jul 13, 2023

Great, again someone else will have to test. Also, please squash the 8 commits

@markwalet
Copy link
Contributor Author

Saw that QA was failing so I fixed that now (hopefully).
Regarding the squashing. Is that something that you want me to do? I'm used to that being done on the merge request itself but I'll look into it.

@shamoon
Copy link
Collaborator

shamoon commented Jul 13, 2023

It's not a big deal, yes we can squash here or you can force-push. Different projects do it differently, theres no real clear consensus here

@LAX18
Copy link

LAX18 commented Jul 16, 2023

I can test with my proxmox installation if that is allowed

@markwalet
Copy link
Contributor Author

I've tested it on my own cluster as well. Not sure it @JazzFisch is available for testing. But I don't see how we can test it otherwise

@shamoon
Copy link
Collaborator

shamoon commented Jul 18, 2023

Well I hope you did yes 😉 but some external validation would be good

@markwalet
Copy link
Contributor Author

Completely understand haha

@divemasterjm
Copy link

Well I hope you did yes 😉 but some external validation would be good

I've tested and works flawesly, thanks!!!

@shamoon shamoon merged commit 6e58191 into gethomepage:main Jul 20, 2023
1 check passed
@divemasterjm
Copy link

Alright, I'll remove that then. Is there another way to customise the appearance of these kind of blocks?

Do you think is possible to have also cpu/men for a specific lxc/VM? we can use proxmox widget in any service to show cpu/men.
like add for example lxc/vm: "number"

Thanks

@markwalet markwalet deleted the improve-proxmox branch July 23, 2023 09:44
@tdiekel
Copy link

tdiekel commented Aug 2, 2023

Alright, I've removed the resource displays. Screenshot: image

Maybe we can add it back in in the future, when there are more tested display modes for blocks or something. I personally like a bit of freedom here

#1749
I guess the future is close :)

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.

5 participants