Spec URL: https://fedorapeople.org/~csnyder/subscription-manager-cockpit/subscription-manager-cockpit.spec SRPM URL: https://fedorapeople.org/~csnyder/subscription-manager-cockpit/subscription-manager-cockpit-2-1.fc35.src.rpm Description: This package contains the Subscription Manager Cockpit UI. It provides a graphical interface to allow common subscription management operations like registration. Fedora Account System Username: csnyder
Taking this review.
Note for reviewers: the binary packages, subscription-manager-cockpit and rhsm-icons, already exists in Fedora, currently built by the subscription-manager source. We (subscription-manager developers) recently split the cockpit plugin in its own repository, as it is much easier to work on. The content (in terms of files installed, directories owned, etc) of the subscription-manager-cockpit & rhsm-icons built by this source should the same as the current version built by the subscription-manager source; the only change should be the addition of %license, more in line with the modern Fedora packaging style. Also, thanks to the feedback that the review of another Cockpit module (bug 1969450), we have some Fedora packaging fixes pending for the upstream repository: https://github.com/candlepin/subscription-manager-cockpit/pull/21 the proposed source in comment 0 currently does not include them, so consider them as part of future reviews/iterations.
Spec review notes: > %setup -q -n %{name} > %setup -q -a 1 -n %{name} This can be collapsed to a single macro: "%autosetup -n %{name} -a 1". As it currently stands, the second command is redoing the first command. :) > appstream-util validate-relax --nonet %{buildroot}/%{_datadir}/metainfo/* > desktop-file-validate %{buildroot}/%{_datadir}/applications/* Please move these to %check.
(In reply to Neal Gompa from comment #3) > Spec review notes: > > > %setup -q -n %{name} > > %setup -q -a 1 -n %{name} > > This can be collapsed to a single macro: "%autosetup -n %{name} -a 1". As it > currently stands, the second command is redoing the first command. :) Makes sense. > > appstream-util validate-relax --nonet %{buildroot}/%{_datadir}/metainfo/* > > desktop-file-validate %{buildroot}/%{_datadir}/applications/* > > Please move these to %check. I can do, seems harmless. https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/ seems to suggest it can be either in %install or %check. Since the build system of subscription-manager-cockpit is based/derived on cockpit' starter-kit, I submitted the fixes there: https://github.com/cockpit-project/starter-kit/pull/575
(In reply to Pino Toscano from comment #4) > Since the build system of subscription-manager-cockpit is based/derived on > cockpit' starter-kit, I submitted the fixes there: > https://github.com/cockpit-project/starter-kit/pull/575 This was approved and merged, so I added them to the PR for subscription-manager-cockpit: https://github.com/candlepin/subscription-manager-cockpit/pull/21 Thanks!
Review notes: * Package builds and installs * Package follows packaging guidelines * No serious issues from rpmlint This packaging looks pretty reasonable. I expect to see those improvements land shortly. PACKAGE APPROVED.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/subscription-manager-cockpit