Informal Review:

Guessing this should be added to RDO: https://openstack.redhat.com/packaging/rdo-packaging.html#_how_to_add_a_new_package_to_rdo_master_packaging

Take a look at:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag for versioning and dist tag

See https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL to set Source0 to a URL (or a comment)

Requires and BuildRequires are fragmented, which makes the spec harder to read.

Not sure where the 'ceilometer' group is created, but you might want to make a note about, I'm assuming it gets created by another openstack rpm, but i'm not sure.
https://fedoraproject.org/wiki/Packaging:UsersAndGroups

%license is missing

Download and run 'fedora-review -b 1243550' you will see other errors and messages that  will need to be cleaned up.  https://fedoraproject.org/wiki/Package_Review_Process

Spec URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh.spec
SRPM URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh-1.0.0-1.fc22.src.rpm

Since we released and tagged 1.0, here is the new updated spec.

## Blockers


* I'll need your help to properly understand ceilometer/aodh relationship and figure out the proper upgrade path.

I pretty much get that openstack-aodh-{notifier,evaluator} services replace services with the same in ceilometer.
Problem is that openstack-ceilometer-{notifier,evaluator} were shipped in a single openstack-ceilometer-alarm package, previously.

=> You already obsoletes openstack-ceilometer-alarm but it's unversioned, should be:
Provides: openstack-ceilometer-alarm = 2:%{version}-%{release}
Obsoletes: openstack-ceilometer-alarm < last n-v-r with epoch of openstack-ceilometer-alarm

As ceilometer switched to semantic versioning, we need to carry epoch from ceilometer in our provides/obsoletes. No need to add epoch to the package though

=> Problem is that your main package and  doesn't require openstack-aodh-{notifier,evaluator} and hence the upgrade path is still incorrect.
So either we create a subpackage openstack-aodh-alarm-compat requiring all the packages providing features from openstack-ceilometer-alarm, so we just 


Relevant guidelines
[1] https://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages#.28n:m.29_Many_to_many_replacement

If you're aware of other similar features mapping in ceilometer/aodh, let me know so we could figure out the upgrade path.

* use versioned python macros

* I exempt this package from newer python guidelines as we need to make sure that the upgrade path from ceilometer alarm is ok before renaming stuff.
Moreover, openstack-xxx packages will be retired from F24 at a later point.

(In reply to Haïkel Guémar from comment #3)

Thanks for the review. Replying inline:

> ## Blockers
> 
> 
> * I'll need your help to properly understand ceilometer/aodh relationship
> and figure out the proper upgrade path.
> 
> I pretty much get that openstack-aodh-{notifier,evaluator} services replace
> services with the same in ceilometer.
> Problem is that openstack-ceilometer-{notifier,evaluator} were shipped in a
> single openstack-ceilometer-alarm package, previously.


That is correct. We discussed weather to keep these together or separate. The reason i split is because these services can be run on thier own nodes. Which would be hard to do if its a single package? If the recommendation is to keep these together as part of deprecation then i understand and can change it. 


> 
> => You already obsoletes openstack-ceilometer-alarm but it's unversioned,
> should be:
> Provides: openstack-ceilometer-alarm = 2:%{version}-%{release}
> Obsoletes: openstack-ceilometer-alarm < last n-v-r with epoch of
> openstack-ceilometer-alarm
> 
> As ceilometer switched to semantic versioning, we need to carry epoch from
> ceilometer in our provides/obsoletes. No need to add epoch to the package
> though


Yep understand, reason i dint add it yet is i was not sure which version to put in. Since liberty is not out yet? Ideally this version should be the final version of ceilometer-alarm package? Is it ok to add the current version and update later?


> 
> => Problem is that your main package and  doesn't require
> openstack-aodh-{notifier,evaluator} and hence the upgrade path is still
> incorrect.
> So either we create a subpackage openstack-aodh-alarm-compat requiring all
> the packages providing features from openstack-ceilometer-alarm, so we just 

I was thinking i can make the aodh-common obsolete ceilometer-alarm. Wouldnt that work? since aodh-common will be required by all sub-packages?


> 
> 
> Relevant guidelines
> [1]
> https://fedoraproject.org/wiki/
> Upgrade_paths_%E2%80%94_renaming_or_splitting_packages#.28n:m.
> 29_Many_to_many_replacement
> 
> If you're aware of other similar features mapping in ceilometer/aodh, let me
> know so we could figure out the upgrade path.
> 
> * use versioned python macros
> 
> * I exempt this package from newer python guidelines as we need to make sure
> that the upgrade path from ceilometer alarm is ok before renaming stuff.
> Moreover, openstack-xxx packages will be retired from F24 at a later point.

> That is correct. We discussed weather to keep these together or separate.
> The reason i split is because these services can be run on thier own nodes.
> Which would be hard to do if its a single package? If the recommendation is
> to keep these together as part of deprecation then i understand and can
> change it. 
> 

Thanks, I appreciate that you made it a bit clearer for me. :)
+1 for splitting the services in separate packages.

 
> 
> Yep understand, reason i dint add it yet is i was not sure which version to
> put in. Since liberty is not out yet? Ideally this version should be the
> final version of ceilometer-alarm package? Is it ok to add the current
> version and update later?
> 

