Page MenuHomePhabricator

Rewrite refreshCdbJsonFiles in python
ClosedPublic

Authored by thcipriani on Feb 19 2016, 10:05 PM.

Details

Maniphest Tasks
T125685: refreshCdbJsonFiles should be rewritten in python
Reviewers
dduvall
mmodell
bd808
demon
Group Reviewers
Release-Engineering-Team
Commits
rMSCAba8e257037e0: Rewrite refreshCdbJsonFiles in python
Patch without arc
git checkout -b D133 && curl -L https://phabricator.wikimedia.org/D133?download=true | git apply
Summary

Fixes T125685

Test Plan

regenerate json files and compare

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

thcipriani updated this revision to Diff 390.Feb 19 2016, 10:05 PM
thcipriani retitled this revision from to Rewrite refreshCdbJsonFiles in python.
thcipriani updated this object.
thcipriani edited the test plan for this revision. (Show Details)
thcipriani added reviewers: bd808, mmodell, demon, dduvall.
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptFeb 19 2016, 10:05 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
bd808 requested changes to this revision.Feb 19 2016, 11:02 PM
bd808 edited edge metadata.

Looks pretty good. A few things noted inline about the json file formatting.

The new entry point script should be added to docs/scap2/commands.rst as well.

scap/main.py
733

"Directory containing"

735

uppercase Number

scap/tasks.py
621

Why not just test and return here?

638

json.dumps should use indent=0 to add newlines so the files are readable by humans too. To match the hand built json from the PHP version you should set separators=(',', ':') as well.

This revision now requires changes to proceed.Feb 19 2016, 11:02 PM
thcipriani updated this revision to Diff 392.Feb 22 2016, 4:39 PM
thcipriani edited edge metadata.

Logic tweaks and doc updates

  • Spelling changes in help text
  • Add json.dumps options to make json human readable
  • Return as early as possible in refresh_cdb_json_file
  • Add refreshCdbJsonFiles to scap2 docs
bd808 accepted this revision.Feb 22 2016, 4:57 PM
bd808 edited edge metadata.

Not tested (my scap dev vm is badly broken) but the code looks good. This should be tested on beta cluster for a bit before rolling out to production just to be sure that it works as expected. My main concern would be ensuring that the N+1th run doesn't vary the the JSON output unnecessarily. I would expect that the first run following deployment will dirty all the JSON blobs due to dict ordering differences from the PHP insert order behavior.

This revision is now accepted and ready to land.Feb 22 2016, 4:57 PM
thcipriani updated this revision to Diff 440.Mar 17 2016, 5:48 PM
thcipriani edited edge metadata.
  • Maintain json_encode style compatibility
  • Remove php5-cli from required
This revision now requires review to proceed.Mar 17 2016, 5:48 PM

Build is unstable

W: scap source: binary-nmu-debian-revision-in-source 3.0.3-1+0~20160317174845.32~1.gbpad5a3c
N:
N: The version number of your source package ends in +b and a number or has
N: a Debian revision containing three parts. These version numbers are used
N: by binary NMUs and should not be used as the source version. (The +b
N: form is the current standard; the three-part version number now
N: obsolete.)
N:
N: Refer to Debian Developer's Reference section 5.10.2.1 (Recompilation or
N: binary-only NMU) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: fields, Type: binary, udeb, source
N:
W: scap source: newer-standards-version 3.9.6 (current is 3.9.5)
N:
N: The source package refers to a Standards-Version which is newer than the
N: highest one lintian is programmed to check. If the source package is
N: correct, then please upgrade lintian to the newest version. (If there is
N: no newer lintian version, then please bug li

Link to build: https://integration.wikimedia.org/ci/job/beta-build-deb/32/
See console output for more information: https://integration.wikimedia.org/ci/job/beta-build-deb/32/console

thcipriani updated this revision to Diff 442.Mar 17 2016, 10:35 PM
thcipriani edited edge metadata.
  • os.rename fails across filesystems

Build is unstable

W: scap source: binary-nmu-debian-revision-in-source 3.0.3-1+0~20160317223557.34~1.gbp86dce1
N:
N: The version number of your source package ends in +b and a number or has
N: a Debian revision containing three parts. These version numbers are used
N: by binary NMUs and should not be used as the source version. (The +b
N: form is the current standard; the three-part version number now
N: obsolete.)
N:
N: Refer to Debian Developer's Reference section 5.10.2.1 (Recompilation or
N: binary-only NMU) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: fields, Type: binary, udeb, source
N:
W: scap source: newer-standards-version 3.9.6 (current is 3.9.5)
N:
N: The source package refers to a Standards-Version which is newer than the
N: highest one lintian is programmed to check. If the source package is
N: correct, then please upgrade lintian to the newest version. (If there is
N: no newer lintian version, then please bug li

Link to build: https://integration.wikimedia.org/ci/job/beta-build-deb/34/
See console output for more information: https://integration.wikimedia.org/ci/job/beta-build-deb/34/console

bd808 accepted this revision.Mar 17 2016, 11:43 PM
bd808 edited edge metadata.
bd808 added inline comments.
scap/tasks.py
613

Nit: Python's logger magically does msg % args interpolation.

This revision is now accepted and ready to land.Mar 17 2016, 11:43 PM
demon awarded a token.Mar 18 2016, 5:15 PM
This revision was automatically updated to reflect the committed changes.