Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
324503002:
Add optional cloud bucket permission property to page and page_set (Closed)
|
Created:
6 years, 6 months ago by aiolos (Not reviewing) Modified:
6 years, 6 months ago CC:
chromium-reviews, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd optional cloud bucket permission property to page and page_set
The bug says to "require" that people put the permissions in their page sets,
but from talking to people it sounded like an optional parameter was better.
If this should be more restrictive, let me know.
R=dtu@chromium.org
BUG=356956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276350
Patch Set 1 #
Total comments: 6
Patch Set 2 : round two, page.py changes still included. #
Total comments: 1
Patch Set 3 : Removed page.py changes, added testing, nits #
Total comments: 5
Patch Set 4 : addressing review comments #
Total comments: 2
Patch Set 5 : fixing nits #
Total comments: 1
Patch Set 6 : last nit for Tony #
Messages
Total messages: 30 (0 generated)
Very nice. Just style nits. Dave should probably take a pass too. https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (left): https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:16: The style guide wants this to stay: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:50: raise PageSetError("Pageset privacy bucket %s is invalid" Just a nit, but we try to use ' for all strings instead of ". As you can tell from the code, this isn't always followed. So no biggie. https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:58: if page.privacy_bucket is not None: The style guide wants this to be "if page.privacy_bucket:" http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:61: % page.privacy_bucket, page.display_name) I think the last two params have to be in parens for this to work. That probably points out that we should have a unittest. Just add a case to page_unittest.py that would trigger this and use assertRaises to verify the exception. https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:62: elif (self.privacy_bucket is None or self.privacy_bucket == 'public' Since the above bails out, this should be if instead of elif. Also, a neat pythonism: if not self.privacy_bucket or self.privacy_bucket in ['public', 'google-only']: https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... tools/telemetry/telemetry/page/page_set.py:107: if (bucket_name == 'public' or bucket_name == 'google-and-partners' Maybe: if not bucket_name: return True if bucket_name in ['public', 'google-and-partners', 'google-only']: return True return False Also, I'm wondering whether we should have constants for the bucket names (or maybe we already do somewhere)? That way pages don't have to repeat these strings and possibly typo them.
On 2014/06/06 16:32:23, tonyg wrote: > Very nice. Just style nits. > > Dave should probably take a pass too. > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > File tools/telemetry/telemetry/page/page_set.py (left): > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > tools/telemetry/telemetry/page/page_set.py:16: > The style guide wants this to stay: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines Done. > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > File tools/telemetry/telemetry/page/page_set.py (right): > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > tools/telemetry/telemetry/page/page_set.py:50: raise PageSetError("Pageset > privacy bucket %s is invalid" > Just a nit, but we try to use ' for all strings instead of ". As you can tell > from the code, this isn't always followed. So no biggie. Done. Is this true for triple quotes as well? '''Is this better''' """than this?""" > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > tools/telemetry/telemetry/page/page_set.py:58: if page.privacy_bucket is not > None: > The style guide wants this to be "if page.privacy_bucket:" Done > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > tools/telemetry/telemetry/page/page_set.py:61: % page.privacy_bucket, > page.display_name) > I think the last two params have to be in parens for this to work. That probably > points out that we should have a unittest. > > Just add a case to page_unittest.py that would trigger this and use assertRaises > to verify the exception. I changed it just to be sure, and will add a test before the next review. Are there any others that should be added? > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > tools/telemetry/telemetry/page/page_set.py:62: elif (self.privacy_bucket is None > or self.privacy_bucket == 'public' > Since the above bails out, this should be if instead of elif. Done. > > Also, a neat pythonism: > > if not self.privacy_bucket or self.privacy_bucket in ['public', 'google-only']: That is neat. Done. > > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > tools/telemetry/telemetry/page/page_set.py:107: if (bucket_name == 'public' or > bucket_name == 'google-and-partners' > Maybe: > > if not bucket_name: > return True > > if bucket_name in ['public', 'google-and-partners', 'google-only']: > return True > > return False > Done > Also, I'm wondering whether we should have constants for the bucket names (or > maybe we already do somewhere)? That way pages don't have to repeat these > strings and possibly typo them. I was going based on a short chat with Dave, but may very well have mis-interpreted him. In cloud storage there are PUBLIC_BUCKET, PARTNER_BUCKET, and INTERNAL_BUCKET. Should I be importing cloud_storage and using those instead?
> https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > > tools/telemetry/telemetry/page/page_set.py:50: raise PageSetError("Pageset > > privacy bucket %s is invalid" > > Just a nit, but we try to use ' for all strings instead of ". As you can tell > > from the code, this isn't always followed. So no biggie. > > Done. Is this true for triple quotes as well? > > '''Is this better''' > > """than this?""" No. Confusingly, they should be """ > https://codereview.chromium.org/324503002/diff/1/tools/telemetry/telemetry/pa... > > tools/telemetry/telemetry/page/page_set.py:61: % page.privacy_bucket, > > page.display_name) > > I think the last two params have to be in parens for this to work. That > probably > > points out that we should have a unittest. > > > > Just add a case to page_unittest.py that would trigger this and use > assertRaises > > to verify the exception. > > I changed it just to be sure, and will add a test before the next review. Are > there any others that should be added? It's generally good to exercise all the code paths where possible. Sometimes exceptions are harder to exercise and we skip it. But tests are definitely a good thing :) There's some more info here: http://www.chromium.org/developers/telemetry/telemetry-feature-guidelines > > Also, I'm wondering whether we should have constants for the bucket names (or > > maybe we already do somewhere)? That way pages don't have to repeat these > > strings and possibly typo them. > > I was going based on a short chat with Dave, but may very well have > mis-interpreted him. > In cloud storage there are PUBLIC_BUCKET, PARTNER_BUCKET, and INTERNAL_BUCKET. > Should I be importing cloud_storage and using those instead? Yeah, importing that sgtm
I've been talking with Dave offline, and it's unclear whether having the bucket permissions on the pages is helpful enough to warrant the added complexity. This is especially true since we're trying to move away from checking every bucket when we upload/download from the could storage. I didn't want to lose these changes incase either a) we want to keep the page buckets in. or b) we might want to add them later, and having a record of the changes would be useful. What I would really like feedback on is whether to keep the page changes in, or not. I'm currently thinking about whether there is a way to have separate wpr archives for the different permission groups included in a pageset. I mostly want to avoid having to manually move older archives between the buckets, or have to keep searching all buckets for possibly older archives from before a pageset might have changed permissions. Keep in mind that a next step for these changes is a) adding bucket permissions for many of the page_sets we have and b) updating record_wpr to upload the wpr archives to the appropriate bucket(s?) automatically so we can not worry about the newer versions of the archive ending up in the wrong bucket later on.
I think this'll be ready to go after this. Should also cut down on the size of the patch. https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page.py (right): https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page.py:65: def privacy_bucket(self): Dave and I were chatting too. Let's just remove this and then talk about how to handle any cases which currently have pages in different buckets (I suspect we'll find they're all unintentional or avoidable).
On 2014/06/06 22:59:56, tonyg wrote: > I think this'll be ready to go after this. Should also cut down on the size of > the patch. Removing the page.py changes will cut down a lot of the logic, since that will also remove the AddPage logic from PageSet. > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page.py (right): > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page.py:65: def privacy_bucket(self): > Dave and I were chatting too. Let's just remove this and then talk about how to > handle any cases which currently have pages in different buckets (I suspect > we'll find they're all unintentional or avoidable). Ok. I know that the tough_video_cases.py has an intended addition of an internal-only page to an otherwise public test. I'm not sure if we can avoid it though.
On 2014/06/06 22:59:56, tonyg wrote: > I think this'll be ready to go after this. Should also cut down on the size of > the patch. Removing the page.py changes will cut down a lot of the logic, since that will also remove the AddPage logic from PageSet. > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page.py (right): > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page.py:65: def privacy_bucket(self): > Dave and I were chatting too. Let's just remove this and then talk about how to > handle any cases which currently have pages in different buckets (I suspect > we'll find they're all unintentional or avoidable). Ok. I know that the tough_video_cases.py has an intended addition of an internal-only page to an otherwise public test. I'm not sure if we can avoid it though.
On 2014/06/06 23:08:02, aiolos wrote: > On 2014/06/06 22:59:56, tonyg wrote: > > I think this'll be ready to go after this. Should also cut down on the size of > > the patch. > > Removing the page.py changes will cut down a lot of the logic, since that will > also remove the AddPage logic from PageSet. > > > > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/page/page.py (right): > > > > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/page/page.py:65: def privacy_bucket(self): > > Dave and I were chatting too. Let's just remove this and then talk about how > to > > handle any cases which currently have pages in different buckets (I suspect > > we'll find they're all unintentional or avoidable). > > Ok. I know that the tough_video_cases.py has an intended addition of an > internal-only page to an otherwise public test. I'm not sure if we can avoid it > though. We can see who owns it and ask them to split it out into a separate set. If they don't want to, we can make the whole thing internal. I don't think it makes sense to have a benchmark that only partially runs publicly. They'd just get weird results.
On 2014/06/06 23:28:47, tonyg wrote: > On 2014/06/06 23:08:02, aiolos wrote: > > On 2014/06/06 22:59:56, tonyg wrote: > > > I think this'll be ready to go after this. Should also cut down on the size > of > > > the patch. > > > > Removing the page.py changes will cut down a lot of the logic, since that will > > also remove the AddPage logic from PageSet. > > > > > > > > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > > > File tools/telemetry/telemetry/page/page.py (right): > > > > > > > > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/page/page.py:65: def privacy_bucket(self): > > > Dave and I were chatting too. Let's just remove this and then talk about how > > to > > > handle any cases which currently have pages in different buckets (I suspect > > > we'll find they're all unintentional or avoidable). > > > > Ok. I know that the tough_video_cases.py has an intended addition of an > > internal-only page to an otherwise public test. I'm not sure if we can avoid > it > > though. > > We can see who owns it and ask them to split it out into a separate set. If they > don't want to, we can make the whole thing internal. > > I don't think it makes sense to have a benchmark that only partially runs > publicly. They'd just get weird results. Agreed. That's why I was having the permission bucket on the page set change when you added a page with a higher permission requirement. But from talking with Dave, people may not know to add the permissions to the page, so it might not catch the issue any way.
On 2014/06/07 00:05:50, aiolos wrote: > On 2014/06/06 23:28:47, tonyg wrote: > > On 2014/06/06 23:08:02, aiolos wrote: > > > On 2014/06/06 22:59:56, tonyg wrote: > > > > I think this'll be ready to go after this. Should also cut down on the > size > > of > > > > the patch. > > > > > > Removing the page.py changes will cut down a lot of the logic, since that > will > > > also remove the AddPage logic from PageSet. > > > > > > > > > > > > > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > > > > File tools/telemetry/telemetry/page/page.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/324503002/diff/20001/tools/telemetry/telemetr... > > > > tools/telemetry/telemetry/page/page.py:65: def privacy_bucket(self): > > > > Dave and I were chatting too. Let's just remove this and then talk about > how > > > to > > > > handle any cases which currently have pages in different buckets (I > suspect > > > > we'll find they're all unintentional or avoidable). > > > > > > Ok. I know that the tough_video_cases.py has an intended addition of an > > > internal-only page to an otherwise public test. I'm not sure if we can avoid > > it > > > though. > > > > We can see who owns it and ask them to split it out into a separate set. If > they > > don't want to, we can make the whole thing internal. > > > > I don't think it makes sense to have a benchmark that only partially runs > > publicly. They'd just get weird results. > > Agreed. That's why I was having the permission bucket on the page set change > when you added a page with a higher permission requirement. But from talking > with Dave, people may not know to add the permissions to the page, so it might > not catch the issue any way. Makes sense.
I've taken out the page changes, added testing, and moved to using constants. PTAL
https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set.py:23: INTERNAL_BUCKET = cloud_storage.INTERNAL_BUCKET I prefer these at the module level. https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set.py:57: raise PageSetError("Pageset privacy bucket %s is invalid" ValueError for "function receives an argument that has the right type but an inappropriate value" https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set.py:141: def privacy_bucket(self): nit: Just bucket, or cloud_storage_bucket? https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_set_unittest.py (right): https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set_unittest.py:11: from telemetry.page import cloud_storage nit: alphabetize
Looks really nice now. I suspect this will be ready to land after Dave's last set of comments. https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set.py:115: def IsValidPrivacyBucket(bucket_name): If we don't expect outside callers, should we make this private (underscore prefix)?
On 2014/06/09 20:24:16, dtu wrote: > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page_set.py (right): > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_set.py:23: INTERNAL_BUCKET = > cloud_storage.INTERNAL_BUCKET > I prefer these at the module level. Done > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_set.py:57: raise PageSetError("Pageset > privacy bucket %s is invalid" > ValueError for "function receives an argument that has the right type but an > inappropriate value" Done > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_set.py:141: def privacy_bucket(self): > nit: Just bucket, or cloud_storage_bucket? I can change it to cloud_storage_bucket if you want, but didn't originally chose a name that long because I was already getting some super long lines. Moving the constants to the module level will help, but you'll still end up with lines like: cloud_storage_bucket=page_set_module.INTERNAL_BUCKET I'd prefer not to include privacy in the name if we add cloud_storage though, since that init parameter is already 52 characters long. Thoughts? > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page_set_unittest.py (right): > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_set_unittest.py:11: from telemetry.page > import cloud_storage > nit: alphabetize Done
On 2014/06/09 20:35:39, tonyg wrote: > Looks really nice now. I suspect this will be ready to land after Dave's last > set of comments. > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page_set.py (right): > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_set.py:115: def > IsValidPrivacyBucket(bucket_name): > If we don't expect outside callers, should we make this private (underscore > prefix)? Done
lgtm after two more nits are addressed. Please wait for Dave too. I'm okay w/ whatever you guys decide to name the privacy property. https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set.py:58: % privacy_bucket) nit: indentation is 2 spaces too deep. This may also fit on the previous line. https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/unittest... File tools/telemetry/unittest_data/test_page_set.py (right): https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/unittest... tools/telemetry/unittest_data/test_page_set.py:6: from telemetry.page import page as page_module I think the style guide frowns on these. If they're conflicting with other variable names, let's rename the variables.
On 2014/06/09 21:58:21, aiolos wrote: > On 2014/06/09 20:24:16, dtu wrote: > > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/page/page_set.py:141: def privacy_bucket(self): > > nit: Just bucket, or cloud_storage_bucket? > > I can change it to cloud_storage_bucket if you want, but didn't originally chose > a name that long because I was already getting some super long lines. Moving the > constants to the module level will help, but you'll still end up with lines > like: > > cloud_storage_bucket=page_set_module.INTERNAL_BUCKET > > I'd prefer not to include privacy in the name if we add cloud_storage though, > since that init parameter is already 52 characters long. > > Thoughts? Reasoning is, there's nothing in this code that's specific to it being about "privacy." Theoretically any bucket would work. So just "bucket." Also no need to rename the modules in test_page_set.py, there's no places where it reuses the names "page" or "page_set."
On 2014/06/09 22:02:38, tonyg wrote: > lgtm after two more nits are addressed. > > Please wait for Dave too. I'm okay w/ whatever you guys decide to name the > privacy property. > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page_set.py (right): > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_set.py:58: % privacy_bucket) > nit: indentation is 2 spaces too deep. This may also fit on the previous line. After changing the variable to bucket, it does fit on one line now. :) Done. > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/unittest... > File tools/telemetry/unittest_data/test_page_set.py (right): > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/unittest... > tools/telemetry/unittest_data/test_page_set.py:6: from telemetry.page import > page as page_module > I think the style guide frowns on these. If they're conflicting with other > variable names, let's rename the variables. Done. Most of the page sets in perf/page_sets import as page_module or page_set_module. Do I need to fix that as part of https://codereview.chromium.org/321863003/ ?
On 2014/06/09 22:03:22, dtu wrote: > On 2014/06/09 21:58:21, aiolos wrote: > > On 2014/06/09 20:24:16, dtu wrote: > > > > > > https://codereview.chromium.org/324503002/diff/40001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/page/page_set.py:141: def privacy_bucket(self): > > > nit: Just bucket, or cloud_storage_bucket? > > > > I can change it to cloud_storage_bucket if you want, but didn't originally > chose > > a name that long because I was already getting some super long lines. Moving > the > > constants to the module level will help, but you'll still end up with lines > > like: > > > > cloud_storage_bucket=page_set_module.INTERNAL_BUCKET > > > > I'd prefer not to include privacy in the name if we add cloud_storage though, > > since that init parameter is already 52 characters long. > > > > Thoughts? > > Reasoning is, there's nothing in this code that's specific to it being about > "privacy." Theoretically any bucket would work. So just "bucket." Ah, gotcha. Changed to bucket. > > Also no need to rename the modules in test_page_set.py, there's no places where > it reuses the names "page" or "page_set." Done.
lgtm
On 2014/06/09 22:50:07, aiolos wrote: > On 2014/06/09 22:02:38, tonyg wrote: > > lgtm after two more nits are addressed. > > > > Please wait for Dave too. I'm okay w/ whatever you guys decide to name the > > privacy property. > > > > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/page/page_set.py (right): > > > > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/page/page_set.py:58: % privacy_bucket) > > nit: indentation is 2 spaces too deep. This may also fit on the previous line. > > After changing the variable to bucket, it does fit on one line now. :) Done. > > > > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/unittest... > > File tools/telemetry/unittest_data/test_page_set.py (right): > > > > > https://codereview.chromium.org/324503002/diff/60001/tools/telemetry/unittest... > > tools/telemetry/unittest_data/test_page_set.py:6: from telemetry.page import > > page as page_module > > I think the style guide frowns on these. If they're conflicting with other > > variable names, let's rename the variables. > > Done. Most of the page sets in perf/page_sets import as page_module or > page_set_module. Do I need to fix that as part of > https://codereview.chromium.org/321863003/ ? Whatever's convenient.
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiolos@chromium.org/324503002/80001
lgtm https://codereview.chromium.org/324503002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_set.py (right): https://codereview.chromium.org/324503002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_set.py:12: from telemetry.page import cloud_storage alphabetization nit
The CQ bit was unchecked by aiolos@chromium.org
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aiolos@chromium.org/324503002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 276350 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