Workaround would using a n-v-r like EPOCH:2015.1.99-1 that will be never attained by kilo point-release.


> 
> I was thinking i can make the aodh-common obsolete ceilometer-alarm. Wouldnt
> that work? since aodh-common will be required by all sub-packages?
> 

When you'll update existing deployments aodh-common will be indeed installed but won't pull aodh-notifier/evaluator in turn.
Let's have a dummy -compat subpackage that does the obsoletions and requires the services.
The good thing, upgrades will work smoothly and new deployments won't be affected an benefit from the split.

Thanks! addressed your comments, New spec and src are here:

Spec URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh.spec
SRPM URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh-1.0.0-1.fc22.src.rpm

* Latest tarball was not uploaded in PyPI, could you get it fixed?
If we can't, we could switch to launchpad url but they have OpenStack release name there.
* License should be ASL 2.0
* We have to fix oslo.messaging first (needs oslo.service first) => I'm currently fixing it.

Minor stuff:
* wrapping openstack-aodh-compat description (line too long)
* cleanup tabs from spec files

(In reply to Haïkel Guémar from comment #7)
> * Latest tarball was not uploaded in PyPI, could you get it fixed?
> If we can't, we could switch to launchpad url but they have OpenStack
> release name there.


This is already in progress. The upstream pypi upload job was missing. The pending review is here: https://review.openstack.org/#/c/222579/


> * License should be ASL 2.0


Sure, will fix that.


> * We have to fix oslo.messaging first (needs oslo.service first) => I'm
> currently fixing it.
> 
> Minor stuff:
> * wrapping openstack-aodh-compat description (line too long)
> * cleanup tabs from spec files

Will clean these up.

Addressed the above comments:

Spec URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh.spec
SRPM URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh-1.0.0-1.fc22.src.rpm

Hi Prad, I was able to run aodh services now with fixed packages, still some changes needed:
* %{__python} to %{__python2} (by default, the former will be /usr/bin/python3 in Fedora)
* You should move %license LICENSE to python-aodh. If you install python-aodh, it doesn't install license file and it's pulled by common which itself is pulled by all the other subpackages.

Thanks Haikel!

Addressed the above comments:

Spec URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh.spec
SRPM URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh-1.0.0-1.fc22.src.rpm

Please review.

One small oversight => %{_sysconfdir}/aodh/ is unowned
%dir {_sysconfdir}/aodh should fix it in -common subpackage.

Review granted on provision that this is fixed.


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", "*No copyright* Apache
     (v2.0)". 10 files have unknown license. Detailed output of
     licensecheck in /home/haikel/1243550-openstack-aodh/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /etc/aodh
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/systemd,
     /usr/lib/systemd/system, /etc/logrotate.d, /etc/aodh
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: 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.
[x]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 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 %license.
[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]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[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 does not use a name that already exists.
[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

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

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

Generic:
[!]: Sources can be downloaded from URI in Source: tag
     Note: Could not download Source0:
     https://pypi.python.org/packages/source/a/aodh/aodh-1.0.0.tar.gz
     See: http://fedoraproject.org/wiki/Packaging:Guidelines#Tags
[-]: 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).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
     openstack-aodh-compat , python-aodh , openstack-aodh-common ,
     openstack-aodh-api , openstack-aodh-evaluator , openstack-aodh-
     notifier , openstack-aodh-listener , openstack-aodh-expirer
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: 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]: 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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[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-aodh-compat-1.0.0-1.fc24.noarch.rpm
          python-aodh-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-common-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-api-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-evaluator-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-notifier-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-listener-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-expirer-1.0.0-1.fc24.noarch.rpm
          openstack-aodh-1.0.0-1.fc24.src.rpm
openstack-aodh-compat.noarch: W: spelling-error %description -l en_US ceilometer -> milometer, kilometer, cyclometer
openstack-aodh-compat.noarch: W: no-documentation
python-aodh.noarch: W: no-documentation
openstack-aodh-common.noarch: E: non-readable /etc/aodh/api_paste.ini 640
openstack-aodh-common.noarch: E: non-readable /etc/aodh/aodh.conf 640
openstack-aodh-common.noarch: E: non-readable /etc/aodh/policy.json 640
openstack-aodh-common.noarch: E: incoherent-logrotate-file /etc/logrotate.d/openstack-aodh
openstack-aodh-api.noarch: W: spelling-error %description -l en_US servicesfor -> services for, services-for, cervices
openstack-aodh-api.noarch: W: no-documentation
openstack-aodh-api.noarch: W: no-manual-page-for-binary aodh-dbsync
openstack-aodh-api.noarch: W: no-manual-page-for-binary aodh-api
openstack-aodh-evaluator.noarch: W: no-documentation
openstack-aodh-evaluator.noarch: W: no-manual-page-for-binary aodh-evaluator
openstack-aodh-notifier.noarch: W: no-documentation
openstack-aodh-notifier.noarch: W: no-manual-page-for-binary aodh-notifier
openstack-aodh-listener.noarch: W: no-documentation
openstack-aodh-listener.noarch: W: no-manual-page-for-binary aodh-listener
openstack-aodh-expirer.noarch: W: no-documentation
openstack-aodh-expirer.noarch: W: no-manual-page-for-binary aodh-expirer
openstack-aodh.src: W: strange-permission aodh-1.0.0.tar.gz 640
openstack-aodh.src: W: invalid-url Source0: https://pypi.python.org/packages/source/a/aodh/aodh-1.0.0.tar.gz HTTP Error 404: Not Found
9 packages and 0 specfiles checked; 4 errors, 17 warnings.




Rpmlint (installed packages)
----------------------------
openstack-aodh-evaluator.noarch: W: no-documentation
openstack-aodh-evaluator.noarch: W: no-manual-page-for-binary aodh-evaluator
openstack-aodh-common.noarch: E: non-readable /etc/aodh/aodh.conf 640
openstack-aodh-common.noarch: E: non-readable /etc/aodh/api_paste.ini 640
openstack-aodh-common.noarch: E: non-readable /etc/aodh/policy.json 640
openstack-aodh-common.noarch: E: incoherent-logrotate-file /etc/logrotate.d/openstack-aodh
openstack-aodh-listener.noarch: W: no-documentation
openstack-aodh-listener.noarch: W: no-manual-page-for-binary aodh-listener
openstack-aodh-api.noarch: W: no-documentation
openstack-aodh-api.noarch: W: no-manual-page-for-binary aodh-dbsync
openstack-aodh-api.noarch: W: no-manual-page-for-binary aodh-api
openstack-aodh-notifier.noarch: W: no-documentation
openstack-aodh-notifier.noarch: W: no-manual-page-for-binary aodh-notifier
python-aodh.noarch: W: no-documentation
openstack-aodh-expirer.noarch: W: no-documentation
openstack-aodh-expirer.noarch: W: no-manual-page-for-binary aodh-expirer
openstack-aodh-compat.noarch: W: no-documentation
8 packages and 0 specfiles checked; 4 errors, 13 warnings.



Requires
--------
openstack-aodh-evaluator (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-aodh-common

openstack-aodh-common (rpmlib, GLIBC filtered):
    /bin/sh
    config(openstack-aodh-common)
    python-aodh
    python-ceilometerclient
    python-oslo-log
    python-oslo-utils
    python-six
    shadow-utils
    systemd-units

openstack-aodh-listener (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-aodh-common

openstack-aodh-api (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-aodh-common
    python-ceilometerclient

openstack-aodh-notifier (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-aodh-common

python-aodh (rpmlib, GLIBC filtered):
    /bin/bash
    pysnmp
    python(abi)
    python-alembic
    python-ceilometerclient
    python-croniter
    python-jsonschema
    python-keystoneclient
    python-keystonemiddleware
    python-lxml
    python-migrate
    python-oslo-concurrency
    python-oslo-config
    python-oslo-context
    python-oslo-db
    python-oslo-i18n
    python-oslo-log
    python-oslo-messaging
    python-oslo-middleware
    python-oslo-policy
    python-oslo-serialization
    python-paste-deploy
    python-pbr
    python-pecan
    python-requests
    python-retrying
    python-six
    python-sqlalchemy
    python-tooz
    python-webob
    python-werkzeug
    python-wsme
    pytz

openstack-aodh-expirer (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/python2
    openstack-aodh-common

openstack-aodh-compat (rpmlib, GLIBC filtered):
    openstack-aodh-api
    openstack-aodh-common
    openstack-aodh-evaluator
    openstack-aodh-expirer
    openstack-aodh-listener
    openstack-aodh-notifier
    python-aodh



Provides
--------
openstack-aodh-evaluator:
    openstack-aodh-evaluator

openstack-aodh-common:
    config(openstack-aodh-common)
    openstack-aodh-common

openstack-aodh-listener:
    openstack-aodh-listener

openstack-aodh-api:
    openstack-aodh-api

openstack-aodh-notifier:
    openstack-aodh-notifier

python-aodh:
    python-aodh

openstack-aodh-expirer:
    openstack-aodh-expirer

openstack-aodh-compat:
    openstack-aodh-compat
    openstack-ceilometer-alarm



Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review -b 1243550 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Generic, Shell-api
Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Thanks! fixed the /etc/aodh own issue.

Spec URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh.spec
SRPM URL: https://pkilambi.fedorapeople.org/openstack-aodh/openstack-aodh-1.0.0-1.fc22.src.rpm

Imported to rdo-liberty distgit https://github.com/openstack-packages/aodh/commits/rdo-liberty

http://cbs.centos.org/koji/buildinfo?buildID=6878

In cloud7-openstack-liberty-testing