https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Bug ID: 1576879 Summary: Review Request: ignition - First boot installer and configuration tool Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: smilner@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://copr-be.cloud.fedoraproject.org/results/dustymabe/ignition/fedora-28... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/dustymabe/ignition/fedora-28... Description: First boot installer and configuration tool. Fedora Account System Username: smilner
This is a request to move ignition from our current copr repos into Fedora proper.
""" Ignition is the utility used by CoreOS Container Linux to manipulate disks during the initramfs. This includes partitioning disks, formatting partitions, writing files (regular files, systemd units, networkd units, etc.), and configuring users. On first boot, Ignition reads its configuration from a source of truth (remote URL, network metadata service, hypervisor bridge, etc.) and applies the configuration. """
See https://github.com/coreos/ignition/
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Colin Walters walters@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |walters@redhat.com
--- Comment #1 from Colin Walters walters@redhat.com --- Looking at https://fedoraproject.org/wiki/PackagingDrafts/Go
and docker.spec, it looks like we should do:
BuildRequires: %{?go_compiler:compiler(go-compiler)}%{!?go_compiler:golang >= 1.6.2}
Maybe? There's also:
ExclusiveArch: %{go_arches}
Which it looks like docker is carrying a workaround that we could probably drop.
Other than that, LGTM.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #2 from Steve Milner smilner@redhat.com --- Thanks Colin. I'll update, test, and upload changes to my Fedora People space.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #3 from Steve Milner smilner@redhat.com --- Updated and uploaded both spec and SRPM rebuild to https://smilner.fedorapeople.org/ignition/.
[steve@qward SPECS]$ rpmlint /home/steve/rpmbuild/RPMS/x86_64/ignition-0.23.0-0.4.fc26.x86_64.rpm # not an ssue ignition.x86_64: W: spelling-error %description -l en_US systemd -> systems, system, system d # Not an issue ignition.x86_64: W: spelling-error %description -l en_US networkd -> networks, network, networked # Expected ignition.x86_64: W: unstripped-binary-or-object /usr/bin/ignition # Can raise with upstream if this is an issue, but I don't think this would stop inclusion ignition.x86_64: E: missing-call-to-chdir-with-chroot /usr/bin/ignition # Expected ignition.x86_64: W: unstripped-binary-or-object /usr/bin/ignition-validate # Expected ignition.x86_64: W: no-manual-page-for-binary ignition # Expected ignition.x86_64: W: no-manual-page-for-binary ignition-validate 1 packages and 0 specfiles checked; 1 errors, 6 warnings.
[steve@qward SPECS]$ rpmlint ignition.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Derek Gonyeo dgonyeo@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dgonyeo@redhat.com
--- Comment #4 from Derek Gonyeo dgonyeo@redhat.com --- I'm not sure of what's typical here so forgive my ignorance, but Ignition isn't particularly useful without an appropriate dracut configuration. Would the corresponding dracut module be something that should be included in the Ignition RPM, or would that typically be in a separate RPM?
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #5 from Steve Milner smilner@redhat.com --- Derek,
Most likely it would be seperate. It's possible that the ignition package would eventually require said dracut module/config.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #6 from Dusty Mabe dustymabe@redhat.com --- I think I agree with derek here. seems like ignition isn't that useful without the dracut modules so why not include them?
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #7 from Steve Milner smilner@redhat.com --- In this case so that we can make the stream pull from dist-git to rebuild. If we need to make more modifications to the spec then I'll suggest we move towards direct building for the time being in the stream.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #8 from Neal Gompa ngompa13@gmail.com --- I have a couple of questions here:
1. Why are we disabling debuginfo? That has been working with Go for several releases now.
2. My understanding is that ignition uses dracut modules and has dracut module configuration too. I don't see it in the ignition source tree, though. Where is it, and what will provide it? I would expect a dracut module subpackage, like how other packages do it (e.g. kiwi).
I also think the changes that Colin requested in comment 1 need to be done as well.
You also have an empty message entry in your %changelog. Please fix. :)
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com
--- Comment #9 from Robert-André Mauchin zebob.m@gmail.com --- - 0.24 is published.
- Unbundle vendor, that might require lots of work packaging the missing deps. Use gofed to package these.
%prep %autosetup -n %{name}-%{version} rm -rf vendor
Discovering package dependencies Class: github.com/ajeddeloh/go-json (golang-github-ajeddeloh-go-json) PkgDB=False Class: github.com/aws/aws-sdk-go (golang-github-aws-aws-sdk-go) PkgDB=True Class: github.com/coreos/go-semver (golang-github-coreos-go-semver) PkgDB=True Class: github.com/coreos/go-systemd (golang-github-coreos-go-systemd) PkgDB=True Class: github.com/pin/tftp (golang-github-pin-tftp) PkgDB=False Class: github.com/sigma/vmw-guestinfo (golang-github-sigma-vmw-guestinfo) PkgDB=False Class: github.com/vincent-petithory/dataurl (golang-github-vincent-petithory-dataurl) PkgDB=False Class: github.com/vmware/vmw-ovflib (golang-github-vmware-vmw-ovflib) PkgDB=False
Discovering test dependencies Class: github.com/stretchr/testify (golang-github-stretchr-testify) PkgDB=True
- Consider using https://fedoraproject.org/wiki/More_Go_packaging . See examples: https://eclipseo.fedorapeople.org/golang/
- Thus I would not use the build script but a standard %build section like this:
%build %gobuildroot %gobuild -o _bin/ignition %{goipath}/internal %gobuild -o _bin/ignition-validate" %{goipath}/validate
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #10 from Colin Walters walters@redhat.com --- - Unbundle vendor, that might require lots of work packaging the missing deps. Use gofed to package these.
I don't think we have an interest in doing this right now. We have higher priority things to do: for example, fixing Ignition to work more nicely with SELinux.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #11 from Steve Milner smilner@redhat.com --- Colin,
Agreed.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #12 from Neal Gompa ngompa13@gmail.com --- (In reply to Colin Walters from comment #10)
- Unbundle vendor, that might require lots of work packaging the missing
deps. Use gofed to package these.
I don't think we have an interest in doing this right now. We have higher priority things to do: for example, fixing Ignition to work more nicely with SELinux.
If you're not going to unbundle (as I agree you should), then you need to do the right thing and document _all_ your bundled dependencies, per the policy: https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle
Also, please properly post the Spec and Source RPM links every time you update the package. No one can properly review your package if you don't.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #13 from Dusty Mabe dustymabe@redhat.com --- yeah. will document bundled dependencies using gofed like I did for the kompose rpm when I originally created it:
https://src.fedoraproject.org/rpms/kompose/c/abdc98fbde943502c205f3d410991f7...
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #14 from Dusty Mabe dustymabe@redhat.com ---
ok updated. ran it through gofed and tried to take notes about my steps.
Here are my notes [1], the original spec generated by gofed [2], and the modified spec I edited [3]. I created a scratch build [4] and an srpm from that [5] can be accessed.
[1] https://raw.githubusercontent.com/dustymabe/ignition-rpm/master/notes.txt [2] https://raw.githubusercontent.com/dustymabe/ignition-rpm/cd20621c8cfa1027f7a... [3] https://raw.githubusercontent.com/dustymabe/ignition-rpm/cd20621c8cfa1027f7a... [4] https://koji.fedoraproject.org/koji/taskinfo?taskID=27790594 [5] https://kojipkgs.fedoraproject.org//work/tasks/595/27790595/ignition-0.26.0-...
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |ngompa13@gmail.com Flags| |fedora-review?
--- Comment #15 from Neal Gompa ngompa13@gmail.com --- Taking this review.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #16 from Dusty Mabe dustymabe@redhat.com --- it was requested that I add ignition-dracut as a subpackage. While I originally didn't want to do this because of spec file complexity I gave in (you're welcome Neal :)). Here is updated information spec [1] SRPM [2] scratch build [3]
[1] https://raw.githubusercontent.com/dustymabe/ignition-rpm/3d83b5d89ce2613647a... [2] https://kojipkgs.fedoraproject.org//work/tasks/2449/27862449/ignition-0.26.0... [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=27862448
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #17 from Dusty Mabe dustymabe@redhat.com --- and to make fedora-review tool happy:
Spec URL: https://raw.githubusercontent.com/dustymabe/ignition-rpm/3d83b5d89ce2613647a... SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/2449/27862449/ignition-0.26.0...
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #18 from Neal Gompa ngompa13@gmail.com ---
%global dracutprovider_prefix %{provider}.%{provider_tld}/%{project}/%{repo} %global dracutimport_path %{provider_prefix}
fedora-review failed to validate the sources because the URL produced for Source1 was wrong. That was cuased by these two not being set to the correct macros. Please fix.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #19 from Dusty Mabe dustymabe@redhat.com --- fixed.
Spec URL: https://raw.githubusercontent.com/dustymabe/ignition-rpm/e60050652e3bf43c79e... SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/3005/27863005/ignition-0.26.0...
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #20 from Neal Gompa ngompa13@gmail.com --- *** Bug 1594378 has been marked as a duplicate of this bug. ***
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #21 from Neal Gompa ngompa13@gmail.com --- Package review notes:
[x] Package naming follows guidelines [x] Bundled components are correctly specified [x] Licensing is correctly documented [x] Macros are used consistently [x] No serious issues from rpmlint that aren't already being worked on [!] ignition-dracut does not do "Requires: %{name} = %{version}-%{release}" [!] Unnecessary "%defattr(-,root,root,0755)" in dracut subpackage
Please fix the following issues noted above.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #22 from Dusty Mabe dustymabe@redhat.com --- updated:
Spec URL: https://raw.githubusercontent.com/dustymabe/ignition-rpm/a789cdea4942165f80d...
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #23 from Neal Gompa ngompa13@gmail.com --- Looks good to me.
PACKAGE APPROVED.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #24 from Steve Milner smilner@redhat.com --- Per request, I officially hand off the package in this review to Dusty Mabe.
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #25 from Ralph Bean rbean@redhat.com --- (fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/ignition
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
--- Comment #26 from Ralph Bean rbean@redhat.com --- (fedrepo-req-admin): Processing with --force at the request of @mohanboddu
https://bugzilla.redhat.com/show_bug.cgi?id=1576879
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |NEXTRELEASE Last Closed| |2018-09-08 10:43:04
package-review@lists.fedoraproject.org