Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1512925 - Review Request: golang-ecosystem - The ecosystem of packages for the Go programming language
Summary: Review Request: golang-ecosystem - The ecosystem of packages for the Go progr...
Keywords:
Status: ON_QA
Alias: None
Product: Fedora Modules
Classification: Fedora
Component: Module Review
Version: unspecified
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Nils Philippsen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1500502
Blocks: 1502378 1502381 1502382
TreeView+ depends on / blocked
 
Reported: 2017-11-14 12:52 UTC by Lokesh Mandvekar
Modified: 2018-03-13 06:30 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of: 1500502
Environment:
Last Closed:
nphilipp: fedora-review+


Attachments (Terms of Use)

Description Lokesh Mandvekar 2017-11-14 12:52:57 UTC
Modulemd URL: https://raw.githubusercontent.com/lsm5/go-md2man-module/master/go-md2man.yaml

Description: go-md2man is a golang tool using blackfriday to process markdown into manpages.

Fedora Account System Username: lsm5

Comment 1 Nils Philippsen 2017-11-15 16:23:46 UTC
Here are the issues I found with the modulemd file:

- yamllint reports errors:

nils@gibraltar:~/devel/reviews/fedora/1512925> yamllint go-md2man.yaml 
go-md2man.yaml
  1:1       warning  missing document start "---"  (document-start)
  5:81      error    line too long (96 > 80 characters)  (line-length)
  7:22      error    trailing spaces  (trailing-spaces)

- Not a formal error, but the module description refers to implementation details (i.e. that it's implemented with golang, blackfriday) that shouldn't be important to the user of the module. Something like this should get to the point: "The go-md2man tool converts markdown documents into manpages."

- The license/module field covers how the modulemd file is licensed, not the content/the packages that go into it. It also has to be a list: "Every module MUST contain a license section and declare a list of the module's licenses. Note these aren't the module's components' licenses." If you don't have a reason to deviate just stick with the default:

    license:
        module:
            - MIT

- You mix master and f27 streams in the build dependencies, this will attempt to pull in different streams of the platform module and probably break at some point. Please switch the stream name of the perl module to 5.24 or 5.26.

- The upstream references should exist, otherwise leave them out. E.g. you could use this (granted the documentation one is lame ;):

    references:
        community: https://github.com/cpuguy83/go-md2man
        documentation: https://github.com/cpuguy83/go-md2man/blob/master/go-md2man.1.md
        tracker: https://github.com/cpuguy83/go-md2man/issues

- profiles/*/rpms references binary packages that get installed with the respective profile, but golang-github-cpuguy83-go-md2man is a source package (the binary one is golang-github-cpuguy83-go-md2man-devel but shouldn't be installed on end user systems). I guess it should simply be left out here, possibly even be filtered out:

    filter:
        rpms:
            - golang-github-cpuguy83-go-md2man-devel

- components/rpms/golang-github-russross-blackfriday/rationale should state that it is a build dependency for golang-github-cpuguy83-go-md2man

- components/rpms/*/rationale fields should end with a period.

On a final note, I'm a little bit worried that we'll see a module for every single (or 2, or 5) golang related SRPM. Have you and your colleagues discussed if there are better ways to handle this? For instance, the Python folks have python2/3-ecosystem modules for widely used Python packages, but this might not be suitable for the Go language where bundling build dependencies might be a better approach.

Comment 2 Lokesh Mandvekar 2017-11-23 20:48:19 UTC
There's no strong preference for the go-md2man module name, and I'll just go ahead with golang-ecosystem module name to keep it consistent with python.

Modulemd URL: https://raw.githubusercontent.com/lsm5/golang-ecosystem/master/go-lang-ecosystem.yaml

Comment 3 Nils Philippsen 2017-11-24 09:52:32 UTC
I think you mean https://raw.githubusercontent.com/lsm5/golang-ecosystem/master/golang-ecosystem.yaml (with one dash fewer).

Apart from that, if you want to develop this as a fully-fledged ecosystem module the references need to cover all of that and not only md2man eventually. I'm not sure if there are any helpful guidelines for this scenario, though ;).

This module is APPROVED. Before requesting the dist-git repo to be created, you need to come up with a scheme for stream names (we don't want streams tied to specific Fedora versions if we can avoid it). Because the components in the module don't seem to share versions, I'd suggest making it time-based, e.g. like the system-tools module whose stream is 2017.0 which is the year plus an integer that can be incremented if necessary. You'd then create a new stream when introducing backwards-incompatible changes.

Comment 4 Nils Philippsen 2017-11-24 10:04:04 UTC
Damn, oversaw something: The license fields need to be lists even if you only specify one license. Currently you have:


    license:
        module: MIT

This needs to be either...:

    license:
        module:
            - MIT

... or:

    license:
        module: [ MIT ]

Comment 5 Lokesh Mandvekar 2017-11-27 15:18:10 UTC
See: https://raw.githubusercontent.com/lsm5/golang-ecosystem/2017.0/golang-ecosystem.yaml

As far as ecosystem references go, I hope it's ok if we update those as and when we see the need to include more packages to it. Right now, go-md2man is the only thing that comes to mind as far as deps go to build docker and others.

Comment 6 Lokesh Mandvekar 2017-11-27 15:21:45 UTC
2017.0 stream name format sounds good to me and the URL above includes that as well.

Comment 7 Nils Philippsen 2017-11-27 15:35:03 UTC
Looks good to me. This module is APPROVED.

Comment 8 Gwyn Ciesla 2017-11-29 09:58:01 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/modules/golang-ecosystem. You may commit to the branch "2017.0" in about 10 minutes.

Comment 9 Lokesh Mandvekar 2018-03-13 06:30:51 UTC
built here: https://koji.fedoraproject.org/koji/buildinfo?buildID=1057280


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