https://bugzilla.redhat.com/show_bug.cgi?id=2030814 --- Comment #2 from Matthew Miller <mattdm@xxxxxxxxxx> --- Wow, modern spec files are getting closer and closer to the ideal. :) 1. Shouldn't it use `%{_datadir}/cockpit` in %files? (The main cockpit spec file does.) 2. Should this `Recommends: samba` and `nfs-utils` instead of Requires? I can see not wanting both. 3. This includes bundled fontawesome. I tested, and I don't think it needs to. * `rm -rf fontawesome` * sed -i '/fontawesome/d' file-sharing.html Having done this, the fontawesome glyphs (a plus sign and downward triangle) still work. This is what the warning about an unmarked `LICENSE.txt` file is all about. If we de-bundle, though... not needed. 4. Also includes a `file-sharing-patternfly.css.gz` — I hoped this would be the same and can also just be dropped, but removing it causes the utility to get boxed in a little viewport at the top of the its cockpit panel rather than using the whole thing. Not sure what is causing this -- mismatch in patternfly versions, maybe? I think this is probably the right thing to do, just not sure how to do it. Need web developer help I guess. Not sure if this is a blocker. 5. License of the files in "branding" file is unclear. There's nothing explicit excluding them from the GPLv3 license, so it's probably okay, but since we're not including this as part of one of their offerings, it seems respectful to debrand so they don't get misdirected support requests. Plus we don't have that kind of branding anywhere else in Cockpit in Fedora that I can see. It's easy to remove `branding.svg` — deleting that file and removing the line referencing it from file-sharing.html (sed -i '/branding\.svg/d' file-sharing.html) is sufficient. The other file, `spinner.svg`, also appears to be their logo, but without the name. Changing `class="spinner-45d"` to `class="spinner spinner-lg"` in `samba-manager/samba-manager.html` and `nfs-manager/nfs-manager.html` replaces this with a generic spinner — it doesn't quite match the styling of other Cockpit panels, but that could be a future improvement. Removing the entire spinner-45d from the css files would be the cleanest, but isn't necessary if nothing references that, and we can just rm -rf the branding subdir. 6. Automatic checklist complaint about no package owning `/usr/share/cockpit` is odd, as that belongs to `cockpit-bridge`. 7. Summary currently is "A cockpit module to make file sharing with Samba and NFS easier." and Description is: Cockpit File Sharing A cockpit module to make file sharing with Samba and NFS easier. I wouldn't nitpick this, except we should capitalize Cockpit, and while we're at it might as well match the descriptions of other Cockpit modules (and take the description directly from the readme.) AND, I think it's important to get "SMB" in here. So Summary: Cockpit user interface for managing SMB and NFS file sharing. A Cockpit component for managing SMB exports and NFS shares. This package uses Samba and nfs-utils. 8. There is a 2.4.2 version out already. :) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2030814 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
