[Telemetry] Add PRESUBMIT check to prevent .sha file uploaded without archive file uploaded

[Telemetry] Add PRESUBMIT check to prevent .sha file uploaded without archive file uploaded

BUG=461379
TEST=Add a foo.wpr.sha1 file in tools/perf/page_set/data/ with random content, commit it, then run "git cl upload presubmit".

Committed: https://crrev.com/417ee8e828edf67ba172eb560ac96055fb4320da
Cr-Commit-Position: refs/heads/master@{#317955}

[nednguyen, 2015-02-24 18:11:09]

Patchset #1 (id:1) has been deleted

[nednguyen, 2015-02-24 18:11:46]

nednguyen@google.com changed reviewers:
+ aiolos@chromium.org, dtu@chromium.org, slamm@chromium.org

[aiolos (Not reviewing), 2015-02-24 18:19:45]

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:52: results.append(output_api.PresubmitError(
Should this be a warning instead of an error? People outside of Google don't
have permission to access all of the buckets, and will need to record their own
wpr files to run the page_sets in Internal, and sometimes Partner. They
definitely shouldn't be uploading or committing those recordings.

[nednguyen, 2015-02-24 18:24:40]

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:52: results.append(output_api.PresubmitError(
On 2015/02/24 18:19:45, aiolos wrote:
> Should this be a warning instead of an error? People outside of Google don't
> have permission to access all of the buckets, and will need to record their
own
> wpr files to run the page_sets in Internal, and sometimes Partner. They
> definitely shouldn't be uploading or committing those recordings.

If they don't commit or upload, then this PRESUBMIT check will not run. It
doesn't run on all .wpr.sha1 file, only the one that included in your commit.
And it only runs when you do "git cl upload" or "git cl presubmit".

[aiolos (Not reviewing), 2015-02-24 18:53:07]

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:14: sys.path = [os.path.join(os.pardir, 'telemetry')] +
sys.path
PRESUBMIT scripts aren't supposed to have side effects, so this should go back
in the try block format that is was in earlier.

[dtu, 2015-02-24 19:12:47]

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:45: wpr_file_exist_on_cloud = False
nit: wpr_file_exists_in_cloud

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:52: results.append(output_api.PresubmitError(
On 2015/02/24 18:24:40, nednguyen wrote:
> On 2015/02/24 18:19:45, aiolos wrote:
> > Should this be a warning instead of an error? People outside of Google don't
> > have permission to access all of the buckets, and will need to record their
> own
> > wpr files to run the page_sets in Internal, and sometimes Partner. They
> > definitely shouldn't be uploading or committing those recordings.
> 
> If they don't commit or upload, then this PRESUBMIT check will not run. It
> doesn't run on all .wpr.sha1 file, only the one that included in your commit.
> And it only runs when you do "git cl upload" or "git cl presubmit". 

I think this is fine. I guess you have to be careful that the buckets are
ordered from most open to most restrictive.

https://codereview.chromium.org/954753002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:25: SUPPORTED_BUCKETS =
(PUBLIC_BUCKET, PARTNER_BUCKET, INTERNAL_BUCKET)
Avoid redundancy: use BUCKET_ALIASES.itervalues(). If you need to retain the
order, use collections.OrderedDict

[slamm, 2015-02-24 19:22:59]

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:39: """ Check whether the wpr sha files have matching
URLs """
No spaces immediately inside triple quotes.

"""Check whether the wpr sha files have matching URLs."""

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:45: wpr_file_exist_on_cloud = False
How about the following?

is_wpr_file_uploaded

or 

is_wpr_file_in_cloud

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:49: cloud_storage.Exists(bucket, expected_hash))
is_wpr_file_uploaded = any(cloud_storage.Exists(bucket, expected_hash)
                           for bucket in cloud_storage.SUPPORTED_BUCKETS))

https://codereview.chromium.org/954753002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:25: SUPPORTED_BUCKETS =
(PUBLIC_BUCKET, PARTNER_BUCKET, INTERNAL_BUCKET)
On 2015/02/24 19:12:47, dtu wrote:
> Avoid redundancy: use BUCKET_ALIASES.itervalues(). If you need to retain the
> order, use collections.OrderedDict

itervalues gives an undefined order.

[nednguyen, 2015-02-24 19:28:34]

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:14: sys.path = [os.path.join(os.pardir, 'telemetry')] +
sys.path
On 2015/02/24 18:53:07, aiolos wrote:
> PRESUBMIT scripts aren't supposed to have side effects, so this should go back
> in the try block format that is was in earlier.

Done.

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:39: """ Check whether the wpr sha files have matching
URLs """
On 2015/02/24 19:22:59, slamm wrote:
> No spaces immediately inside triple quotes.
> 
> """Check whether the wpr sha files have matching URLs."""

Done.

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:45: wpr_file_exist_on_cloud = False
On 2015/02/24 19:12:47, dtu wrote:
> nit: wpr_file_exists_in_cloud

Done.

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:45: wpr_file_exist_on_cloud = False
On 2015/02/24 19:22:59, slamm wrote:
> How about the following?
> 
> is_wpr_file_uploaded
> 
> or 
> 
> is_wpr_file_in_cloud

Done.

https://codereview.chromium.org/954753002/diff/20001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:49: cloud_storage.Exists(bucket, expected_hash))
On 2015/02/24 19:22:59, slamm wrote:
> is_wpr_file_uploaded = any(cloud_storage.Exists(bucket, expected_hash)
>                            for bucket in cloud_storage.SUPPORTED_BUCKETS))

Nice, I like this.

https://codereview.chromium.org/954753002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:25: SUPPORTED_BUCKETS =
(PUBLIC_BUCKET, PARTNER_BUCKET, INTERNAL_BUCKET)
On 2015/02/24 19:12:47, dtu wrote:
> Avoid redundancy: use BUCKET_ALIASES.itervalues(). If you need to retain the
> order, use collections.OrderedDict

Done.

[nednguyen, 2015-02-24 19:30:16]

Patchset #2 (id:40001) has been deleted

[nednguyen, 2015-02-24 19:34:02]

Patchset #2 (id:60001) has been deleted

[aiolos (Not reviewing), 2015-02-24 19:37:50]

https://codereview.chromium.org/954753002/diff/80001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/80001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:43: old_sys_path = sys.path
The nested sys.path changes are unneeded. Either this one should be removed, or
the one in _CommonChecks should be moved to _CheckJson.

[dtu, 2015-02-24 19:39:49]

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:31: BUCKET_ALIASES['internal'] =
INTERNAL_BUCKET
collections.OrderedDict((
  ('public', PUBLIC_BUCKET),
  ('partner', PARTNER_BUCKET),
  ('internal', INTERNAL_BUCKET),
))

[slamm, 2015-02-24 20:51:19]

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:31: BUCKET_ALIASES['internal'] =
INTERNAL_BUCKET
On 2015/02/24 19:39:49, dtu wrote:
> collections.OrderedDict((
>   ('public', PUBLIC_BUCKET),
>   ('partner', PARTNER_BUCKET),
>   ('internal', INTERNAL_BUCKET),
> ))

Also,

collections.OrderedDict(
  public=PUBLIC_BUCKET,
  partner=PARTNER_BUCKET,
  internal=INTERNAL_BUCKET)

https://codereview.chromium.org/954753002/diff/40002/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/40002/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:48: is_wpr_file_uploaded = False
Not needed.

[nednguyen, 2015-02-24 20:57:46]

https://codereview.chromium.org/954753002/diff/80001/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/80001/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:43: old_sys_path = sys.path
On 2015/02/24 19:37:49, aiolos wrote:
> The nested sys.path changes are unneeded. Either this one should be removed,
or
> the one in _CommonChecks should be moved to _CheckJson.

Err, I didn't look at this carefully and thought that _CommonChecks is in
parallel with _CheckWprShaFiles :-)

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:31: BUCKET_ALIASES['internal'] =
INTERNAL_BUCKET
On 2015/02/24 19:39:49, dtu wrote:
> collections.OrderedDict((
>   ('public', PUBLIC_BUCKET),
>   ('partner', PARTNER_BUCKET),
>   ('internal', INTERNAL_BUCKET),
> ))

Done.

https://codereview.chromium.org/954753002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/util/cloud_storage.py:31: BUCKET_ALIASES['internal'] =
INTERNAL_BUCKET
On 2015/02/24 20:51:19, slamm wrote:
> On 2015/02/24 19:39:49, dtu wrote:
> > collections.OrderedDict((
> >   ('public', PUBLIC_BUCKET),
> >   ('partner', PARTNER_BUCKET),
> >   ('internal', INTERNAL_BUCKET),
> > ))
> 
> Also,
> 
> collections.OrderedDict(
>   public=PUBLIC_BUCKET,
>   partner=PARTNER_BUCKET,
>   internal=INTERNAL_BUCKET)

Cool. But I prefer Dave's syntax.

https://codereview.chromium.org/954753002/diff/40002/tools/perf/PRESUBMIT.py
File tools/perf/PRESUBMIT.py (right):

https://codereview.chromium.org/954753002/diff/40002/tools/perf/PRESUBMIT.py#...
tools/perf/PRESUBMIT.py:48: is_wpr_file_uploaded = False
On 2015/02/24 20:51:19, slamm wrote:
> Not needed.

Done.

[slamm, 2015-02-24 20:59:27]

LGTM (though I am ignorant of the ways of the PRESUBMIT)

[aiolos (Not reviewing), 2015-02-24 21:40:01]

On 2015/02/24 20:59:27, slamm wrote:
> LGTM (though I am ignorant of the ways of the PRESUBMIT)

also lgtm

[nednguyen, 2015-02-24 21:59:38]

On 2015/02/24 21:40:01, aiolos wrote:
> On 2015/02/24 20:59:27, slamm wrote:
> > LGTM (though I am ignorant of the ways of the PRESUBMIT)
> 
> also lgtm

Ping Dave since Steve and Kari don't have owner yet.

[aiolos (Not reviewing), 2015-02-24 22:00:51]

On 2015/02/24 21:59:38, nednguyen wrote:
> On 2015/02/24 21:40:01, aiolos wrote:
> > On 2015/02/24 20:59:27, slamm wrote:
> > > LGTM (though I am ignorant of the ways of the PRESUBMIT)
> > 
> > also lgtm
> 
> Ping Dave since Steve and Kari don't have owner yet.

I'm pretty sure that's fine since you do.

[aiolos (Not reviewing), 2015-02-24 22:01:19]

On 2015/02/24 22:00:51, aiolos wrote:
> On 2015/02/24 21:59:38, nednguyen wrote:
> > On 2015/02/24 21:40:01, aiolos wrote:
> > > On 2015/02/24 20:59:27, slamm wrote:
> > > > LGTM (though I am ignorant of the ways of the PRESUBMIT)
> > > 
> > > also lgtm
> > 
> > Ping Dave since Steve and Kari don't have owner yet.
> 
> I'm pretty sure that's fine since you do.

One way to find out... ;)

[aiolos (Not reviewing), 2015-02-24 22:02:40]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-02-24 22:03:10]

The CQ bit was unchecked by aiolos@chromium.org

[dtu, 2015-02-25 00:30:28]

https://codereview.chromium.org/954753002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/cloud_storage.py:38: BUCKET_ALIASES['internal'] =
INTERNAL_BUCKET
now this is redundant

[nednguyen, 2015-02-25 00:33:01]

https://codereview.chromium.org/954753002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/cloud_storage.py (right):

https://codereview.chromium.org/954753002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/cloud_storage.py:38: BUCKET_ALIASES['internal'] =
INTERNAL_BUCKET
On 2015/02/25 00:30:28, dtu wrote:
> now this is redundant

Oopps, silly me!

[dtu, 2015-02-25 00:39:44]

lgtm

[nednguyen, 2015-02-25 01:31:33]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-02-25 01:31:34]

The patchset sent to the CQ was uploaded after l-g-t-m from aiolos@chromium.org,
slamm@chromium.org
Link to the patchset: https://codereview.chromium.org/954753002/#ps120001
(title: "Fix redundant BUCKET_ALIASES assignment")

[commit-bot: I haz the power, 2015-02-25 01:32:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/954753002/120001

[commit-bot: I haz the power, 2015-02-25 02:34:34]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-02-25 02:35:21]

Patchset 5 (id:??) landed as
https://crrev.com/417ee8e828edf67ba172eb560ac96055fb4320da
Cr-Commit-Position: refs/heads/master@{#317955}