tryserver recipe_module: Add get_tags.

tryserver recipe_module: Add get_tags.

Lets you get CL tags for a given CL.

BUG=591172
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300658

[martiniss, 2016-04-26 00:45:39]

martiniss@chromium.org changed reviewers:
+ iannucci@chromium.org, tandrii@chromium.org

[martiniss, 2016-04-26 00:45:40]

PTAL

It's currently broken, because of gerrit. Right now it just fails; I figured I
could do that until I got feedback from tandrii about how gerrit would get the
patch url, but looks like I can't

I tried looking at the tryserver module, but I don't know which properties I
should use to construct something like
https://chrome-internal-review.googlesource.com/#/c/252679/ :(

Also, general feedback would be great. I'm adding it to the tryserver module,
but not sure if that's the best place to put it.

[martiniss, 2016-04-26 00:46:20]

Description was changed from

==========
tryserver recipe_module: Add get_tags.

Lets you get CL tags for a given CL.

BUG=
==========

to

==========
tryserver recipe_module: Add get_tags.

Lets you get CL tags for a given CL.

BUG=591172
==========

[tandrii(chromium), 2016-04-26 10:29:59]

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:250: def get_tags(self, patch_text=None):
get_tags is confusing because 1) tag has different meaning git 2) there is
already subproject_tag here, which seems unrelated.

