Page MenuHomePhabricator

Install script refreshCdbJsonFiles
AbandonedPublic

Authored by demon on Mar 23 2016, 9:04 PM.

Details

Reviewers
thcipriani
mmodell
hashar
Group Reviewers
Release-Engineering-Team
Patch without arc
git checkout -b D162 && curl -L https://phabricator.wikimedia.org/D162?download=true | git apply
Summary

We used to have setup.py to skip installing the refreshCdbJsonFiles
script since it was PHP. The script has since been ported to python.

Adjust .arclint to have refreshCdbJsonFiles linted via python linters
instead of PHP ones.

Change shebang to use /usr/bin/env

With D161, that fix the documentation building (tox -e doc).

Event Timeline

hashar updated this revision to Diff 467.Mar 23 2016, 9:04 PM
hashar retitled this revision from to Install script refreshCdbJsonFiles.
hashar updated this object.
hashar edited the test plan for this revision. (Show Details)
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptMar 23 2016, 9:04 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
hashar added inline comments.Mar 23 2016, 9:06 PM
.arclint
16

I have no clue what those pep8/pyflakes entries are for nor whether they are actually used. Linting should be done via tox -eflake8. But I digress.

bin/refreshCdbJsonFiles
1

That is needed to have tox -e doc to be able to execute refreshCdbJsonFiles --help. I don't have python2 on my machine for example, but the venv does.

demon added a subscriber: demon.Mar 23 2016, 9:28 PM
demon added inline comments.
.arclint
16

No it shouldn't, it should be done with arc lint or arc lint --everything. That's why we configure pyflakes and/or pep8 :)

demon added inline comments.Mar 23 2016, 9:33 PM
bin/refreshCdbJsonFiles
1

This was done on purpose. See D102: Use python2 interpreter explicitly.

hashar marked 2 inline comments as done.Mar 24 2016, 8:28 PM
hashar added a subscriber: thcipriani.
hashar added inline comments.
.arclint
16

Can talk about arcanist linter/test to use a venv in another task :-)

bin/refreshCdbJsonFiles
1

But reverted to use /usr/bin/env in a later patch D143. At that point refreshCdbJsonFiles was still PHP.

The rewrite to python in D133 by @thcipriani used /usr/bin/python2.

mmodell accepted this revision.Mar 25 2016, 3:35 AM
mmodell added a reviewer: mmodell.
mmodell added a subscriber: mmodell.
mmodell added inline comments.
bin/refreshCdbJsonFiles
1

This is fine, just need to add it to the patch for packaging purposes.

I don't think this is critical so I'm going to accept this so you can amend (or not) and land this.

This revision is now accepted and ready to land.Mar 25 2016, 3:35 AM
hashar updated this revision to Diff 482.Apr 1 2016, 7:14 PM
hashar edited edge metadata.
hashar marked an inline comment as done.

Make Debian quilt patch to change shebang of refreshCdbJsonFiles

This revision now requires review to proceed.Apr 1 2016, 7:14 PM
hashar added a comment.Apr 1 2016, 7:15 PM

I have managed to amend the patch! Hopefully it is properly updating debian/patches/change-shebangs.patch to apply the shebang. I inserted the diff manually though.

mmodell accepted this revision.Apr 2 2016, 1:31 AM
mmodell edited edge metadata.

lgtm

This revision is now accepted and ready to land.Apr 2 2016, 1:31 AM
thcipriani accepted this revision.Apr 4 2016, 2:54 PM
thcipriani added a reviewer: thcipriani.

@hashar thanks for the cleanup :)

hashar added a comment.Apr 5 2016, 8:04 PM

One might want to resend this and push to the staging Area so the .deb package is build and can be inspected.

thcipriani commandeered this revision.Apr 5 2016, 8:40 PM
thcipriani edited reviewers, added: hashar; removed: thcipriani.
thcipriani updated this revision to Diff 527.Apr 5 2016, 8:41 PM
thcipriani edited edge metadata.

Resubmit to staging

This revision now requires review to proceed.Apr 5 2016, 8:41 PM
thcipriani updated this revision to Diff 528.Apr 5 2016, 9:17 PM
thcipriani edited edge metadata.

Push to staging again

Got tests to run/pass https://integration.wikimedia.org/ci/job/harbormaster-test/350/ by putting my ssh key up on phabricator to push to staging (not sure why I've never had to do that before).

I have no idea about the failure to build the deb: https://integration.wikimedia.org/ci/job/beta-build-deb/53/ seems like it's unrelated to this patch @20after4 mind taking a peak.

Mainly I want to get this merged so that doc generation will work again :\

hashar commandeered this revision.Apr 6 2016, 2:59 PM
hashar edited reviewers, added: thcipriani; removed: hashar.
hashar added a subscriber: chasemp.

@mmodell the beta-build-deb job fails because it can't git clone from Phabricator over ssh:

ssh: connect to host git-ssh.wikimedia.org port 22: Network is unreachable

Afaik the 'talk to phabricator' portion here is relevant for git-ssh.wikimedia.org which is notable as it's a separate IP (and port).
a rule exists to block all ssh to public ipv4 addresses from labs things and it will need and exclusion inserted above it for this LVS ip only. Something like:

set firewall family inet filter labs-in4 term git_ssh from destination-address 208.80.154.250/32
set firewall family inet filter labs-in4 term git_ssh from destination-port ssh

So labs instance can reach git-ssh :-( Not sure why it used to work before, most probably was cloning over HTTPS per @thcipriani :D

build is now failing due to the quilt patch...

@hashar: can we just remove the patch and let dh-python2 handle this?

demon added a comment.EditedOct 12 2016, 6:42 PM

D412: Remove the refreshCdbJsonFiles exceptions from .arclint and setup.py handles the .arclint/setup.py fixes which we should've done in D133: Rewrite refreshCdbJsonFiles in python to begin with. The script is already installed (and has correct shebang). The only thing missing is the shebang patch but considering we're deprecating these toward removal I'm not sure if it's worth it :)

Abandon?

demon commandeered this revision.Oct 25 2016, 5:20 PM
demon added a reviewer: hashar.
demon abandoned this revision.Oct 25 2016, 5:20 PM

Per IRC, not needed.