Also, sync-file and sync-dir are not in version control for some reason.

and sync-dir has various silly assumptions about file. For one, it tries to do php-l on it, which is pointless since the line above asserted it was a directory.

And then it uses sync-common-file, which should probably be renamed to sync-common-path since it does either files or directories.

Several issues in one ticket... not good. Might be a ticket for a sprint (like Amsterdam), like one of platformeng and one of ops working on it together.

1. Adding to version control.
2. Update the scripts.

Tim's changes in I9d315c4cb9 fix linting.

(In reply to comment #3)

Tim's changes in I9d315c4cb9 fix linting.

That would be Ic40757f3f04 instead.

So, checklist then:

For each of the sync scrips, ensure:

• In version control
• Have checks for SSH_AUTH_SOCK and dsh etc. (probably best to centralise in a common function)
• Have checks for existence of directory or file
• Lint PHP
• Have consistency in parameters to dsh,mwdeploy,rsync (perhaps centralise in a common function)
• notify dologmsg
• notify deploy2graphite

I'm assigning to myself. I'd like to do this clean up once Tim's scap-to-tin branch[1] is ready and merged.

[1] https://gerrit.wikimedia.org/r/#/q/project:operations/puppet+topic:scap-to-tin,n,z

(In reply to comment #5)

So, checklist then:

For each of the sync scrips, ensure:

• In version control

https://gerrit.wikimedia.org/r/#/c/57854/

• Have checks for SSH_AUTH_SOCK and dsh etc. (probably best to centralise in a common function)
• Have checks for existence of directory or file
• Lint PHP
• Have consistency in parameters to dsh,mwdeploy,rsync (perhaps centralise in a common function)

https://gerrit.wikimedia.org/r/#/c/57890/

• notify dologmsg
• notify deploy2graphite

I'm assigning to myself. I'd like to do this clean up once Tim's scap-to-tin
branch[1] is ready and merged.

[1]
https://gerrit.wikimedia.org/r/#/q/project:operations/puppet+topic:scap-to-
tin,n,z

I think the checklist from comment 5 is pretty far along. If someone could reply to comment 5 with an updated list of what's been done, that'd be nice.

Krinkle: Are you still actively working on this/think of it as your task to take? If not, go ahead an unassign yourself.

Krinkle: Are you still actively working on this/think of it as your task to
take? If not, go ahead an unassign yourself.

Go ahead :)

(See also comment #5, comment #6)

For each of the sync scrips, ensure:

[OK] In version control
[ ] Checks SSH_AUTH_SOCK
[ ] Checks dsh
[ ] Checks existence of file or directory
[ ] Recursively lint php files
[ ] Have consistency in parameters to dsh and rsync
[ ] notify dologmsg
[ ] notify deploy2graphite

• scap, sync-common-file and sync-wikiversions check SSH_AUTH_SOCK
• scap-1skins, scap-2, sync-common-file, sync-dblist, sync-docroot and sync-wikiversions call rsync with parameters
• scap, sync-common-file, sync-dblist, sync-docroot and sync-wikiversions call dologmsg
• sync-dir and sync-file use sync-common-file internally

In I9d315c4cb937389fb8 Tim centralised a lot of information already (though not everything, it did make it a lot easier). That change introduced a similar pattern as Ori proposed in I702777b4ada8706b653, namely "/usr/local/lib/mw-deployment-vars.sh" which sets the variables.

Depending on how much variance is needed between all the various scripts, I think it should be possible to do all of the above in 1 common function that takes only a path as argument. Pretty much what sync-common-file is now (except that it is oddly named "-file", where in fact "sync-common-path" would be a better name).

Current state:
(

scap,
* loads vars
* checks SSH_AUTH_SOCK
* lint-php
* !! does not check dsh
* dsh -F30 -cM -o -oSetupTimeout=10
* > sync-common > scap-1 > scap-2
  * rsync -a --delete --exclude=svn,git --no-perms
* notifies dologmsg
* notifies deploy2graphite

scap-1skins
scap-1
scap-2
* used by scap

sync-common > scap-1

sync-common-all > scap

sync-common-file
* loads vars
* checks SSH_AUTH_SOCK
* checks existence
* checks dsh
* !! does not lint-php, but callers (sync-dir, sync-file) do
* notifies dologmsg, deploy2graphite
* dsh -cM -o -oSetupTimeout=30 -F30
* rsync -a --delete --exclude=svn,git --exclude=cache/l10n --no-perms
* notifies dologmsg
* notifies deploy2graphite

sync-dblist
* loads vars
* !! does not check SSH_AUTH_SOCK
* !! does not check dsh
* dsh -cM -o -oSetupTimeout=30 -F30
* rsync -a # doesn't pass --delete, --no-perms and --exclude
  # though --exclude does seem (harmlessly) redundant in this case
* notifies dologmsg
* !! does not notify deploy2graphite

sync-docroot
* loads vars
* !! does not check SSH_AUTH_SOCK
* !! does not check dsh
* !! does not lint-php
* dsh -cM -o -oSetupTimeout=30 -F30
* rsync -a --no-perms  # doesn't pass --delete
* notifies dologmsg
* notifies deploy2graphite

sync-dir
* loads vars
* checks existence
* lint-php
* > sync-common-file

sync-file
* loads vars
* checks existence
* lint-php
* > sync-common-file

sync-wikiversions
* loads vars
* checks SSH_AUTH_SOCK
* checks dsh
* dsh -cM -o -oSetupTimeout=30 -F30
* rsync -l 
* notifies dologmsg
* notifies deploy2graphite

)

Change 57890 merged by Tim Starling:
Set common rsync and dsh parameters in mw-deployment-vars

https://gerrit.wikimedia.org/r/57890

I think this bug has aged a lot over the past 6 months.

The good: I think much of this has been done by Bryan in the migration of scap (and sub/helper commands) from php/shell to python.

The bad: It's overly time consuming for me at this point to go through each of these checklists and see what's still needed for scap(py).

The basic checklist I believe is complete though:
[ ] In version control
[ ] Checks SSH_AUTH_SOCK
[ ] Checks dsh
[ ] Checks existence of file or directory
[ ] Recursively lint php files
[ ] Have consistency in parameters to dsh and rsync
[ ] notify dologmsg
[ ] notify deploy2graphite

Bryan: Correct me if I'm wrong on those assertions.

Unless I hear a "you're wrong" I'll close this bug, but at this point I'd ideally prefer specific actionable bug reports for specific parts of scap/our deployment tooling instead of making ever more sub-checklists here.

(In reply to Greg Grossmeier from comment #13)

Unless I hear a "you're wrong" I'll close this bug, but at this point I'd
ideally prefer specific actionable bug reports for specific parts of
scap/our deployment tooling instead of making ever more sub-checklists here.

Today some of Krinkle's analysis is dated due to the scap port to python, but since the sync-* family is still unported some of it remains valid. I'm planning to work on sync-* soon. We should revisit this once that work is done. It should be easy to close then.

I believe that this is resolved since the merge of I08ec8e987225b8e7fb1ae2119a4623383cba4db7. The only scripts left in the scap collection that have not been converted to python at this point are refreshCdbJsonFiles, restart-twemproxy and scap-recompile.