Run git-lfs-install earlier, before cloning
ClosedPublic

Authored by mmodell on May 2 2018, 9:35 PM.

Details

Reviewers
awight
thcipriani
Group Reviewers
Release-Engineering-Team
Commits
rMSCAa2c3add38e34: Run git-lfs-install earlier, before cloning
Patch without arc
git checkout -b D1040 && curl -L https://phabricator.wikimedia.org/D1040?download=true | git apply
Summary

Since we don't run git-lfs-pull in submodules, we need to make sure that
git lfs install happens even earlier in the process.

Test Plan

untested, unfortunately

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.
mmodell created this revision.May 2 2018, 9:35 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptMay 2 2018, 9:35 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
mmodell requested review of this revision.May 2 2018, 9:37 PM
demon added a subscriber: demon.May 2 2018, 10:01 PM
demon added inline comments.
scap/deploy.py
319

s/is/==/?

thcipriani added inline comments.May 2 2018, 10:03 PM
scap/deploy.py
316

I think this else is unnecessary since there is a return above. Should this be an elif not os.path.isdir(rev_dir)?

319

I think this line should be: if git_binary_manager and 'git-lfs' in git_binary_manager

Long explanation:

There's a chance git_binary_manager is None so in that instance this will throw TypeError: argument of type 'NoneType' is not iterable. So we should check that first.

Also, I don't think the or condition will ever be true. git_binary_manager will either be None, or a list containing strings. Those strings won't reference the same object as git.LFS so you should use == instead of is:

>>> x = 'git-fat'
>>> x is 'git-fat'
False

but you shouldn't need the == comparison at all since git_binary_manager should always be a list or None.

mmodell updated this revision to Diff 2738.May 2 2018, 10:04 PM

s/is/==/

mmodell updated this revision to Diff 2739.May 2 2018, 10:06 PM

check for none, get rid of == check

mmodell updated this revision to Diff 2740.May 2 2018, 10:09 PM

if not os.path.isdir(rev_dir):

mmodell marked 3 inline comments as done.May 2 2018, 10:09 PM
mmodell added inline comments.May 2 2018, 10:11 PM
scap/deploy.py
317

just let this condition stand on it's own, it's unrelated to the previous if condition anyway and the previous if does a return so the else doesn't serve much purpose.

320

this is now checked for None

thcipriani added inline comments.May 2 2018, 10:17 PM
scap/git.py
135

I don't think this will work in the default case or the case where args are passed.

[] + something only concatenates if something is another list.

If you provide args args is a tuple. If you don't provide args, args is a string. Both with throw type errors when +'d with a list.

mmodell updated this revision to Diff 2741.May 2 2018, 10:30 PM

convert to lists instead of tuples

mmodell marked an inline comment as done.May 2 2018, 10:30 PM
thcipriani accepted this revision.May 2 2018, 10:51 PM
This revision is now accepted and ready to land.May 2 2018, 10:51 PM
This revision was automatically updated to reflect the committed changes.