Bug 755602

Summary: Review Request: json_diff - Generates diff between two JSON files
Product: [Fedora] Fedora Reporter: Matěj Cepl <mcepl>
Component: Package ReviewAssignee: Parag AN(पराग) <panemade>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: mcepl, notting, package-review, panemade, pingou, tpelka
Target Milestone: ---Flags: panemade: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: json_diff-1.1.0-1.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-23 23:32:28 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Matěj Cepl 2011-11-21 15:14:52 UTC
Spec URL: http://mcepl.fedorapeople.org/rpms/json_diff.spec
SRPM URL: http://mcepl.fedorapeople.org/rpms/json_diff-0.9.1-1.el6.src.rpm
Description:
Compares two JSON files (http://json.org) and generates a new JSON file
with the result. Allows exclusion of some keys from the comparison, or
in other way to include only some keys.

Comment 1 Parag AN(पराग) 2011-11-21 16:20:32 UTC
Review:

+ koji scratch build (f17) ->http://koji.fedoraproject.org/koji/taskinfo?taskID=3529825

+ rpmlint on rpms gave
json_diff.src:28: W: macro-in-comment %_
json_diff.src:33: W: macro-in-comment %_
json_diff.src:33: W: macro-in-comment %{buildroot}
json_diff.noarch: W: no-manual-page-for-binary json_diff
2 packages and 0 specfiles checked; 0 errors, 4 warnings.
==> Above is not a blocker but at least macro warning can be fixed using %% instead %

- source match with upstream as (sha1sum)
  Ok I got confused here. upstream have latest today's commit with 0.9.0 tag whereas packaged tarball is 0.9.1 version

+ rest looks fine

suggestions:
1) If building this on Fedora only then you don't need
  a) buidlroot
  b) cleaning of buildroot in %install
  c) %clean section
  d) defattr(-,root,root,-)

2) Python sitelib macro not needed now in Fedora

3) install command should preserve timestamp http://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

Comment 2 Matěj Cepl 2011-11-21 18:52:25 UTC
(In reply to comment #1)

My koji build is http://koji.fedoraproject.org/koji/taskinfo?taskID=3530449

> ==> Above is not a blocker but at least macro warning can be fixed using %%
> instead %

All commented macros are now eliminated, they are actually used now.

> - source match with upstream as (sha1sum)
>   Ok I got confused here. upstream have latest today's commit with 0.9.0 tag
> whereas packaged tarball is 0.9.1 version

To make things even more confusing, it is now 0.9.2 (both in the upstream tag, at PyPI and in the package).

> suggestions:
> 1) If building this on Fedora only then you don't need
>   a) buidlroot
>   b) cleaning of buildroot in %install
>   c) %clean section
>   d) defattr(-,root,root,-)
> 
> 2) Python sitelib macro not needed now in Fedora

Package should be available even for EL-5.

> 3) install command should preserve timestamp
> http://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

Just now using only python setup.py install.

.spec file is at the same URL and .src.rpm at
http://mcepl.fedorapeople.org/rpms/json_diff-0.9.2-1.el6.src.rpm

Comment 3 Parag AN(पराग) 2011-11-22 11:36:20 UTC
+ koji build -> http://koji.fedoraproject.org/koji/taskinfo?taskID=3531690

+ rpmlint on rpms is now
json_diff.noarch: W: no-manual-page-for-binary json_diff
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

+ source verified with upstream as (shs1sum)
094324bb69e5ae45deb189efd996e30cf1bfbe7c  json_diff-0.9.2.tar.gz
094324bb69e5ae45deb189efd996e30cf1bfbe7c  ../SOURCES/json_diff-0.9.2.tar.gz


Suggestion:
1) you should write a wrapper script in /usr/bin instead of linking json_diff.py in /usr/bin/json_diff

see examples files createrepo or system-config-printer in /usr/bin

Comment 4 Matěj Cepl 2011-11-22 14:36:17 UTC
(In reply to comment #3)
> Suggestion:
> 1) you should write a wrapper script in /usr/bin instead of linking
> json_diff.py in /usr/bin/json_diff
> 
> see examples files createrepo or system-config-printer in /usr/bin

Why? And how does it break Packaging Guidelines?

Comment 5 Parag AN(पराग) 2011-11-22 15:09:14 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Suggestion:
> > 1) you should write a wrapper script in /usr/bin instead of linking
> > json_diff.py in /usr/bin/json_diff
> > 
> > see examples files createrepo or system-config-printer in /usr/bin
> 
> Why? And how does it break Packaging Guidelines?

Fine. That was just a suggestion.

APPROVED.

Comment 6 Matěj Cepl 2011-11-22 16:51:02 UTC
New Package SCM Request
=======================
Package Name: json_diff
Short Description: Generates diff between two JSON files
Owners: mcepl
Branches: f16 el6 el5
InitialCC:

Comment 7 Gwyn Ciesla 2011-11-22 17:04:16 UTC
Git done (by process-git-requests).

Comment 8 Fedora Update System 2011-11-22 19:29:21 UTC
json_diff-0.9.2-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/json_diff-0.9.2-2.fc16

Comment 9 Fedora Update System 2011-11-22 19:29:51 UTC
json_diff-0.9.2-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/json_diff-0.9.2-2.el6

Comment 10 Fedora Update System 2011-11-22 19:30:25 UTC
json_diff-0.9.2-2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/json_diff-0.9.2-2.el5

Comment 11 Fedora Update System 2011-11-23 23:32:28 UTC
json_diff-0.9.2-2.fc16 has been pushed to the Fedora 16 stable repository.

Comment 12 Fedora Update System 2011-11-29 19:36:21 UTC
json_diff-1.1.0-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/json_diff-1.1.0-1.el6

Comment 13 Fedora Update System 2011-11-29 19:42:41 UTC
json_diff-1.1.0-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/json_diff-1.1.0-1.el5

Comment 14 Fedora Update System 2011-12-16 17:42:19 UTC
json_diff-1.1.0-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Fedora Update System 2011-12-16 17:42:27 UTC
json_diff-1.1.0-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.