Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428383010:
git cl try: Remove --name option, stop sending "reason" property. (Closed)
|
Created:
4 years, 2 months ago by qyearsley Modified:
4 years, 1 month ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, tandrii+omg_git_cl_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl try: Remove --name option, stop sending "reason" property.
When triggering the build, there's a field called "reason",
which is currently populated by default with the user's
branch name.
I have a feeling that this is optional and could be completely
omitted.
Andrii notes that if users want to send a "reason" string,
they could send invoke git cl try with the argument -p reason="<reason>".
PSA on infra-dev:
https://groups.google.com/a/chromium.org/d/msg/infra-announce/PXkM1WinTYY/w2IvN5B4AwAJ
BUG=659552
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/5eb0519b4c3af94c1093aa81997a7ff99d39e72c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Don't add to try job properties if not specified #Patch Set 3 : Remove reason option #Messages
Total messages: 18 (8 generated)
qyearsley@chromium.org changed reviewers: + tandrii@chromium.org
https://codereview.chromium.org/2428383010/diff/1/git_cl.py File git_cl.py (left): https://codereview.chromium.org/2428383010/diff/1/git_cl.py#oldcode4771 git_cl.py:4771: # TODO(tandrii): if this even used? It's used for the "reason" string when triggering a build, but I'm not sure if *that* is used or necessary. Do you know?
https://codereview.chromium.org/2428383010/diff/1/git_cl.py File git_cl.py (left): https://codereview.chromium.org/2428383010/diff/1/git_cl.py#oldcode4771 git_cl.py:4771: # TODO(tandrii): if this even used? On 2016/10/22 19:14:29, qyearsley wrote: > It's used for the "reason" string when triggering a build, but I'm not sure if > *that* is used or necessary. Do you know? not necessary, ignored by CQ (though CQ sets it to 'cq') https://codereview.chromium.org/2428383010/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2428383010/diff/1/git_cl.py#newcode4773 git_cl.py:4773: help='Try job "reason" string; defaults to current branch name') i like this renaming, but please you remove default value completely (ie default=None) I'd also not specify in list of tryjob properties at all if it's None. https://codereview.chromium.org/2428383010/diff/1/git_cl.py#newcode4804 git_cl.py:4804: if not options.reason: that is, let's get rid of these two lines.
https://codereview.chromium.org/2428383010/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2428383010/diff/1/git_cl.py#newcode4773 git_cl.py:4773: help='Try job "reason" string; defaults to current branch name') On 2016/10/23 at 15:25:52, tandrii(chromium) wrote: > i like this renaming, but please you remove default value completely (ie default=None) > I'd also not specify in list of tryjob properties at all if it's None. Do you mean let's only add it to try job properties if it's specified? Actually, would you be OK with removing it completely and never adding "reason" to try job properties?
LGTM as is % LMK to TAL if you decided to remove it completely. https://codereview.chromium.org/2428383010/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2428383010/diff/1/git_cl.py#newcode4773 git_cl.py:4773: help='Try job "reason" string; defaults to current branch name') On 2016/10/25 17:32:13, qyearsley wrote: > On 2016/10/23 at 15:25:52, tandrii(chromium) wrote: > > i like this renaming, but please you remove default value completely (ie > default=None) > > I'd also not specify in list of tryjob properties at all if it's None. > > Do you mean let's only add it to try job properties if it's specified? yes, PS#2 is exactly what I asked for :) > Actually, would you be OK with removing it completely and never adding "reason" > to try job properties? Hm, come to think about this - we have -p|--property flag, so reason can still be specified if necessary. So, hell yeah. But if you do that, please send PSA to infra-announce@chromium.dev and chrome-infrastructure-announce@ MLs with link to this CL. And in any case, please update description of this CL.
Description was changed from ========== Change option --name to --reason and update help string. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted, but if it's not kept, then perhaps it makes more sense to rename the option. ========== to ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". ==========
On 2016/10/25 at 17:56:30, tandrii wrote: > LGTM as is % LMK to TAL if you decided to remove it completely. > > https://codereview.chromium.org/2428383010/diff/1/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/2428383010/diff/1/git_cl.py#newcode4773 > git_cl.py:4773: help='Try job "reason" string; defaults to current branch name') > On 2016/10/25 17:32:13, qyearsley wrote: > > On 2016/10/23 at 15:25:52, tandrii(chromium) wrote: > > > i like this renaming, but please you remove default value completely (ie > > default=None) > > > I'd also not specify in list of tryjob properties at all if it's None. > > > > Do you mean let's only add it to try job properties if it's specified? > > yes, PS#2 is exactly what I asked for :) > > > Actually, would you be OK with removing it completely and never adding "reason" > > to try job properties? > > Hm, come to think about this - we have -p|--property flag, so reason can still be specified if necessary. So, hell yeah. But if you do that, please send PSA to infra-announce@chromium.dev and chrome-infrastructure-announce@ MLs with link to this CL. > > And in any case, please update description of this CL. Sounds good, done :-)
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Is this also removed in CQ? Or will we still see reason=CQ then? There are a bunch of test expectations that simulate the reason property in V8. Guess it's not important, but those could be removed too I guess.
Description was changed from ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". ========== to ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". ==========
Description was changed from ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". ========== to ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". BUG=659552 ==========
Description was changed from ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". BUG=659552 ========== to ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". PSA on infra-dev: https://groups.google.com/a/chromium.org/d/msg/infra-announce/PXkM1WinTYY/w2I... BUG=659552 ==========
Filed bug http://crbug.com/659552, added CL to remove this from CQ as well https://chromereviews.googleplex.com/528237013/
And LGTM for this CL.
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". PSA on infra-dev: https://groups.google.com/a/chromium.org/d/msg/infra-announce/PXkM1WinTYY/w2I... BUG=659552 ========== to ========== git cl try: Remove --name option, stop sending "reason" property. When triggering the build, there's a field called "reason", which is currently populated by default with the user's branch name. I have a feeling that this is optional and could be completely omitted. Andrii notes that if users want to send a "reason" string, they could send invoke git cl try with the argument -p reason="<reason>". PSA on infra-dev: https://groups.google.com/a/chromium.org/d/msg/infra-announce/PXkM1WinTYY/w2I... BUG=659552 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/5eb0519b4c3af9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/5eb0519b4c3af9... |
