Bug 1066629 - Review Request: openstack-tripleo - OpenStack TripleO
Review Request: openstack-tripleo - OpenStack TripleO
Status: CLOSED UPSTREAM
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Steven Dake
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-18 13:17 EST by James Slagle
Modified: 2016-04-26 17:37 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-04-01 21:21:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
sdake: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description James Slagle 2014-02-18 13:17:46 EST
Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/tripleo-incubator/openstack-tripleo.spec
SRPM URL: http://repos.fedorapeople.org/repos/openstack-m/openstack-m/fedora-20/SRPMS/openstack-tripleo-0.0.2-1.fc20.src.rpm
Description: TripleO is a program aimed at installing, upgrading and operating OpenStack
clouds using OpenStack's own cloud facilities as the foundations - building on
nova, neutron and heat to automate fleet management at datacenter scale.

Fedora Account System Username: slagle

koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=6543886

This is my first package submission for Fedora, and I need a sponsor. I have a few more package review requests that I'll be submitting for the other OpenStack TripleO related projects.

I'll look for some other reviews to review myself to help get up to speed on the process :).
Comment 1 Steven Dake 2014-02-19 10:25:22 EST
James,

I'll sponsor you.

Please provide some links to other bugs where you have done package reviews in the past.
Comment 2 Steven Dake 2014-02-19 19:03:56 EST
James,

I haven't taken a look at the SRPM, but these problems pop out from the spec file.

1)
Python packages need specific BuildRequires:
https://fedoraproject.org/wiki/Packaging:Python#BuildRequires

I'd add python-setuptools for good measure

2)
The installation process is wrong.  Please use the python installer to install packages.  For an example, check out:

https://github.com/sdake/fedora-reviews/blob/master/os-collect-config/os-collect-config.spec#L39

3)
The git snapshotting used in the spec file is wrong and does not offer a seamless upgrade process for users.  Please use a version of 0 and release of 0.1.snapshot.dist.  For an example check out:
com/sdake/fedora-reviews/blob/master/openstack-heat-templates/openstack-heat-templates.spec#L7

4)
Alignment is likely a mix of tabs and spaces.  Recommend sticking to tabs and aligning things on the nearest boundary that makes sense

5)
the %files section is wrong.  If something is commented out, that means the package doesn't work as you expect - recommend fixing.

6)
Recommend %doc any LICENSE or README

7) did you run the spec file, SRPM, and RPM through rpmlint and fix the resulting problems?

Regards
-steve
Comment 3 James Slagle 2014-02-20 08:26:35 EST
Hi Steve, thanks for the initial feedback.

I've made some updates:
Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/tripleo-incubator/openstack-tripleo.spec
SRPM URL: http://repos.fedorapeople.org/repos/openstack-m/openstack-m/fedora-20/SRPMS/openstack-tripleo-0.0.1-1.20140220git.fc20.src.rpm

1) & 2):
This is not a python package per se. There are 1 or 2 python scripts underneath scripts/, but no python modules. The upstream setup.{py,cfg}, doesn't actually install anything. If we used them to install, it would just create an empty python module called tripleo-incubator.

3) updated for review

4) updated for review

5) updated for review

6) updated for review