I think get_footers is better.

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:256: can set BUG=123123 to tell bugdroid to
update a bug when you close this CL.
iannucci@ gave us awesome footers support in depot_tools (git_footers.py). IMO,
we should be transitioning to proper git footers  instead of k=v stuff. That
said, k=v is reality, so this code deserves to exist. Still, I'd probably prefer
a tool inside depot_tools, maybe in that git_footers.py that would extra all
footers.

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:263: raise recipe_api.StepFailure("Cannot get
tags from gerrit yet.")
tbh, i don't understand why this doens't work for gerrit. git cl desc works.

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:271: patch_text = self.m.step('Get description',
cmd).stdout
i'd add separate "get_cl_description" api with just this step.

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ex...
File recipe_modules/tryserver/example.expected/with_git_patch.json (left):

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ex...
recipe_modules/tryserver/example.expected/with_git_patch.json:72: "cmd": [
where did this go?

[iannucci, 2016-04-26 23:50:56]

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:256: can set BUG=123123 to tell bugdroid to
update a bug when you close this CL.
On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> iannucci@ gave us awesome footers support in depot_tools (git_footers.py).
IMO,
> we should be transitioning to proper git footers  instead of k=v stuff. That
> said, k=v is reality, so this code deserves to exist. Still, I'd probably
prefer
> a tool inside depot_tools, maybe in that git_footers.py that would extra all
> footers.

Yes, if we can use the footers format, that would be best. The K=V format is
pretty ad hoc and gets messed up by various processes.

That said, CQ currently does crap like:
  
  Review URL: https://codereview.chromium.org/1897903002

In order to use footers-style, we need to change CQ to use footers style and not
break existing footers on the review message first.

The reason that footers are better than k=v is that gerrit (and some of our
tools, like gnumbd) have code to natively understand/manipulate them. It would
be conceivable to add indexing support for them e.g. in gerrit, whereas it would
never be feasible to add support for them for k=v (since there's basically no
format restrictions for k=v stuff, so it's not possible to reliably pluck them
out of the commit message).

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:271: patch_text = self.m.step('Get description',
cmd).stdout
On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> i'd add separate "get_cl_description" api with just this step.

git_cl.description, but yes +1

[tandrii(chromium), 2016-04-27 12:36:50]

On 2016/04/26 23:50:56, iannucci wrote:
>
https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
> File recipe_modules/tryserver/api.py (right):
> 
>
https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
> recipe_modules/tryserver/api.py:256: can set BUG=123123 to tell bugdroid to
> update a bug when you close this CL.
> On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > iannucci@ gave us awesome footers support in depot_tools (git_footers.py).
> IMO,
> > we should be transitioning to proper git footers  instead of k=v stuff. That
> > said, k=v is reality, so this code deserves to exist. Still, I'd probably
> prefer
> > a tool inside depot_tools, maybe in that git_footers.py that would extra all
> > footers.
> 
> Yes, if we can use the footers format, that would be best. The K=V format is
> pretty ad hoc and gets messed up by various processes.
> 
> That said, CQ currently does crap like:
>   
>   Review URL: https://codereview.chromium.org/1897903002
> 
> In order to use footers-style, we need to change CQ to use footers style and
not
> break existing footers on the review message first.
> 
> The reason that footers are better than k=v is that gerrit (and some of our
> tools, like gnumbd) have code to natively understand/manipulate them. It would
> be conceivable to add indexing support for them e.g. in gerrit, whereas it
would
> never be feasible to add support for them for k=v (since there's basically no
> format restrictions for k=v stuff, so it's not possible to reliably pluck them
> out of the commit message).
> 
>
https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
> recipe_modules/tryserver/api.py:271: patch_text = self.m.step('Get
description',
> cmd).stdout
> On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > i'd add separate "get_cl_description" api with just this step.
> 
> git_cl.description, but yes +1
Hey, yeah! +100 for a good recipe module.

[tandrii(chromium), 2016-04-27 12:37:06]

On 2016/04/27 12:36:50, tandrii(chromium) wrote:
> On 2016/04/26 23:50:56, iannucci wrote:
> >
>
https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
> > File recipe_modules/tryserver/api.py (right):
> > 
> >
>
https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
> > recipe_modules/tryserver/api.py:256: can set BUG=123123 to tell bugdroid to
> > update a bug when you close this CL.
> > On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > > iannucci@ gave us awesome footers support in depot_tools (git_footers.py).
> > IMO,
> > > we should be transitioning to proper git footers  instead of k=v stuff.
That
> > > said, k=v is reality, so this code deserves to exist. Still, I'd probably
> > prefer
> > > a tool inside depot_tools, maybe in that git_footers.py that would extra
all
> > > footers.
> > 
> > Yes, if we can use the footers format, that would be best. The K=V format is
> > pretty ad hoc and gets messed up by various processes.
> > 
> > That said, CQ currently does crap like:
> >   
> >   Review URL: https://codereview.chromium.org/1897903002
> > 
> > In order to use footers-style, we need to change CQ to use footers style and
> not
> > break existing footers on the review message first.
> > 
> > The reason that footers are better than k=v is that gerrit (and some of our
> > tools, like gnumbd) have code to natively understand/manipulate them. It
would
> > be conceivable to add indexing support for them e.g. in gerrit, whereas it
> would
> > never be feasible to add support for them for k=v (since there's basically
no
> > format restrictions for k=v stuff, so it's not possible to reliably pluck
them
> > out of the commit message).
> > 
> >
>
https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
> > recipe_modules/tryserver/api.py:271: patch_text = self.m.step('Get
> description',
> > cmd).stdout
> > On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > > i'd add separate "get_cl_description" api with just this step.
> > 
> > git_cl.description, but yes +1
> Hey, yeah! +100 for a good recipe module.

In fact, I might actually start this today.

[tandrii(chromium), 2016-04-27 13:06:13]

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:256: can set BUG=123123 to tell bugdroid to
update a bug when you close this CL.
On 2016/04/26 23:50:56, iannucci wrote:
> On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > iannucci@ gave us awesome footers support in depot_tools (git_footers.py).
> IMO,
> > we should be transitioning to proper git footers  instead of k=v stuff. That
> > said, k=v is reality, so this code deserves to exist. Still, I'd probably
> prefer
> > a tool inside depot_tools, maybe in that git_footers.py that would extra all
> > footers.
> 
> Yes, if we can use the footers format, that would be best. The K=V format is
> pretty ad hoc and gets messed up by various processes.
> 
> That said, CQ currently does crap like:
>   
>   Review URL: https://codereview.chromium.org/1897903002
Fix: https://chromereviews.googleplex.com/412857013

> In order to use footers-style, we need to change CQ to use footers style and
not
> break existing footers on the review message first.
> 
> The reason that footers are better than k=v is that gerrit (and some of our
> tools, like gnumbd) have code to natively understand/manipulate them. It would
> be conceivable to add indexing support for them e.g. in gerrit, whereas it
would
> never be feasible to add support for them for k=v (since there's basically no
> format restrictions for k=v stuff, so it's not possible to reliably pluck them
> out of the commit message).

[martiniss, 2016-04-29 21:34:40]

Ok, changed a bunch of stuff. PTAL :)

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:250: def get_tags(self, patch_text=None):
On 2016/04/26 at 10:29:58, tandrii(chromium) wrote:
> get_tags is confusing because 1) tag has different meaning git 2) there is
already subproject_tag here, which seems unrelated.
> 
> I think get_footers is better.

good point, changed.

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:256: can set BUG=123123 to tell bugdroid to
update a bug when you close this CL.
On 2016/04/26 at 23:50:56, iannucci wrote:
> On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > iannucci@ gave us awesome footers support in depot_tools (git_footers.py).
IMO,
> > we should be transitioning to proper git footers  instead of k=v stuff. That
> > said, k=v is reality, so this code deserves to exist. Still, I'd probably
prefer
> > a tool inside depot_tools, maybe in that git_footers.py that would extra all
> > footers.
> 
> Yes, if we can use the footers format, that would be best. The K=V format is
pretty ad hoc and gets messed up by various processes.
> 
> That said, CQ currently does crap like:
>   
>   Review URL: https://codereview.chromium.org/1897903002
> 
> In order to use footers-style, we need to change CQ to use footers style and
not break existing footers on the review message first.
> 
> The reason that footers are better than k=v is that gerrit (and some of our
tools, like gnumbd) have code to natively understand/manipulate them. It would
be conceivable to add indexing support for them e.g. in gerrit, whereas it would
never be feasible to add support for them for k=v (since there's basically no
format restrictions for k=v stuff, so it's not possible to reliably pluck them
out of the commit message).

