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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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
  • 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 ?

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...

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.

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.

  • Redo overcomplication for generic rsync args as use-case-specific --delete-excluded
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)
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.