Bug 1451124 - (cni) Review Request: containernetworking-cni
Summary: (cni) Review Request: containernetworking-cni
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jan Chaloupka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-05-15 21:32 UTC by Timothy St. Clair
Modified: 2020-05-31 09:22 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-05-31 09:22:25 UTC
Type: Bug
Embargoed:
jchaloup: fedora-review+


Attachments (Terms of Use)

Description Timothy St. Clair 2017-05-15 21:32:13 UTC
Spec URL: https://github.com/timothysc/containernetworking/blob/master/containernetworking.spec

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

Description: The CNI (Container Network Interface) project consists of a specification 
and libraries for writing plugins to configure network interfaces in Linux 
containers, along with a number of supported plugins. CNI concerns itself 
only with network connectivity of containers and removing allocated resources 
when the container is deleted. 

Fedora Account System Username: tstclair

Comment 1 Jan Chaloupka 2017-05-16 12:23:35 UTC
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

Comment 2 Timothy St. Clair 2017-05-16 13:36:46 UTC
Some of the auto-generated portions make my eyes sore,  I'll merge the two and repost today.

Comment 3 Timothy St. Clair 2017-05-16 21:44:57 UTC
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.

Comment 4 Jan Chaloupka 2017-05-18 11:25:22 UTC
> # 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.

Comment 5 Timothy St. Clair 2017-05-18 14:39:07 UTC
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

Comment 6 Jan Chaloupka 2017-05-18 19:59:30 UTC
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

Comment 7 Timothy St. Clair 2017-05-18 20:38:18 UTC
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

Comment 8 Mattia Verga 2020-05-31 09:22:25 UTC
This package was approved and imported in repositories and it was later retired, but this review ticket was never closed.
I'm closing it now.


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