Spec URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07281253-ec2-instance-connect/ec2-instance-connect.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07281253-ec2-instance-connect/ec2-instance-connect-1.1.17-1.fc41.src.rpm Description: This package contains the EC2 instance configuration and scripts necessary to enable AWS EC2 Instance Connect. Fedora Account System Username: wombelix It's my first package submission and I need a Sponsor. I work with David Duncan and plan to become co-maintainer of his packages. I submitted a PR for another package yesterday https://src.fedoraproject.org/rpms/ec2-hibinit-agent/pull-request/4. I submit features and bug fixes to other open source projects on a regular basis. Neal Gompa knows me from Pagure for example. I manually tested the package on F39. The Copr build (https://copr.fedorainfracloud.org/coprs/wombelix/ec2-instance-connect/packages/) is successful on f38, f39, f40, rawhide, epel7, epel8, epel9.
Taking this review.
Initial spec review: > %define __spec_install_post %{nil} > %define debug_package %{nil} > %define __os_install_post %{_dbpath}/brp-compress What's all this for? We shouldn't need any of this... > %global with_selinux 1 We do not need a conditional if it's always enabled, so drop it. > Requires: openssh >= 6.9.0, coreutils, openssh-server >= 6.9.0, openssl, curl, systemd > Requires(pre): glibc-common, shadow-utils, systemd, systemd-units > Requires(post): grep, coreutils, openssh-server >= 6.9.0, systemd, systemd-units > Requires(preun): systemd, systemd-units > Requires(postun): shadow-utils, systemd, systemd-units Please reformat this to have one dependency per line instead of listing them out like this. That makes it easier to recognize in diffs. > # Create/configure system user > %{_bindir}/getent passwd ec2-instance-connect || %{_sbindir}/useradd -r -M -s %{_sbindir}/nologin ec2-instance-connect > %{_sbindir}/usermod -L ec2-instance-connect This needs to be converted to sysusers. Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_dynamic_allocation Additionally, all the scriptlets need to be rethought, we can't have scriptlets modifying things like this when drop-in files can be shipped in subpackages or whatnot.
Thanks for your initial spec Review Neal. > > %define __spec_install_post %{nil} > > %define debug_package %{nil} > > %define __os_install_post %{_dbpath}/brp-compress > > What's all this for? We shouldn't need any of this... I have to admit, idk. Removed. Next time I will highlight which lines I'm not sure about and seek guidance. > > %global with_selinux 1 > > We do not need a conditional if it's always enabled, so drop it. Dropped. > > Requires: openssh >= 6.9.0, coreutils, openssh-server >= 6.9.0, openssl, curl, systemd > > Requires(pre): glibc-common, shadow-utils, systemd, systemd-units > > Requires(post): grep, coreutils, openssh-server >= 6.9.0, systemd, systemd-units > > Requires(preun): systemd, systemd-units > > Requires(postun): shadow-utils, systemd, systemd-units > > Please reformat this to have one dependency per line instead of listing them out like this. That makes it easier to recognize in diffs. Done. > > # Create/configure system user > > %{_bindir}/getent passwd ec2-instance-connect || %{_sbindir}/useradd -r -M -s %{_sbindir}/nologin ec2-instance-connect > > %{_sbindir}/usermod -L ec2-instance-connect > > This needs to be converted to sysusers. Done. > Additionally, all the scriptlets need to be rethought, we can't have scriptlets modifying things like this when drop-in files can be shipped in subpackages or whatnot. As discussed via Matrix, upstream it was done that way to have some sort of safeguard to not affect a already existing `AuthorizedKeysCommand` config. More details here: https://github.com/aws/aws-ec2-instance-connect-config/issues/19 As you suggested, I moved the shell printf part to create a drop-in file into a separate file that is used in the sub-package `ec2-instance-connect-config`. `ec2-instance-connect` recommends the config sub-package. It can be removed by a User in case of issues with a current config (sshd_config, another drop-in file). In that case the actual ec2-instance-connect scripts and selinux policy can stay on the system and used by applying the `AuthorizedKeysCommand` settings manually. New version from latest copr build (https://copr.fedorainfracloud.org/coprs/wombelix/ec2-instance-connect/build/7337112/): - Spec: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07337112-ec2-instance-connect/ec2-instance-connect.spec - SRPM: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07337112-ec2-instance-connect/ec2-instance-connect-1.1.17-1.fc41.src.rpm
> %if 0%{?fedora} || 0%{?rhel} >= 8 > Requires: (%{name}-selinux if selinux-policy-%{selinuxtype}) > %else > Requires: %{name}-selinux > %endif > > %if 0%{?fedora} || 0%{?rhel} >= 8 > Recommends: %{name}-config > %endif I don't think the conditionals are necessary since we're not aiming to add this to EPEL 7...
Also, is it okay for the spec file to use the Fedora default license for spec files (MIT)? If so, then could we drop the license header in the spec file...
(In reply to Neal Gompa from comment #4) > > I don't think the conditionals are necessary since we're not aiming to add > this to EPEL 7... Indeed, dropped (In reply to Neal Gompa from comment #5) > Also, is it okay for the spec file to use the Fedora default license for > spec files (MIT)? If so, then could we drop the license header in the spec > file... Yes that's fine. I made a diff between the original spec and mine. I rewrote the whole thing from scratch at the end. Here is the latest version: Copr Build: https://copr.fedorainfracloud.org/coprs/wombelix/ec2-instance-connect/build/7393104/ Spec URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07393104-ec2-instance-connect/ec2-instance-connect.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07393104-ec2-instance-connect/ec2-instance-connect-1.1.17-1.fc41.src.rpm
Since the spec was completely rewritten, maybe you should reset the changelog too.
(In reply to Neal Gompa from comment #7) > Since the spec was completely rewritten, maybe you should reset the > changelog too. sure, done. Spec URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07399985-ec2-instance-connect/ec2-instance-connect.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07399985-ec2-instance-connect/ec2-instance-connect-1.1.17-1.fc41.src.rpm
Copr build: https://copr.fedorainfracloud.org/coprs/build/7399991 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2274150-ec2-instance-connect/fedora-rawhide-x86_64/07399991-ec2-instance-connect/fedora-review/review.txt Found issues: - Systemd service file(s) in ec2-instance-connect-config Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
You're missing one scriptlet for the config subpackage: %preun config %systemd_preun sshd.service
(In reply to Neal Gompa from comment #10) > You're missing one scriptlet for the config subpackage: > > %preun config > %systemd_preun sshd.service thank you, added to the latest built. Spec URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07400658-ec2-instance-connect/ec2-instance-connect.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/wombelix/ec2-instance-connect/fedora-rawhide-x86_64/07400658-ec2-instance-connect/ec2-instance-connect-1.1.17-1.fc41.src.rpm
Created attachment 2030988 [details] The .spec file difference from Copr build 7399991 to 7400691
Copr build: https://copr.fedorainfracloud.org/coprs/build/7400691 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2274150-ec2-instance-connect/fedora-rawhide-x86_64/07400691-ec2-instance-connect/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Review notes: * Package follows Fedora Packaging Guidelines * Package builds and installs * Package licensing is correctly handled and recorded * No serious issues from rpmlint PACKAGE APPROVED.
The Pagure repository was created at https://src.fedoraproject.org/rpms/ec2-instance-connect