Ok, i'm switching to this style. Updating code appropriately.

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:263: raise recipe_api.StepFailure("Cannot get
tags from gerrit yet.")
On 2016/04/26 at 10:29:58, tandrii(chromium) wrote:
> tbh, i don't understand why this doens't work for gerrit. git cl desc works.

It does, but I don't know how to generate the URL that I need to pass it. For
rietveld, we have that information, but I don't understand which properties to
look at.

I tried looking at clank for inspiration, and they only care about
event.patchSet.ref, and the url, so I can't use that I think.

Tandrii, do you know the magic incantation of properties I should use?

https://codereview.chromium.org/1915833003/diff/1/recipe_modules/tryserver/ap...
recipe_modules/tryserver/api.py:271: patch_text = self.m.step('Get description',
cmd).stdout
On 2016/04/26 at 23:50:56, iannucci wrote:
> On 2016/04/26 10:29:58, tandrii(chromium) wrote:
> > i'd add separate "get_cl_description" api with just this step.
> 
> git_cl.description, but yes +1

Done.

[iannucci, 2016-05-12 00:25:52]

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/depot_to...
File recipe_modules/depot_tools/example.expected/basic.json (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/depot_to...
recipe_modules/depot_tools/example.expected/basic.json:10:
"Path('RECIPE_PACKAGE_REPO[depot_tools]', 'cros')"
hm... did this change? I'd expect for these to be rendered like
`RECIPE_PACKAGE_REPO[depot_tools]/cros`?

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/a...
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/a...
recipe_modules/git_cl/api.py:20: assert patch and codereview, "Both patch and
codereview must be provided"
why make them optional and defaulted to None then?

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/e...
File recipe_modules/git_cl/example.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/e...
recipe_modules/git_cl/example.py:23: api.git_cl.set_config("basic")
I thought we liked single quotes...

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/api.py:255: These are  used to annotate a CL with some
machine readable data.
This description is not accurate. Footers are extracted from the last paragraph
of the text, as long as every line in that last paragraph has the form of
`^\w+(-(\w+))*:\s+.*$`. If any line in the last paragraph does not have this
format, the last paragraph is ignored and the text is considered to have no
footers.

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/api.py:279: return tags
wait... why not use `git-footers` for this? If necessary, git footers should be
modified to accept text on stdin and write parsed footers to a json file. Don't
re-invent this parser.

[martiniss, 2016-05-13 19:23:58]

Ok, updated to use git-footers, PTAL

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/depot_to...
File recipe_modules/depot_tools/example.expected/basic.json (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/depot_to...
recipe_modules/depot_tools/example.expected/basic.json:10:
"Path('RECIPE_PACKAGE_REPO[depot_tools]', 'cros')"
On 2016/05/12 at 00:25:52, iannucci wrote:
> hm... did this change? I'd expect for these to be rendered like
`RECIPE_PACKAGE_REPO[depot_tools]/cros`?

This was wrong, I fixed; see new patch for comments though.

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/a...
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/a...
recipe_modules/git_cl/api.py:20: assert patch and codereview, "Both patch and
codereview must be provided"
On 2016/05/12 at 00:25:52, iannucci wrote:
> why make them optional and defaulted to None then?

If one of them is provided, the other must be provided, but if neither are, then
it's fine... Should I do something different? Maybe update the assert?

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/e...
File recipe_modules/git_cl/example.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/e...
recipe_modules/git_cl/example.py:23: api.git_cl.set_config("basic")
On 2016/05/12 at 00:25:52, iannucci wrote:
> I thought we liked single quotes...

We do! Fixed.

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/api.py:255: These are  used to annotate a CL with some
machine readable data.
On 2016/05/12 at 00:25:52, iannucci wrote:
> This description is not accurate. Footers are extracted from the last
paragraph of the text, as long as every line in that last paragraph has the form
of `^\w+(-(\w+))*:\s+.*$`. If any line in the last paragraph does not have this
format, the last paragraph is ignored and the text is considered to have no
footers.

Ok, changed. I just moved to use git-footers, and said to look at those docs if
you want more info.

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/api.py:279: return tags
On 2016/05/12 at 00:25:52, iannucci wrote:
> wait... why not use `git-footers` for this? If necessary, git footers should
be modified to accept text on stdin and write parsed footers to a json file.
Don't re-invent this parser.

Done.

https://codereview.chromium.org/1915833003/diff/80001/recipe_modules/depot_to...
File recipe_modules/depot_tools/example.py (right):

https://codereview.chromium.org/1915833003/diff/80001/recipe_modules/depot_to...
recipe_modules/depot_tools/example.py:8: 'recipe_engine/path',
So it turns out I need to import this for this test to pass. This is because of
https://github.com/luci/recipes-py/blob/master/recipe_modules/path/api.py#L135 ;
if the module isn't initialized, then using a path fails, since it doesn't have
a tostring method........

https://codereview.chromium.org/1915833003/diff/80001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://codereview.chromium.org/1915833003/diff/80001/tests/gclient_test.py#n...
tests/gclient_test.py:713: sorted([
flaky test D:

[martiniss, 2016-05-19 22:31:24]

ping

[iannucci, 2016-05-23 19:52:33]

lgtm though I'm not sure about the change to git_footers?

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/a...
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/1915833003/diff/40001/recipe_modules/git_cl/a...
recipe_modules/git_cl/api.py:20: assert patch and codereview, "Both patch and
codereview must be provided"
On 2016/05/13 19:23:56, martiniss wrote:
> On 2016/05/12 at 00:25:52, iannucci wrote:
> > why make them optional and defaulted to None then?
> 
> If one of them is provided, the other must be provided, but if neither are,
then
> it's fine... Should I do something different? Maybe update the assert?

Acknowledged.

https://codereview.chromium.org/1915833003/diff/100001/git_footers.py
File git_footers.py (right):

https://codereview.chromium.org/1915833003/diff/100001/git_footers.py#newcode185
git_footers.py:185: message = '\n'.join(l for l in sys.stdin)
isn't this just sys.stdin.read().strip('\n')?

https://codereview.chromium.org/1915833003/diff/100001/git_footers.py#newcode206
git_footers.py:206: print '%s: %s' % (k, v)
this change is not orthographic. Previously this would render

  {"footer": [1, 2, 3]}

As 
  footer: 1
  footer: 2
  footer: 3

Now it will render it as

  footer: [1, 2, 3]

Why did you make this change?

https://codereview.chromium.org/1915833003/diff/100001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://codereview.chromium.org/1915833003/diff/100001/tests/gclient_test.py#...
tests/gclient_test.py:713: sorted([
not really a need to do this at runtime, since it's a static list :)

[martiniss, 2016-05-23 21:30:02]

Fixed. You were correct about git footers.

https://codereview.chromium.org/1915833003/diff/100001/git_footers.py
File git_footers.py (right):

https://codereview.chromium.org/1915833003/diff/100001/git_footers.py#newcode185
git_footers.py:185: message = '\n'.join(l for l in sys.stdin)
On 2016/05/23 at 19:52:33, iannucci wrote:
> isn't this just sys.stdin.read().strip('\n')?

You can't mock out sys.stdin.read with StringIO, so you have to do this :( it's
annoying, but makes it work for tests.

https://codereview.chromium.org/1915833003/diff/100001/git_footers.py#newcode206
git_footers.py:206: print '%s: %s' % (k, v)
On 2016/05/23 at 19:52:33, iannucci wrote:
> this change is not orthographic. Previously this would render
> 
>   {"footer": [1, 2, 3]}
> 
> As 
>   footer: 1
>   footer: 2
>   footer: 3
> 
> Now it will render it as
> 
>   footer: [1, 2, 3]
> 
> Why did you make this change?

Ah whoops, I thought I was optimizing ;) changed back.

https://codereview.chromium.org/1915833003/diff/100001/tests/gclient_test.py
File tests/gclient_test.py (right):

https://codereview.chromium.org/1915833003/diff/100001/tests/gclient_test.py#...
tests/gclient_test.py:713: sorted([
On 2016/05/23 at 19:52:33, iannucci wrote:
> not really a need to do this at runtime, since it's a static list :)

Ah true.

[martiniss, 2016-05-23 21:30:04]

The CQ bit was checked by martiniss@chromium.org

[martiniss, 2016-05-23 21:30:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1915833003/#ps120001
(title: "Minor things.")

[commit-bot: I haz the power, 2016-05-23 21:30:11]

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

[commit-bot: I haz the power, 2016-05-23 21:33:29]

Description was changed from

==========
tryserver recipe_module: Add get_tags.

Lets you get CL tags for a given CL.

BUG=591172
==========

to

==========
tryserver recipe_module: Add get_tags.

Lets you get CL tags for a given CL.

BUG=591172

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300658
==========

[commit-bot: I haz the power, 2016-05-23 21:33:30]

Committed patchset #7 (id:120001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=300658