7) yes, there are several Warnings, mostly for conffile-without-noreplace-flag (which I don't want), and no-manual-page-for-binary (no manpages exist). Are these Ok?
Comment 4 James Slagle 2014-02-20 12:17:55 EST
Steve, here are the bugzilla links for the reviews I've been doing:
https://bugzilla.redhat.com/show_bug.cgi?id=1067445 (python-pyghmi)
https://bugzilla.redhat.com/show_bug.cgi?id=1067200 (os-collect-config)
https://bugzilla.redhat.com/show_bug.cgi?id=1067216 (os-refresh-config)
https://bugzilla.redhat.com/show_bug.cgi?id=1065562 (python-scp)
Comment 5 Steven Dake 2014-02-26 13:41:24 EST
James,

Can you provide an unofficial review for:
https://bugzilla.redhat.com/show_bug.cgi?id=1069335

Thanks
-steve
Comment 6 James Slagle 2014-02-27 09:54:05 EST
just completed an unofficial review for openstack-ironic
Comment 7 Steven Dake 2014-02-27 10:14:52 EST
James,

The review isn't complete until the package is fedora-review+ :)  But since you can't do that quite yet, please continue to drive the openstack-ironic review to conclusion.

Regards
-steve
Comment 8 Angus Thomas 2014-03-07 13:06:11 EST
Hi. I'll do an informal review

Is it safe to put all those executables into /usr/bin, rather than in a 
a tripleo-specific directory. Some of the names look dangerously generalised
e.g. /usr/bin/cleanup-env /usr/bin/set-os-type /usr/bin/user-config & 
/usr/bin/wait_for


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "Unknown or generated". 11 files have unknown license.
     Detailed output of licensecheck in /home/athomas/openstack-
     tripleo/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config /etc/tripleo

	Is the absence of noreplace justified?

[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: openstack-tripleo-0.0.1-1.20140220git.fc20.noarch.rpm
          openstack-tripleo-doc-0.0.1-1.20140220git.fc20.noarch.rpm
          openstack-tripleo-0.0.1-1.20140220git.fc20.src.rpm
openstack-tripleo.noarch: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/undercloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc-user
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/seedrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/cloudprompt
openstack-tripleo.noarch: W: devel-file-in-non-devel-package /usr/bin/user-config
openstack-tripleo.noarch: W: no-manual-page-for-binary get-vm-mac
openstack-tripleo.noarch: W: no-manual-page-for-binary os-make-password
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-clienttools
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-baremetal
openstack-tripleo.noarch: W: no-manual-page-for-binary cleanup-env
openstack-tripleo.noarch: W: no-manual-page-for-binary pull-tools
openstack-tripleo.noarch: W: no-manual-page-for-binary wait_for
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-endpoints
openstack-tripleo.noarch: W: no-manual-page-for-binary takeovernode
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_overcloud.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_end.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_seed.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary create-nodes
openstack-tripleo.noarch: W: no-manual-page-for-binary register-nodes
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-undercloud-passwords
openstack-tripleo.noarch: W: no-manual-page-for-binary assert-users
openstack-tripleo.noarch: W: no-manual-page-for-binary load-image
openstack-tripleo.noarch: W: no-manual-page-for-binary write-tripleorc
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-network
openstack-tripleo.noarch: W: no-manual-page-for-binary boot-seed-vm
openstack-tripleo.noarch: W: no-manual-page-for-binary assert-admin-users
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_variables.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-neutron
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_undercloud.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary init-keystone
openstack-tripleo.noarch: W: no-manual-page-for-binary user-config
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-env
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-overcloud-passwords
openstack-tripleo.noarch: W: no-manual-page-for-binary assert-user
openstack-tripleo.noarch: W: no-manual-page-for-binary configure-vm
openstack-tripleo.noarch: W: no-manual-page-for-binary os-adduser
openstack-tripleo.noarch: W: no-manual-page-for-binary install-dependencies
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-seed-vm
openstack-tripleo.noarch: W: no-manual-page-for-binary refresh-env
openstack-tripleo.noarch: W: no-manual-page-for-binary send-irc
openstack-tripleo.noarch: W: no-manual-page-for-binary set-os-type
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_ramdisk.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary stack-ready
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_setup.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary register-endpoint
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_testenv.sh
openstack-tripleo-doc.noarch: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.src: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.src: W: no-%build-section
3 packages and 0 specfiles checked; 0 errors, 52 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint openstack-tripleo-doc openstack-tripleo
openstack-tripleo-doc.noarch: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.noarch: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/undercloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc-user
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/seedrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/cloudprompt
openstack-tripleo.noarch: W: devel-file-in-non-devel-package /usr/bin/user-config
openstack-tripleo.noarch: W: no-manual-page-for-binary get-vm-mac
openstack-tripleo.noarch: W: no-manual-page-for-binary os-make-password
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-clienttools
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-baremetal
openstack-tripleo.noarch: W: no-manual-page-for-binary cleanup-env
openstack-tripleo.noarch: W: no-manual-page-for-binary pull-tools
openstack-tripleo.noarch: W: no-manual-page-for-binary wait_for
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-endpoints
openstack-tripleo.noarch: W: no-manual-page-for-binary takeovernode
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_overcloud.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_end.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_seed.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary create-nodes
openstack-tripleo.noarch: W: no-manual-page-for-binary register-nodes
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-undercloud-passwords
openstack-tripleo.noarch: W: no-manual-page-for-binary assert-users
openstack-tripleo.noarch: W: no-manual-page-for-binary load-image
openstack-tripleo.noarch: W: no-manual-page-for-binary write-tripleorc
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-network
openstack-tripleo.noarch: W: no-manual-page-for-binary boot-seed-vm
openstack-tripleo.noarch: W: no-manual-page-for-binary assert-admin-users
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_variables.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-neutron
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_undercloud.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary init-keystone
openstack-tripleo.noarch: W: no-manual-page-for-binary user-config
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-env
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-overcloud-passwords
openstack-tripleo.noarch: W: no-manual-page-for-binary assert-user
openstack-tripleo.noarch: W: no-manual-page-for-binary configure-vm
openstack-tripleo.noarch: W: no-manual-page-for-binary os-adduser
openstack-tripleo.noarch: W: no-manual-page-for-binary install-dependencies
openstack-tripleo.noarch: W: no-manual-page-for-binary setup-seed-vm
openstack-tripleo.noarch: W: no-manual-page-for-binary refresh-env
openstack-tripleo.noarch: W: no-manual-page-for-binary send-irc
openstack-tripleo.noarch: W: no-manual-page-for-binary set-os-type
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_ramdisk.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary stack-ready
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_setup.sh
openstack-tripleo.noarch: W: no-manual-page-for-binary register-endpoint
openstack-tripleo.noarch: W: no-manual-page-for-binary devtest_testenv.sh
2 packages and 0 specfiles checked; 0 errors, 50 warnings.
# echo 'rpmlint-done:'



Requires
--------
openstack-tripleo-doc (rpmlib, GLIBC filtered):
    openstack-tripleo

openstack-tripleo (rpmlib, GLIBC filtered):
    /bin/bash
    /bin/sh
    /usr/bin/env
    config(openstack-tripleo)



Provides
--------
openstack-tripleo-doc:
    openstack-tripleo-doc

openstack-tripleo:
    config(openstack-tripleo)
    openstack-tripleo



Source checksums
----------------
https://github.com/openstack/tripleo-incubator/archive/d305ad25f2538d829465092146de5cdfb4a803d8.tar.gz :
  CHECKSUM(SHA256) this package     : fd5965b9ef811109e7d8be1754ba5b141df2e74700d5d005df6cc3c243f21186
  CHECKSUM(SHA256) upstream package : fd5965b9ef811109e7d8be1754ba5b141df2e74700d5d005df6cc3c243f21186


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review --rpm-spec -n /home/athomas/rpmbuild/SRPMS/openstack-tripleo-0.0.1-1.20140220git.fc20.src.rpm
Buildroot used: fedora-20-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
Comment 9 Steven Dake 2014-03-07 15:13:05 EST
Angus,

There are a bit of steps beyond just providing the output of the fedora-review tool.  The fedora-review tool makes life a whole bunch easier, but each step should be confirmed.

Especially the rpmlint out should be analyzed to identify blockers from the reviewers perspective about what needs to be fixed versus which can be ignored.
Comment 10 Steven Dake 2014-03-10 18:00:20 EDT
James,

The loads of binaries in /usr/bin needs to be fixed.  You have a couple options.

1) Prefix every binary with tripleo

or

2) If the binaries are only meant to be called by other tripleo tools and not the command line put them in /usr/libexec/openstacktripleo

