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

feat: add cockpit-zfs-manager for zfs builds #145

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Eelviny
Copy link
Contributor

@Eelviny Eelviny commented Apr 15, 2024

Long time lurker, first time contributor. This PR adds https://github.com/45Drives/cockpit-zfs-manager to ZFS builds of ucore-minimal.

You may notice that this repo actually provides .rpm builds and I wanted to use that, however those builds have a hard dependency on ZnapZend making it not easily usable. I instead opted to pull the release tar. I'm unsure whether locking to a specific version in this way is the best and feedback would be appreciated here.

I've tested these changes by creating an rpm-ostree usroverlay and running the commands manually, but unsure how best to properly integration test.

@Eelviny
Copy link
Contributor Author

Eelviny commented Apr 16, 2024

Just noticed that there is an issue open already for this, fixes #100

@bsherman
Copy link
Collaborator

Long time lurker, first time contributor. This PR adds https://github.com/45Drives/cockpit-zfs-manager to ZFS builds of ucore-minimal.

You may notice that this repo actually provides .rpm builds and I wanted to use that, however those builds have a hard dependency on ZnapZend making it not easily usable. I instead opted to pull the release tar. I'm unsure whether locking to a specific version in this way is the best and feedback would be appreciated here.

I've tested these changes by creating an rpm-ostree usroverlay and running the commands manually, but unsure how best to properly integration test.

Last time I tested the cockpit-zfs-manager there were some significant downsides... broken icons, broken javascript, etc...

It's pretty obvious we don't have formal requirements for integration testing, but the fancier a tool is the more cautious I am. 😀

One thing we could do, add some logic to the build process to ONLY add certain things to the testing images.

For example, I am hesitant to add this to stable, but it could be in testing to give people a chance to try it out.

Thoughts about this?

@Eelviny
Copy link
Contributor Author

Eelviny commented Apr 30, 2024

Yep, I tried it and there's definitely still broken icons etc. 45drives is basically keeping it on life support after it got abandoned by the creator.

However in terms of functionality, it's still working fine. I've gotten into the habit of putting this into a usroverlay every time I need to make changes to the zpool, just put in a PR because I thought it might be a good idea but I can understand if it's not quite at the level of quality you'd want to have.

testing image sounds great.

@bronson
Copy link
Contributor

bronson commented Nov 24, 2024

Putting a dead-end package in testing also seems suboptimal. (and it pains me to say that because I run zfs too)

Have you considered making a ucore-based image that includes it?

@bsherman
Copy link
Collaborator

Have you considered making a ucore-based image that includes it?

Actually, a few of us have been been done some testing of Cockpit ZFS Manager with a font fix applied which 45Drives themselves created. This seems to have addressed the specific concerns to which I referred back in April.

Now that we have that fixed, it may be fine to add this. Though I'd only put it in our ucore image not ucore-minimal.

@Eelviny if you don't mind, I'll clean this up a little bit with the fixes we need and we can get it merged.

@Eelviny
Copy link
Contributor Author

Eelviny commented Nov 25, 2024

Please, go ahead and make the necessary changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants