Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1398033003:
[Release] Update merge script to leverage auto-tag bot (Closed)
|
Created:
5 years, 2 months ago by Michael Hablich Modified:
4 years, 5 months ago Reviewers:
Michael Achenbach CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Release] Update merge script to leverage auto-tag bot
The auto-tag bot removes the need to handle version changes
in each merge individually. As a result this 'feature' is
removed.
BUG=v8:4408
R=machenbach@chromium.org
LOG=N
NOTRY=true
Committed: https://crrev.com/06bfc8421b6c026ee6851bbcb047fced7e7fad87
Cr-Commit-Position: refs/heads/master@{#37972}
Patch Set 1 #Patch Set 2 : Working but tests not yet #
Total comments: 16
Patch Set 3 : Reviewable version #
Total comments: 2
Patch Set 4 : Implemented feedback #Patch Set 5 : Rebased #Patch Set 6 : Fix tests and add classic script #Patch Set 7 : Added test for classic script #Patch Set 8 : Check for roll branches #
Total comments: 18
Patch Set 9 : Tune information message #Patch Set 10 : Add CQ options to enable submit via CQ #
Total comments: 2
Patch Set 11 : Adressed comments #
Total comments: 9
Patch Set 12 : Adressed another round of comments #
Total comments: 2
Patch Set 13 : Truncate title more meaningful #
Total comments: 1
Messages
Total messages: 25 (4 generated)
No need to review. I created the CL so this is stored no only on my computer. The current solution simply removes every mentioning of versions. I don't see the need for 'real' cherry-picks because gnumbd will process the commit and thus remove the connection. I think it is more important that we mention the commit hash in the description somewhere so we can find the connection again. Additionally I tweaked the issue title and text so it is more clear what happened: - If a single CL is merged the issue title is the title of the original CL with the Prefix 'Merged: '. - If multiple CLs are merged the title is 'Merged: Squashed multiple commits' -For every commit handled two lines are written to the CL description: '''Merged: <TITLE> Hash: <COMMIT_HASH>''' -TODO: I think adding additional information that it was not a clean merge or additional patch files were supplied should be added to the description -TODO: Fix UnitTests -TODO: Single commit merges don't need the additional 'Merged: <TITLE>' line in the description -TODO: Switch on uploading to Rietveld etc.
PTAL -Fixed UnitTests -Fixed search for ports so the search is case insensitive -Removed handling of versions including tagging -Reworked title and description of the merge CL so they give a better overview of the stuff done See https://codereview.chromium.org/1410023004 for an example CL.
Generally lg. Some suggestions + waiting for the tests. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:105: description = description + "Hash: " + commit_hash + "\n\n" Maybe s/Hash/Revision https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:121: commit_hash = self["full_revision_list"][0] Why not make a pure cherry-pick in case of one commit? https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:180: print "version: %s" % self["version"] Remove version output. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:242: # PrepareVersion, nit: Remove the comments. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:245: UploadStep, Maybe make the upload step configurable and let it add some text to the email explaining the new merge script. Otherwise people might get confused.
Most of my comments still apply for patch 3. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:242: # PrepareVersion, On 2015/10/21 12:32:35, Michael Achenbach wrote: > nit: Remove the comments. Done. https://codereview.chromium.org/1398033003/diff/40001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/40001/tools/release/merge_to_... tools/release/merge_to_branch.py:77: grep="^[P,p]ort %s" % revision, Does there really need to be a comma in the character class?
https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:105: description = description + "Hash: " + commit_hash + "\n\n" On 2015/10/21 12:32:35, Michael Achenbach wrote: > Maybe s/Hash/Revision You mean like Hash: 4df56dsssss r12345 ? https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:121: commit_hash = self["full_revision_list"][0] On 2015/10/21 12:32:35, Michael Achenbach wrote: > Why not make a pure cherry-pick in case of one commit? I didn't want to introduce more complexity into the script especially as a pure cherry-pick is offering no additional value. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:180: print "version: %s" % self["version"] On 2015/10/21 12:32:35, Michael Achenbach wrote: > Remove version output. Ack. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:242: # PrepareVersion, On 2015/10/21 12:32:35, Michael Achenbach wrote: > nit: Remove the comments. Done. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:245: UploadStep, On 2015/10/21 12:32:35, Michael Achenbach wrote: > Maybe make the upload step configurable and let it add some text to the email > explaining the new merge script. Otherwise people might get confused. I don't think more configuration will help. Adding a intermediate change announcement is a good idea though.
https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:105: description = description + "Hash: " + commit_hash + "\n\n" On 2015/10/21 12:41:26, Hablich wrote: > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > Maybe s/Hash/Revision > > You mean like Hash: 4df56dsssss r12345 > > ? No I mean literally. Use the more commonly used word "Revision" instead of "Hash", so that'd read, e.g.: Revision: 4df56dsssss https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:121: commit_hash = self["full_revision_list"][0] On 2015/10/21 12:41:26, Hablich wrote: > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > Why not make a pure cherry-pick in case of one commit? > > I didn't want to introduce more complexity into the script especially as a pure > cherry-pick is offering no additional value. It offers the full original commit message instead of only a title. + this seems to be the practice for chromium branches. I thought that should become the standard case and squashing would be an exception. On the other hand (have to think more): It might get harder for the v8rel script to still refer to the merged CLs. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:245: UploadStep, On 2015/10/21 12:41:26, Hablich wrote: > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > Maybe make the upload step configurable and let it add some text to the email > > explaining the new merge script. Otherwise people might get confused. > > I don't think more configuration will help. > > Adding a intermediate change announcement is a good idea though. I don't care how it would be done technically. But it would be nice if it ended up in the email so that the merger and the reviewers see what's going on.
On 2015/10/21 12:51:17, Michael Achenbach wrote: > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > File tools/release/merge_to_branch.py (right): > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > tools/release/merge_to_branch.py:105: description = description + "Hash: " + > commit_hash + "\n\n" > On 2015/10/21 12:41:26, Hablich wrote: > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > Maybe s/Hash/Revision > > > > You mean like Hash: 4df56dsssss r12345 > > > > ? > > No I mean literally. Use the more commonly used word "Revision" instead of > "Hash", so that'd read, e.g.: > Revision: 4df56dsssss > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > tools/release/merge_to_branch.py:121: commit_hash = > self["full_revision_list"][0] > On 2015/10/21 12:41:26, Hablich wrote: > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > Why not make a pure cherry-pick in case of one commit? > > > > I didn't want to introduce more complexity into the script especially as a > pure > > cherry-pick is offering no additional value. > > It offers the full original commit message instead of only a title. + this seems > to be the practice for chromium branches. I thought that should become the > standard case and squashing would be an exception. > > On the other hand (have to think more): It might get harder for the v8rel script > to still refer to the merged CLs. > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > tools/release/merge_to_branch.py:245: UploadStep, > On 2015/10/21 12:41:26, Hablich wrote: > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > Maybe make the upload step configurable and let it add some text to the > email > > > explaining the new merge script. Otherwise people might get confused. > > > > I don't think more configuration will help. > > > > Adding a intermediate change announcement is a good idea though. > > I don't care how it would be done technically. But it would be nice if it ended > up in the email so that the merger and the reviewers see what's going on. PTAL
On 2015/10/22 14:24:49, Hablich wrote: > On 2015/10/21 12:51:17, Michael Achenbach wrote: > > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > > File tools/release/merge_to_branch.py (right): > > > > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > > tools/release/merge_to_branch.py:105: description = description + "Hash: " + > > commit_hash + "\n\n" > > On 2015/10/21 12:41:26, Hablich wrote: > > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > > Maybe s/Hash/Revision > > > > > > You mean like Hash: 4df56dsssss r12345 > > > > > > ? > > > > No I mean literally. Use the more commonly used word "Revision" instead of > > "Hash", so that'd read, e.g.: > > Revision: 4df56dsssss > > > > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > > tools/release/merge_to_branch.py:121: commit_hash = > > self["full_revision_list"][0] > > On 2015/10/21 12:41:26, Hablich wrote: > > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > > Why not make a pure cherry-pick in case of one commit? > > > > > > I didn't want to introduce more complexity into the script especially as a > > pure > > > cherry-pick is offering no additional value. > > > > It offers the full original commit message instead of only a title. + this > seems > > to be the practice for chromium branches. I thought that should become the > > standard case and squashing would be an exception. > > > > On the other hand (have to think more): It might get harder for the v8rel > script > > to still refer to the merged CLs. > > > > > https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... > > tools/release/merge_to_branch.py:245: UploadStep, > > On 2015/10/21 12:41:26, Hablich wrote: > > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > > Maybe make the upload step configurable and let it add some text to the > > email > > > > explaining the new merge script. Otherwise people might get confused. > > > > > > I don't think more configuration will help. > > > > > > Adding a intermediate change announcement is a good idea though. > > > > I don't care how it would be done technically. But it would be nice if it > ended > > up in the email so that the merger and the reviewers see what's going on. > > PTAL https://codereview.chromium.org/1413453003/ this is how it looks when uploaded.
https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:105: description = description + "Hash: " + commit_hash + "\n\n" On 2015/10/21 12:51:17, Michael Achenbach wrote: > On 2015/10/21 12:41:26, Hablich wrote: > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > Maybe s/Hash/Revision > > > > You mean like Hash: 4df56dsssss r12345 > > > > ? > > No I mean literally. Use the more commonly used word "Revision" instead of > "Hash", so that'd read, e.g.: > Revision: 4df56dsssss Acknowledged. https://codereview.chromium.org/1398033003/diff/20001/tools/release/merge_to_... tools/release/merge_to_branch.py:121: commit_hash = self["full_revision_list"][0] On 2015/10/21 12:51:17, Michael Achenbach wrote: > On 2015/10/21 12:41:26, Hablich wrote: > > On 2015/10/21 12:32:35, Michael Achenbach wrote: > > > Why not make a pure cherry-pick in case of one commit? > > > > I didn't want to introduce more complexity into the script especially as a > pure > > cherry-pick is offering no additional value. > > It offers the full original commit message instead of only a title. + this seems > to be the practice for chromium branches. I thought that should become the > standard case and squashing would be an exception. > > On the other hand (have to think more): It might get harder for the v8rel script > to still refer to the merged CLs. So, I tried cherry-picks on our branches a few times. My impression was that the full commit message is only confusing and there is no mentioning what revision got cherry-picked. I think a squash is much easier to digest and does not introduce extra complexity into the script. https://codereview.chromium.org/1398033003/diff/40001/tools/release/merge_to_... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/40001/tools/release/merge_to_... tools/release/merge_to_branch.py:77: grep="^[P,p]ort %s" % revision, On 2015/10/21 12:38:37, Michael Achenbach wrote: > Does there really need to be a comma in the character class? Done.
What do you think about the idea of renaming the old file to merge_to_roll_branch.py and committing the changed one. We only need the old behavior for roll branches which are already handled by only a few selected people so it should be possible to make them aware of the old file. I don't want to spend a lot time hacking in the old behavior into the same file when I need to rip it out later one.
On 2016/06/14 14:41:48, Hablich wrote: > What do you think about the idea of renaming the old file to > merge_to_roll_branch.py and committing the changed one. We only need the old > behavior for roll branches which are already handled by only a few selected > people so it should be possible to make them aware of the old file. > > I don't want to spend a lot time hacking in the old behavior into the same file > when I need to rip it out later one. Good idea. Please add a check to either scripts to only work in one or the other branch type. Another thing would be to think about code duplication. Maybe move the common code into another common file (common to the two scripts only)?
Rubberstamping the tests... https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:101: patch_merge_desc = self.GitLog(n=1, format="%s", git_hash=commit_hash) Is this done twice? E.g. below? https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:103: description = description + "Revision: " + commit_hash + "\n\n" nit: description += ... or to avoid multiple string buffers use % like return "Merged: %s\nRevision: %s\n\n" % (patch_merge_desc, commit_hash) https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:121: title = "Merged: " + patch_merge_desc nit: inline title or also patch_merge_desc https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:125: msg_pieces.append(self._create_commit_description(commit_hash)) Don't we get a double title here? https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:166: message = ("Please note that you are using a new " Suggestion for making this readable to lazy devs: Remove first and last sentence. E.g.: NOTE: This script will no longer update include/v8-version.h and create a tag. This is done automatically by the autotag bot. See 'merge_to_branch.py --help' for more details. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:197: return ("Performs the necessary steps to merge revisions from " Suggestion: Add more details here including all relevant information about version file, auto tagging, etc. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:225: # This script only supports official release branches, not roll branches nit: Comment is redundant to print message. Rather drop it. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:227: print ("This script does not support merging to roll branches (yet). " nit: remove 'yet' to avoid embarrassment when it's still the same in two years :). https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:228: "Please use tools/release/roll_merge.py for this use case.") nit indentation - one space more...
PTAL https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:101: patch_merge_desc = self.GitLog(n=1, format="%s", git_hash=commit_hash) On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > Is this done twice? E.g. below? Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:103: description = description + "Revision: " + commit_hash + "\n\n" On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > nit: description += ... > > or to avoid multiple string buffers use % like > > return "Merged: %s\nRevision: %s\n\n" % (patch_merge_desc, commit_hash) Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:121: title = "Merged: " + patch_merge_desc On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > nit: inline title or also patch_merge_desc Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:125: msg_pieces.append(self._create_commit_description(commit_hash)) On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > Don't we get a double title here? Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:166: message = ("Please note that you are using a new " On 2016/07/21 13:56:38, Michael Achenbach (slow) wrote: > Suggestion for making this readable to lazy devs: Remove first and last > sentence. E.g.: > > NOTE: This script will no longer update include/v8-version.h and create a tag. > This is done automatically by the autotag bot. See 'merge_to_branch.py --help' > for more details. Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:197: return ("Performs the necessary steps to merge revisions from " On 2016/07/21 13:56:38, Michael Achenbach (slow) wrote: > Suggestion: Add more details here including all relevant information about > version file, auto tagging, etc. Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:225: # This script only supports official release branches, not roll branches On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > nit: Comment is redundant to print message. Rather drop it. Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:227: print ("This script does not support merging to roll branches (yet). " On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > nit: remove 'yet' to avoid embarrassment when it's still the same in two years > :). Done. https://codereview.chromium.org/1398033003/diff/140001/tools/release/merge_to... tools/release/merge_to_branch.py:228: "Please use tools/release/roll_merge.py for this use case.") On 2016/07/21 13:56:37, Michael Achenbach (slow) wrote: > nit indentation - one space more... Done.
https://codereview.chromium.org/1398033003/diff/180001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/180001/tools/release/merge_to... tools/release/merge_to_branch.py:136: msg_pieces.append("NOTRY=true\nNOPRESUBMIT=true\n") I'd add NOTREECHECKS=true as well, as otherwise merges would also wait for our closed tree. https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:103: description += "Revision: " + commit_hash + "\n\n" Is this revision to be processed by scripts or informational? If the latter, maybe include a full link with short hash, so that folks can just click on it? https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:123: self["commit_title"] = full_description[0] We might need to ensure the length of the title here and cut off if too long. Please check with tandrii for rietveld's limit. If the limit is breached, rietveld acts weirdly. https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:167: message = ("IMPORTANT: This script will no longer automatically " I like it. I'd substitute IMPORTANT with NOTE or something similar that's less important. I've the feeling that the word important creates a notion of an action that needs to be taken. https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:179: self.WaitForLGTM() How about we change the default behavior to end this script after upload? With a note to check the CQ after lgtm and that no further action is required? We could keep the old behavior behind a flag.
https://codereview.chromium.org/1398033003/diff/180001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/180001/tools/release/merge_to... tools/release/merge_to_branch.py:136: msg_pieces.append("NOTRY=true\nNOPRESUBMIT=true\n") On 2016/07/22 06:45:56, Michael Achenbach (slow) wrote: > I'd add NOTREECHECKS=true as well, as otherwise merges would also wait for our > closed tree. Done. https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:103: description += "Revision: " + commit_hash + "\n\n" On 2016/07/22 06:45:56, Michael Achenbach (slow) wrote: > Is this revision to be processed by scripts or informational? If the latter, > maybe include a full link with short hash, so that folks can just click on it? Scripts https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:123: self["commit_title"] = full_description[0] On 2016/07/22 06:45:57, Michael Achenbach (slow) wrote: > We might need to ensure the length of the title here and cut off if too long. > Please check with tandrii for rietveld's limit. If the limit is breached, > rietveld acts weirdly. The web interface gives me the ability to create a string 100 characters long. I am taking the safe route and cut off after 80 characters. https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:167: message = ("IMPORTANT: This script will no longer automatically " On 2016/07/22 06:45:56, Michael Achenbach (slow) wrote: > I like it. I'd substitute IMPORTANT with NOTE or something similar that's less > important. I've the feeling that the word important creates a notion of an > action that needs to be taken. Done. https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:179: self.WaitForLGTM() On 2016/07/22 06:45:56, Michael Achenbach (slow) wrote: > How about we change the default behavior to end this script after upload? With a > note to check the CQ after lgtm and that no further action is required? > > We could keep the old behavior behind a flag. Good idea for the next iteration. The most important thing is deprecating the versioning for now. Afterwards we can make all the other little changes incrementally.
lgtm with comments https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/200001/tools/release/merge_to... tools/release/merge_to_branch.py:123: self["commit_title"] = full_description[0] On 2016/07/22 08:55:59, Hablich wrote: > On 2016/07/22 06:45:57, Michael Achenbach (slow) wrote: > > We might need to ensure the length of the title here and cut off if too long. > > Please check with tandrii for rietveld's limit. If the limit is breached, > > rietveld acts weirdly. > > The web interface gives me the ability to create a string 100 characters long. I > am taking the safe route and cut off after 80 characters. Wondering if 80 is too aggressive. We have commit titles a lot longer. I think rietveld might allow 256 or something? https://codereview.chromium.org/1398033003/diff/220001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/220001/tools/release/merge_to... tools/release/merge_to_branch.py:123: self["commit_title"] = full_description[0][:80] nit: I'd add logic that only when cut off adds ' [...]' or similar.
The CQ bit was checked by hablich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1398033003/#ps240001 (title: "Truncate title more meaningful")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/1398033003/diff/220001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/220001/tools/release/merge_to... tools/release/merge_to_branch.py:123: self["commit_title"] = full_description[0][:80] On 2016/07/22 09:12:24, Michael Achenbach (slow) wrote: > nit: I'd add logic that only when cut off adds ' [...]' or similar. Done.
Message was sent while issue was closed.
Description was changed from ========== [Release] Update merge script to leverage auto-tag bot The auto-tag bot removes the need to handle version changes in each merge individually. As a result this 'feature' is removed. BUG=v8:4408 R=machenbach@chromium.org LOG=N NOTRY=true ========== to ========== [Release] Update merge script to leverage auto-tag bot The auto-tag bot removes the need to handle version changes in each merge individually. As a result this 'feature' is removed. BUG=v8:4408 R=machenbach@chromium.org LOG=N NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [Release] Update merge script to leverage auto-tag bot The auto-tag bot removes the need to handle version changes in each merge individually. As a result this 'feature' is removed. BUG=v8:4408 R=machenbach@chromium.org LOG=N NOTRY=true ========== to ========== [Release] Update merge script to leverage auto-tag bot The auto-tag bot removes the need to handle version changes in each merge individually. As a result this 'feature' is removed. BUG=v8:4408 R=machenbach@chromium.org LOG=N NOTRY=true Committed: https://crrev.com/06bfc8421b6c026ee6851bbcb047fced7e7fad87 Cr-Commit-Position: refs/heads/master@{#37972} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/06bfc8421b6c026ee6851bbcb047fced7e7fad87 Cr-Commit-Position: refs/heads/master@{#37972}
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1398033003/diff/240001/tools/release/merge_to... File tools/release/merge_to_branch.py (right): https://codereview.chromium.org/1398033003/diff/240001/tools/release/merge_to... tools/release/merge_to_branch.py:124: #Truncate title because of code review tool nit: space after # |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