If neither of these will work, let me know.

Regards
-steve
Comment 11 Steven Dake 2014-03-10 18:07:24 EDT
(In reply to James Slagle from comment #3)
> Hi Steve, thanks for the initial feedback.
> 
> I've made some updates:
> Spec URL:
> https://raw.github.com/agroup/tripleo-rpm-spec-files/master/tripleo-
> incubator/openstack-tripleo.spec
> SRPM URL:
> http://repos.fedorapeople.org/repos/openstack-m/openstack-m/fedora-20/SRPMS/
> openstack-tripleo-0.0.1-1.20140220git.fc20.src.rpm
> 
> 1) & 2):
> This is not a python package per se. There are 1 or 2 python scripts
> underneath scripts/, but no python modules. The upstream setup.{py,cfg},
> doesn't actually install anything. If we used them to install, it would just
> create an empty python module called tripleo-incubator.
> 
> 3) updated for review
> 
> 4) updated for review
> 
> 5) updated for review
> 
> 6) updated for review
> 
> 7) yes, there are several Warnings, mostly for
> conffile-without-noreplace-flag (which I don't want), and
> no-manual-page-for-binary (no manpages exist). Are these Ok?

If you want the conffile without noreplace, please give a rationale.  Anything is fine, as long as it is recorded in the review.  People may come along later and wonder "why" without understanding the packagers rationale.

For manual pages, it is generally proper to request the packager to request the upstream to produce manual pages in their bug tracker.  In the case of OpenStack, upstream doesn't care for manual pages, so there is little we can do about this point.
Comment 12 Steven Dake 2014-03-10 18:08:32 EDT
(In reply to Steven Dake from comment #10)
> James,
> 
> The loads of binaries in /usr/bin needs to be fixed.  You have a couple
> options.
> 
> 1) Prefix every binary with tripleo
> 
> or
> 
> 2) If the binaries are only meant to be called by other tripleo tools and
> not the command line put them in /usr/libexec/openstacktripleo

