Issue 5250 - Add copyright update script

update-copyright/run_update_copyright.sh

Index: update-copyright/run_update_copyright.sh
===================================================================
new file mode 100755
--- /dev/null
+++ b/update-copyright/run_update_copyright.sh
@@ -0,0 +1,7 @@
+#!/bin/sh

[Sebastian Noack, 2017/07/05 15:46:52]

Is there any purpose of this wrapper script, other than providing defaults? Note
that you could also provide defaults using argsparse in update_copyright.py:

  arg_parser.add_argument('-u', '--hg-url',
                          default='https://hg.adblockplus.org/',
                          ...)
  arg_parser.add_argument('-p', '--push-url',
                          default='ssh://hg@hg.adblockplus.org/',
                          ...)

[Vasily Kuznetsov, 2017/07/05 17:38:05]

I suggested to have a shell script instead of defaults in the python script. The
logic of dealing with the defaults in python script is somewhat unobvious (for
example, think about what would happen if we run `update_copyright.py -u
https://hg.foo.bar/`). Also having the script try to push stuff into our
production repositories when it's run without any arguments doesn't seems like
the best idea.

The shell script separates these things, makes the script behavior simple and
obvious and prevents accidental runs targeted at production.

[Sebastian Noack, 2017/07/05 19:08:25]

Why do we support these arguments anyway? I guessed that these exist to be
overridden by the tests, but apparently the tests import the script and call
it's internal APIs directly. And given all the other eyeo-specific logic in the
script, it doesn't seem to be too useful to have just these URL configurable.

[Vasily Kuznetsov, 2017/07/05 19:24:45]

We support the arguments so that the script would be useful in other contexts,
because why not. The other eyeo-specific logic in the script is not great, but
we can remove it later. For now it doesn't interfere with other uses so no big
deal.

[Sebastian Noack, 2017/07/07 14:48:53]

The wrapper script makes the whole thing less self-contained and less convenient
to use. You need to have the shell script and the Python script in the same
directory, and then run the shell script from that directory (though this could
be improved). Not to mention, that while Python is cross-platform, by using
shell scripts we will require a POSIX environment.

However, I agree to your concern if we'd handle the defaults in the Python
script, that the behavior is inconsistent when -u is provided but not -p. In
order to address this, we could make -p mandatory if -u is given. But I also see
how this is slightly more complicated than using a wrapper script.

But then again, with all the other hard-coded logic, there doesn't seem to be
any use case for -u and -p in the first place. So why not just hard-coding these
URLs? This would be simple and consistent. And if we want to make this script
useful for non-eyeo environments (without having to change the code) in the
future, we can still make everything configurable in the same go, later.

[Vasily Kuznetsov, 2017/07/07 15:55:26]

I think we've been discussing this more than the issue deserves. While in
principle I think that it's much better to have utilities and API calls, that do
one simple thing without mixing levels of abstraction (sort of
Unix-philosophy-like approach) and we would definitely benefit from doing more
of it, especially in sitescripts repository, this particular script is probably
glue code enough that the efforts to separate it into generic and eyeo-specific
parts are misplaced.

So how about this: we remove the shell script but leave the options with eyeo
defaults and make -p mandatory if -u is provided. The options could be useful
for testing and if -p is mandatory, things should not get too confusing for the
users.

[rosie, 2017/07/07 15:55:47]

I don't have a strong opinion either way. Seems like this will only be used once
per year, so I'd go for whichever is more straight-forward and easier to use.

[Sebastian Noack, 2017/07/07 16:05:27]

Fine with me.

[rosie, 2017/07/07 16:44:00]

Done.

+
+echo 'Running python3 update_copyright.py -u https://hg.adblockplus.org/ -p ssh://hg@hg.adblockplus.org/'
+
+python3 update_copyright.py -u https://hg.adblockplus.org/ -p ssh://hg@hg.adblockplus.org/
+
+echo