[WPT Export] Require GitHub API token in WPT remote origin URL

[WPT Export] Require GitHub API token in WPT remote origin URL

Also:
- refactor where WPTGitHub is instantiated to allow gh_token to be passed to LocalWPT

R=qyearsley@chromium.org
BUG=693651
Review-Url: https://codereview.chromium.org/2719173002
Cr-Commit-Position: refs/heads/master@{#453683}
Committed: https://chromium.googlesource.com/chromium/src/+/5165a30947f7e933c1cc3b5d902b7bbae1873976

[jeffcarp, 2017-02-28 18:20:44]

On 2017/02/28 at 18:20:19, jeffcarp wrote:
> 

Oops, realized I didn't actually publish this yesterday.

[qyearsley, 2017-02-28 18:30:05]

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:19: def
__init__(self, host, gh_token, path='/tmp/wpt'):
LocalWPT is used in test_importer, which I believe doesn't need a token -- so
here, gh_token should perhaps be a named optional argument with a default value
of None?

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:40:
self.host.executive.run_command(['git', 'clone', '--depth=10', remote_url,
self.path])
Why depth = 10? What if the test that we want to upstream is nested really
deeply, like 11 or 12 directories deep?

[jeffcarp, 2017-02-28 18:53:38]

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:19: def
__init__(self, host, gh_token, path='/tmp/wpt'):
On 2017/02/28 at 18:30:04, qyearsley wrote:
> LocalWPT is used in test_importer, which I believe doesn't need a token -- so
here, gh_token should perhaps be a named optional argument with a default value
of None?

sgtm

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:40:
self.host.executive.run_command(['git', 'clone', '--depth=10', remote_url,
self.path])
On 2017/02/28 at 18:30:04, qyearsley wrote:
> Why depth = 10? What if the test that we want to upstream is nested really
deeply, like 11 or 12 directories deep?

--depth in this case refers to how many commits back we want to clone (e.g. this
only clones the repository and its most recent 10 commits), but your comment
made me realize that this won't work for a similar reason, because the commit
containing Cr-Commit-Position might be more than 10 commits back.

I added --depth originally to lighten the load since we're cloning the GitHub
repo from scratch every 5-10 minutes or so (since the bot gets torn down every
time). I'll remove that flag and think of other ways to lighten the load.

[jeffcarp, 2017-02-28 18:57:57]

On 2017/02/28 at 18:53:38, jeffcarp wrote:
>
https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):
> 
>
https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:19: def
__init__(self, host, gh_token, path='/tmp/wpt'):
> On 2017/02/28 at 18:30:04, qyearsley wrote:
> > LocalWPT is used in test_importer, which I believe doesn't need a token --
so here, gh_token should perhaps be a named optional argument with a default
value of None?
> 
> sgtm
> 
>
https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:40:
self.host.executive.run_command(['git', 'clone', '--depth=10', remote_url,
self.path])
> On 2017/02/28 at 18:30:04, qyearsley wrote:
> > Why depth = 10? What if the test that we want to upstream is nested really
deeply, like 11 or 12 directories deep?
> 
> --depth in this case refers to how many commits back we want to clone (e.g.
this only clones the repository and its most recent 10 commits), but your
comment made me realize that this won't work for a similar reason, because the
commit containing Cr-Commit-Position might be more than 10 commits back.
> 
> I added --depth originally to lighten the load since we're cloning the GitHub
repo from scratch every 5-10 minutes or so (since the bot gets torn down every
time). I'll remove that flag and think of other ways to lighten the load.

K I updated the CL based on the feedback.

[qyearsley, 2017-02-28 18:58:04]

LGTM after comments are addressed :-)

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):

https://codereview.chromium.org/2719173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:40:
self.host.executive.run_command(['git', 'clone', '--depth=10', remote_url,
self.path])
On 2017/02/28 at 18:53:38, jeffcarp wrote:
> On 2017/02/28 at 18:30:04, qyearsley wrote:
> > Why depth = 10? What if the test that we want to upstream is nested really
deeply, like 11 or 12 directories deep?
> 
> --depth in this case refers to how many commits back we want to clone (e.g.
this only clones the repository and its most recent 10 commits), but your
comment made me realize that this won't work for a similar reason, because the
commit containing Cr-Commit-Position might be more than 10 commits back.
> 
> I added --depth originally to lighten the load since we're cloning the GitHub
repo from scratch every 5-10 minutes or so (since the bot gets torn down every
time). I'll remove that flag and think of other ways to lighten the load.

Ah, I see, that's what depth means :-)

Maybe another depth might be reasonable, as an optimization? Like, there should
be no need to go back further than a few months of commits, probably?

[qyearsley, 2017-02-28 18:58:34]

LGTM

[jeffcarp, 2017-02-28 19:05:26]

The CQ bit was checked by jeffcarp@chromium.org

[jeffcarp, 2017-02-28 19:05:35]

Description was changed from

==========
[WPT Export] Require GitHub API token in WPT remote origin URL

Also:
- refactor where WPTGitHub is instantiated to allow gh_token to be passed to
LocalWPT
- only shallow clone WPT with --depth=10

R=qyearsley@chromium.org
BUG=693651
==========

to

==========
[WPT Export] Require GitHub API token in WPT remote origin URL

Also:
- refactor where WPTGitHub is instantiated to allow gh_token to be passed to
LocalWPT

R=qyearsley@chromium.org
BUG=693651
==========

[commit-bot: I haz the power, 2017-02-28 19:06:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 20:13:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488308726578740,
"parent_rev": "b3fe808c00e4716ff92a9629c8dea953689ba78e", "commit_rev":
"5165a30947f7e933c1cc3b5d902b7bbae1873976"}

[commit-bot: I haz the power, 2017-02-28 20:14:23]

Description was changed from

==========
[WPT Export] Require GitHub API token in WPT remote origin URL

Also:
- refactor where WPTGitHub is instantiated to allow gh_token to be passed to
LocalWPT

R=qyearsley@chromium.org
BUG=693651
==========

to

==========
[WPT Export] Require GitHub API token in WPT remote origin URL

Also:
- refactor where WPTGitHub is instantiated to allow gh_token to be passed to
LocalWPT

R=qyearsley@chromium.org
BUG=693651

Review-Url: https://codereview.chromium.org/2719173002
Cr-Commit-Position: refs/heads/master@{#453683}
Committed:
https://chromium.googlesource.com/chromium/src/+/5165a30947f7e933c1cc3b5d902b...
==========

[commit-bot: I haz the power, 2017-02-28 20:14:24]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5165a30947f7e933c1cc3b5d902b...