[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124



--- Comment #7 from Timothy St. Clair  ---
Looks like the process has changed... but I'll still drop this here. 

New Package SCM Request
===
Package Name: containernetworking-cni
Short Description: Libraries for writing CNI plugin
Owners: tstclair, jchaloup
Branches: f26, epel7

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124

Jan Chaloupka  changed:

   What|Removed |Added

  Flags||fedora-review+



--- Comment #6 from Jan Chaloupka  ---
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=19609820

$ rpmlint containernetworking-cni-debuginfo-0.5.1-1.fc24.x86_64.rpm
containernetworking-cni-unit-test-devel-0.5.1-1.fc24.x86_64.rpm
containernetworking-cni-devel-0.5.1-1.fc24.noarch.rpm
containernetworking-cni-0.5.1-1.fc24.x86_64.rpm
containernetworking-cni-0.5.1-1.fc24.src.rpm
containernetworking-cni-unit-test-devel.x86_64: W: spelling-error %description
-l en_US github -> git hub, git-hub, GitHub
containernetworking-cni-devel.noarch: W: spelling-error %description -l en_US
github -> git hub, git-hub, GitHub
containernetworking-cni.x86_64: W: no-manual-page-for-binary cnitool
5 packages and 0 specfiles checked; 0 errors, 3 warnings.

LGTM, approving

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124



--- Comment #5 from Timothy St. Clair  ---
Updates: 
- Updated name per comment 
- Moved some comments as TODO to cleanup later. 

Spec URL:
https://raw.githubusercontent.com/timothysc/containernetworking-cni/master/containernetworking-cni.spec

SRPM URL:
https://github.com/timothysc/containernetworking-cni/raw/master/containernetworking-cni-0.5.1-1.fc27.src.rpm

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124



--- Comment #4 from Jan Chaloupka  ---
> # Obsoletes: kubernetes-cni

There is no kubernetes-cni in Fedora so far so no need to obsolete one

> # I think this is brittle vs. the build tooling as it will 

Other option is to patch ./build to use gcc-go in case some architectures
requires it. I agree that every time the package is updated the list needs to
be updated as well. Another option is to copy:
```sh
PLUGINS="plugins/meta/* plugins/main/* plugins/ipam/* plugins/test/*"
for d in $PLUGINS; do
if [ -d $d ]; then
plugin=$(basename $d)
echo "  " $plugin
%gobuild -o bin/$plugin %{import_path}/$d
fi
done
```
and replace all the %gobuild -o bin/plugins/ like lines.

> # why can't you go test ./...

Very often some of the tests fail due to incomplete environment configuration.
Given the list the failing tests can be commented out to run at least
something. At the same time the failing tests can be commented with a reason
why they are failing. Once a CI take care of the tests, the %check section will
disappear.

> Name: containernetworking

Given the project import path prefix is github.com/containernetworking/cni, I
would stick with containernetworking-cni name in case other projects will
appear under github.com/containernetworking. In future, they may be
github.com/somecompany/containernetworking project which would collide with the
name.

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124



--- Comment #3 from Timothy St. Clair  ---
I just cleaned up the spec that you had sent, I don't have strong opinions on
the format, there are minor comments in the .spec file.

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124



--- Comment #2 from Timothy St. Clair  ---
Some of the auto-generated portions make my eyes sore,  I'll merge the two and
repost today.

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124



--- Comment #1 from Jan Chaloupka  ---
Given the cni is a go project:
- if it is possible we should package entire devel subpackage (including its
dependencies)
- if it is possible the project should be built from bundled dependencies
- if possible, project tests should be run
- if possible, the project should be built over all available architectures
- if possible, the spec file header should contain macros that reference
project location

I have generated the new spec file via gofed and incorporated part of your spec
file into it [1].

Currently (referring to your spec file):
- project is built from bundled dependencies (I would recommended that only in
cases when there is no other way)
- devel subpackage is missing build-time and run-time dependencies
- tests are not run (given some of the tests need to be run as a root and
others need additional configuration, they do not have to be run)
- no unit-test subpackage: even if the tests are not run, the _test.go files
can by analysed by tooling and/or run in specialized CI so it is always useful
to package the unit-test package as well
- though there is no strict structure of the spec file header, I recommend to
define provider_prefix, import_path and commit macros. I got a tooling that
periodically scans all go projects in Fedora rawhide and collects various
data/metadata from the go rpms. Plus, if the macros are set, you can use gofed
tooling to update the spec file.
- exclusive archs are set to %{go_arches}: out aim is to generate spec files
that can be built over various architectures and OSes. Unfortunately, the
%{go_arches} macro is not present on CentOS or RHEL so the current spec file
with work on Fedora only. Check [1] for the general ExclusiveArch value. The
same holds for golang BuildRequires.
- in Fedora %gobuild macro is defined that encompasses the BUILD_ID for you so
there is no need to do that manually. However, the same limitations hold here.
The macro is present on Fedora only so the macro is redefine at spec file
header if not defined, see [1].
- I would not call the ./build script as it usually runs more than is needed
and hides the building itself. As long as the build steps are simple, I would
recommend to put them into the %build section.
- not sure about the name, kubernetes uses kubernetes-cni, which I would see
instead of containernetworking. But, I have no strong preference over the name
so basically any name that is close to cni is acceptable.

The references spec file [1] is an example, not something you need to follow.
Yeah, it is pretty long and there is a lot of constructions that make it hard
to read. But, it has its advantages.

[1] https://github.com/gofed/reviews/pull/5/files

-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124

Jan Chaloupka  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED



-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124

Timothy St. Clair  changed:

   What|Removed |Added

   Priority|unspecified |medium



-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1451124] (cni) Review Request: containernetworking - CNI Plugins

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1451124

Timothy St. Clair  changed:

   What|Removed |Added

 CC||jchal...@redhat.com,
   ||tstcl...@heptio.com
   Assignee|nob...@fedoraproject.org|jchal...@redhat.com



-- 
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
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org