Page MenuHomePhabricator

Allow passing extra arguments to rsync in sync_common()
ClosedPublic

Authored by demon on Feb 21 2018, 5:48 PM.

Details

Reviewers
mmodell
thcipriani
Group Reviewers
Release-Engineering-Team
Commits
rMSCA98f9dfef6e91: Allow passing extra arguments to rsync in sync_common()
Patch without arc
git checkout -b D981 && curl -L https://phabricator.wikimedia.org/D981?download=true | git apply
Summary

What it says on the tin. This is designed so we can sometimes pass rsync args that we usually wouldn't otherwise want.

In this case? We want scap pull to optionally support --delete-excluded...a flag we absolutely would not want to otherwise use. That's T157030 for the viewers playing along at home.

Test Plan

It should work.

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

demon created this revision.Feb 21 2018, 5:48 PM
Restricted Application added a reviewer: mmodell. · View Herald TranscriptFeb 21 2018, 5:48 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald Transcript
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
demon requested review of this revision.Feb 21 2018, 5:49 PM
demon updated this revision to Diff 2574.Feb 21 2018, 7:43 PM
  • Tidy up rsync_args logic, make it more like includes.
thcipriani added inline comments.
scap/main.py
323

I think you need to modify the _proxy_sync_command in addition to this command. Or maybe instead of...

341

should this be: rsync_args = self.rsync_args ?

demon added inline comments.Feb 21 2018, 8:00 PM
scap/main.py
323

I think in addition to is correct. There's so much duplication here 😭

341

Yeah, I realized that after I pushed. I was kinda stuck on whether I did any validation or not...

thcipriani added inline comments.Feb 21 2018, 8:33 PM
scap/main.py
608

Hrm. Maybe we could just make this flag:

@cli.argument('--delete-excluded', action='store_true', help='Pass --delete-excluded to rsync')

Then the code below becomes:

if self.arguments.delete_excluded:
    rsync_args = ['--delete-excluded']

I think the --delete-excluded seems cleaner as (a) we don't have to do any weird quoting (b) we don't have to do future work ensuring that rsync arguments are correctly parsed/split/quoted of rsync, and, most importantly (c) we don't open ourselves to any accidental rsync nightmares that may occur from erroneous use of the this flag.

This is the simplest change that could full-fill our use-case. YAGNI.

demon added inline comments.Feb 21 2018, 9:11 PM
scap/main.py
608

True. And could just drop the self.rsync_args above too -- we can always add it later if needed. For now, the changes to tasks (clearly safe) and then this minor change is best.

demon updated this revision to Diff 2578.Feb 21 2018, 9:21 PM
  • Redo overcomplication for generic rsync args as use-case-specific --delete-excluded
demon marked 6 inline comments as done.Feb 21 2018, 10:23 PM
demon retitled this revision from WIP: Allow passing extra arguments to rsync on sync common to Allow passing extra arguments to rsync in sync_common().Feb 21 2018, 10:25 PM
demon edited the summary of this revision. (Show Details)
demon edited the test plan for this revision. (Show Details)
thcipriani accepted this revision.Feb 21 2018, 10:29 PM
This revision is now accepted and ready to land.Feb 21 2018, 10:29 PM
This revision was automatically updated to reflect the committed changes.