Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be available on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 1576879 - Review Request: ignition - First boot installer and configuration tool
Summary: Review Request: ignition - First boot installer and configuration tool
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1594378 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-05-10 15:31 UTC by Steve Milner
Modified: 2018-09-08 14:43 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-09-08 14:43:04 UTC
Type: ---
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Steve Milner 2018-05-10 15:31:58 UTC
Spec URL: https://copr-be.cloud.fedoraproject.org/results/dustymabe/ignition/fedora-28-x86_64/00732927-ignition/ignition.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/dustymabe/ignition/fedora-28-x86_64/00732927-ignition/ignition-0.23.0-0.3.fc28.src.rpm
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/

Comment 1 Colin Walters 2018-05-10 15:44:28 UTC
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.

Comment 2 Steve Milner 2018-05-10 16:15:34 UTC
Thanks Colin. I'll update, test, and upload changes to my Fedora People space.

Comment 3 Steve Milner 2018-05-10 16:28:39 UTC
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.

Comment 4 Derek Gonyeo 2018-05-10 16:56:39 UTC
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?

Comment 5 Steve Milner 2018-05-10 17:47:23 UTC
Derek,

Most likely it would be seperate. It's possible that the ignition package would eventually require said dracut module/config.

Comment 6 Dusty Mabe 2018-05-11 16:22:07 UTC
I think I agree with derek here. seems like ignition isn't that useful without the dracut modules so why not include them?

Comment 7 Steve Milner 2018-05-11 17:12:45 UTC
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.

Comment 8 Neal Gompa 2018-05-11 20:18:27 UTC
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. :)

Comment 9 Robert-André Mauchin 🐧 2018-05-12 20:37:16 UTC
 - 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

Comment 10 Colin Walters 2018-05-14 18:29:09 UTC
 - 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.

Comment 11 Steve Milner 2018-05-14 18:52:18 UTC
Colin,

Agreed.

Comment 12 Neal Gompa 2018-06-19 03:25:11 UTC
(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.

Comment 13 Dusty Mabe 2018-06-19 13:37:43 UTC
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/abdc98fbde943502c205f3d410991f7081225baa?branch=master

Comment 15 Neal Gompa 2018-06-25 15:43:52 UTC
Taking this review.

Comment 16 Dusty Mabe 2018-06-25 18:41:34 UTC
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/3d83b5d89ce2613647a4eb6b21c1a6bc66b52ca7/ignition.spec
[2] https://kojipkgs.fedoraproject.org//work/tasks/2449/27862449/ignition-0.26.0-0.1.git7610725.fc29.src.rpm
[3] https://koji.fedoraproject.org/koji/taskinfo?taskID=27862448

Comment 18 Neal Gompa 2018-06-25 19:14:36 UTC
> %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.

Comment 20 Neal Gompa 2018-06-25 19:33:29 UTC
*** Bug 1594378 has been marked as a duplicate of this bug. ***

Comment 21 Neal Gompa 2018-06-25 20:01:21 UTC
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.

Comment 23 Neal Gompa 2018-06-25 20:55:43 UTC
Looks good to me.

PACKAGE APPROVED.

Comment 24 Steve Milner 2018-06-26 13:39:07 UTC
Per request, I officially hand off the package in this review to Dusty Mabe.

Comment 25 Ralph Bean 2018-06-26 15:16:47 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/ignition

Comment 26 Ralph Bean 2018-06-26 15:17:45 UTC
(fedrepo-req-admin):  Processing with --force at the request of @mohanboddu


Note You need to log in before you can comment on or make changes to this bug.