openstack-tripleo - sorry keyboard miss :)
> 
> If neither of these will work, let me know.
> 
> Regards
> -steve
Comment 13 James Slagle 2014-03-11 20:28:27 EDT
(In reply to Steven Dake from comment #10)
> James,
> 
> The loads of binaries in /usr/bin needs to be fixed.  You have a couple
> options.
> 
> 1) Prefix every binary with tripleo

That wouldn't be ideal due to how several of the scripts call one another.

> 
> or
> 
> 2) If the binaries are only meant to be called by other tripleo tools and
> not the command line put them in /usr/libexec/openstacktripleo

They do need to be called from the cli. How about putting them under /usr/libexec/openstacktripleo, and then also packaging an rc file that can be sourced to manipulate the user's $PATH?

> 
> If neither of these will work, let me know.
> 
> Regards
> -steve
Comment 14 Steven Dake 2014-03-11 21:31:18 EDT
(In reply to James Slagle from comment #13)
> (In reply to Steven Dake from comment #10)
> > James,
> > 
> > The loads of binaries in /usr/bin needs to be fixed.  You have a couple
> > options.
> > 
> > 1) Prefix every binary with tripleo
> 
> That wouldn't be ideal due to how several of the scripts call one another.
> 
> > 
> > or
> > 
> > 2) If the binaries are only meant to be called by other tripleo tools and
> > not the command line put them in /usr/libexec/openstacktripleo
> 
> They do need to be called from the cli. How about putting them under
> /usr/libexec/openstacktripleo, and then also packaging an rc file that can
> be sourced to manipulate the user's $PATH?
> 
What I mean here is do they need to be called directly from the shell as CLIs?  If the answer is no, they can go directly in /usr/libexec/openstack-tripleo/* and the references to their calling can be done by changing the path.  Another option is as you mentioned packaging an rc file possibly in profile.d.  I'm not quite sure what the best way to proceed here is.

I've added pbrady to the review CC and sent him an email for further comment.
> > 
> > If neither of these will work, let me know.
> > 
> > Regards
> > -steve
Comment 15 Pádraig Brady 2014-03-11 21:42:25 EDT
I see all the binaries listed in comment #8

They really should be confined to /usr/libexec/openstack-tripleo

If they are to be exposed to a standard user shell then they
should be prefixed with something like tripleo-...
However having so many CLI entry point commands available
is very questionable, and I suspect some refactoring/wrapping
from a single /usr/bin/tripleo command that calls out/includes
these functions would be the way forward.
Comment 16 James Slagle 2014-03-12 08:26:27 EDT
(In reply to Pádraig Brady from comment #15)
> I see all the binaries listed in comment #8
> 
> They really should be confined to /usr/libexec/openstack-tripleo
> 
> If they are to be exposed to a standard user shell then they
> should be prefixed with something like tripleo-...
> However having so many CLI entry point commands available
> is very questionable, and I suspect some refactoring/wrapping
> from a single /usr/bin/tripleo command that calls out/includes
> these functions would be the way forward.

These are all scripts that upstream tripleo has written to aid in doing OpenStack deployments. Longer term, the hope is they will be integrated back into the relevant OpenStack projects (e.g., init-keystone back into keystone as something else). Or, replaced entirely with calls to the tuskar cli.

Many of them need to be called from the CLI.

Given that, can I do a hybrid approach? I'm thinking moving them to get installed under /usr/libexec/openstack-tripleo, and then adding a /usr/bin/tripleo which is pretty much just:

#!/bin/bash
/usr/libexec/openstack-tripleo/$1

A large number of the scripts will need patches to add /usr/libexec/openstack-tripleo to $PATH though. I think that would be easier than prefixing all the calls with /usr/bin/tripleo. That would just be a maintenance burden every time we sync with upstream.

Please let me know if this would be acceptable.
Comment 17 Pádraig Brady 2014-03-12 18:15:49 EDT
James that would be great, thanks.

If the scripts were only ever initiated from that /usr/bin/tripleo script,
then exporting an adjusted PATH there before calling the /usr/libexec script might suffice. If not then it might be possible to add a PATH adjustment
line at the start of the each of each script (assuming all shell script),
by doing something like this in the spec:

sed -i '2iexport PATH="$PATH:/usr/libexec/openstack-tripleo"' %{buildroot}%{_bindir}/*
Comment 18 Pádraig Brady 2014-03-12 18:27:26 EDT
BTW the above is only illustrative and would be better to also have a condition inserted that at runtime checked whether our entry was in the path.
That logic would be best centralized in say /usr/libexec/openstack-tripleo/settings and then each of the scripts could be adjusted to:

sed -i '2i source /usr/libexec/openstack-tripleo/settings' %{buildroot}%{_bindir}/*
Comment 19 James Slagle 2014-03-13 12:05:21 EDT
Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/tripleo-incubator/openstack-tripleo.spec
SRPM URL: http://repos.fedorapeople.org/repos/openstack-m/openstack-m/fedora-20/SRPMS/openstack-tripleo-0.0.2-1.20140220git.fc20.src.rpm

I've moved the binaries to be under /usr/libexec/openstack-tripleo, with just a single /usr/bin/tripleo wrapper script.

For some reason the rpmlint output is saying:
openstack-tripleo.src: W: strange-permission tripleo 0755L

I'm not sure why that's the case. Those are the permissions I want for /usr/bin/tripleo.

The rc files under /etc/tripleo should not be noreplace, they aren't meant to be user edited. Similar to /etc/bashrc, etc. I added a comment to the spec file as well.

I believe I've addressed all the review feedback to date. Thanks!
Comment 20 Steven Dake 2014-03-13 13:00:37 EDT
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "Unknown or generated". 11 files have unknown license.
     Detailed output of licensecheck in /home/sdake/fedora-review/1066629
     -openstack-tripleo/licensecheck.txt
[-]: License file installed when any subpackage combination is installed.
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/libexec/openstack-tripleo
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/libexec/openstack-tripleo
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config /etc/tripleo

These are justified in the spec comments

[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====
Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.
Rpmlint
-------
Checking: openstack-tripleo-0.0.2-1.20140220git.fc20.noarch.rpm
          openstack-tripleo-doc-0.0.2-1.20140220git.fc20.noarch.rpm
          openstack-tripleo-0.0.2-1.20140220git.fc20.src.rpm
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/undercloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc-user
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/seedrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/cloudprompt
openstack-tripleo.noarch: W: no-manual-page-for-binary tripleo

these are ok

openstack-tripleo.src: W: strange-permission tripleo 0755L

my speculation is because the directory has write permissions and datadir is meant for read only data.  try removing the execute bit.  If that doesn't fix it, we will just have to ignore this.  Also recommend changing the macro to {%_datadir}.  It isn't any different, but it could be in the future.  Reference:

https://fedoraproject.org/wiki/Packaging:RPMMacros

openstack-tripleo.src: W: no-%build-section

this is fine

3 packages and 0 specfiles checked; 0 errors, 8 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint openstack-tripleo-doc openstack-tripleo
openstack-tripleo-doc.noarch: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.noarch: W: spelling-error %description -l en_US datacenter -> data center, data-center, centerboard
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/undercloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc-user
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/overcloudrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/seedrc
openstack-tripleo.noarch: W: conffile-without-noreplace-flag /etc/tripleo/cloudprompt
openstack-tripleo.noarch: W: no-manual-page-for-binary tripleo
2 packages and 0 specfiles checked; 0 errors, 8 warnings.
# echo 'rpmlint-done:'
Requires
--------
openstack-tripleo-doc (rpmlib, GLIBC filtered):
    openstack-tripleo

openstack-tripleo (rpmlib, GLIBC filtered):
    /bin/bash
    /bin/sh
    /usr/bin/env
    config(openstack-tripleo)



Provides
--------
openstack-tripleo-doc:
    openstack-tripleo-doc

openstack-tripleo:
    config(openstack-tripleo)
    openstack-tripleo



Source checksums
----------------
https://github.com/openstack/tripleo-incubator/archive/d305ad25f2538d829465092146de5cdfb4a803d8.tar.gz :
  CHECKSUM(SHA256) this package     : fd5965b9ef811109e7d8be1754ba5b141df2e74700d5d005df6cc3c243f21186
  CHECKSUM(SHA256) upstream package : fd5965b9ef811109e7d8be1754ba5b141df2e74700d5d005df6cc3c243f21186


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review -b 1066629
Buildroot used: fedora-20-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
Comment 21 Steven Dake 2014-03-13 13:02:16 EDT
James,

The package is looking really good.  Please fix the following things, and I'll approve the package for SCM.

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/libexec/openstack-tripleo

penstack-tripleo.src: W: strange-permission tripleo 0755L

my speculation is because the directory has write permissions and datadir is meant for read only data.  try removing the execute bit.  If that doesn't fix it, we will just have to ignore this.  Also recommend changing the macro to {%_datadir}.  It isn't any different, but it could be in the future.  Reference:

https://fedoraproject.org/wiki/Packaging:RPMMacros

Regards,
-steve
Comment 22 James Slagle 2014-03-13 14:04:42 EDT
Spec URL: https://raw.github.com/agroup/tripleo-rpm-spec-files/master/tripleo-incubator/openstack-tripleo.spec
SRPM URL: http://repos.fedorapeople.org/repos/openstack-m/openstack-m/fedora-20/SRPMS/openstack-tripleo-0.0.2-2.20140220git.fc20.src.rpm

Package now owns /usr/libexec/openstack-tripleo.

Switched to using %{_datadir} instead of %{_datarootdir}

Still not sure what is going on with /usr/bin/tripleo. It doesn't live under datadir, and is supposed to be executable. Incidentally, I don't get the warning when running rpmlint locally against a built rpm. I only see it when using fedora-review.  I'm fine to ignore it if you are :).
Comment 23 Steven Dake 2014-03-18 13:12:01 EDT
James,

Apologies for not catching this earlier, but the -doc package should have a %doc LICENSE and README section.  While not necessarily mandatory by the review standards, I think it helps reduce confusion about what package a license has.

If you could address that with a 1 liner change, I'll go ahead and approve this package.
Comment 25 Steven Dake 2014-03-19 20:11:22 EDT
Comment #24 package is APPROVED.
Comment 26 James Slagle 2014-03-20 15:19:17 EDT
New Package SCM Request
=======================
Package Name: openstack-tripleo
Short Description: OpenStack TripleO
Owners: slagle
Branches: f20
InitialCC:
Comment 27 Gwyn Ciesla 2014-03-20 16:08:12 EDT
Git done (by process-git-requests).

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