Issue 29508667: Issue 4354, 4355 - handle dirty/outdated repos on release (Closed)
DescriptionIssues 4354, 4355 - handle dirty / outdated (mercurial)-repositories in releaseAutomation
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 28
Patch Set 3 : Adjusting error message, removing complexity, increasing readability #
Total comments: 8
Patch Set 4 : Raise errors on unexpected return-codes #Patch Set 5 : Move sanity-function to toplevel #
Total comments: 4
Patch Set 6 : #
Total comments: 15
Patch Set 7 : removed obsolete print #Patch Set 8 : #MessagesTotal messages: 25
Patch Set 1 Implemented checks of whether repositories are dirty / outdated https://issues.adblockplus.org/ticket/4354 https://issues.adblockplus.org/ticket/4355
https://codereview.adblockplus.org/29508667/diff/29508668/build.py File build.py (right): https://codereview.adblockplus.org/29508667/diff/29508668/build.py#newcode427 build.py:427: path = 'default' This change seems unrelated?
Patch Set 2 Removed unrelated changes (response to kzar's comment) https://codereview.adblockplus.org/29508667/diff/29508668/build.py File build.py (right): https://codereview.adblockplus.org/29508667/diff/29508668/build.py#newcode427 build.py:427: path = 'default' On 2017/08/07 14:59:59, kzar wrote: > This change seems unrelated? Acknowledged.
Hi Tristan, The general outline looks allright, but I have some comments about readability and usability (see below). Also, have you run flake8 with flake8-eyeo on this code? You seem to be aiming for Python3 compatibility, which is good in general, but might backfire as long as we don't actually test this code under Python3. Be careful with this. Cheers, Vasily https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:11: There's no need for this empty line. The imports format is normally[1]: stdlib, 3-rd party libraries, application/library specific, with the groups being separated by empty lines. My interpretation of PEP8 is that there should be no empty lines within groups, otherwise it becomes confusing. [1]: https://www.python.org/dev/peps/pep-0008/#imports https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:52: uncommitted = False You're spending quite some effort reformatting the changes that `hg status` outputs. Have you checked with the users of this script if this has value for them? It just seems to me that a message along the lines of "repo is dirty -- no release for you" would be quite sufficient and then the devs could run `hg status` themselves. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:66: print('Dirty / uncommitted changes in repository!') Using print as a function only seems to work under Python2 if you give it one argument. If someone later adds another one, it will print a tuple. The correct way to use print function would be to add a `from __future__ import print_function` as the first import of the module. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:94: print('Detected incoming changesets:') Here we're printing the incoming changesets but actually it would probably be enough to just print the list of outdated repositories. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:106: input = raw_input AFAIK, this compatibility trick doesn't work (under Python3) if you do it inside a function. Have you tried to run this code with Python3? https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:110: print('The above error/s has/have been detected within the repositories.') I know that you took this from the ticket, but I'm wondering if this line and the following one actually add value. Imagine we have some outgoing commits. What the user will see is: Detected outgoing changesets: changeset 1 changeset 2 The above error/s has/have been detected within the repositories. You might want to check whether this is ok or not. Are you sure about continuing the release-process? This is a lot of text, but it's not very informative. How about replacing the first two lines of the scary message with something like "If you proceed with the release, they will be included in the release and pushed.": Detected outgoing changesets: changeset 1 changeset 2 If you proceed with the release, they will be included in the release and pushed. Are you sure about continuing the release-process? Would not this be better? (I posted it to the ticket) https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:113: while True: What do you think about rewriting this loop, and the return statement like this: while True: choice = input('Please choose (yes / no): ').lower() if choice == 'yes': return True if choice == 'no': return False It seems a bit more clear to me. What do you think? https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:115: if choice.lower() not in ('yes', 'no'): ('yes', 'no') should be a set (this is error A102 in flake8-eyeo plugin) https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:127: repo_checks = ( I think the new code in this function is unnecessarily flexible (which is ok) and rather hard to follow as a result (this is not ok, given that we don't need the flexibility). What if we add a `sanity_check` function instead: function sanity_check(baseDir, downloadsRepo): if repo_has_incoming(): return False if repo_has_uncommitted(): return False if repo_has_outgoing(): return continue_with_outgoing() # I renamed `ask_to_continue`. and then in `run` we'll have: if not sanity_check(): print('Release aborted.') return 1 The whole thing reads almost like English and is about the same number of lines. What do you think?
Hey guys and thank you Vasily, for the review! I'll comment on your comments and ask for details for now, since there is not enough information present for a full new patch set yet. Best, Tristan https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:11: On 2017/08/08 18:10:12, Vasily Kuznetsov wrote: > There's no need for this empty line. The imports format is normally[1]: stdlib, > 3-rd party libraries, application/library specific, with the groups being > separated by empty lines. My interpretation of PEP8 is that there should be no > empty lines within groups, otherwise it becomes confusing. > > [1]: https://www.python.org/dev/peps/pep-0008/#imports Acknowledged. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:52: uncommitted = False On 2017/08/08 18:10:12, Vasily Kuznetsov wrote: > You're spending quite some effort reformatting the changes that `hg status` > outputs. Have you checked with the users of this script if this has value for > them? It just seems to me that a message along the lines of "repo is dirty -- no > release for you" would be quite sufficient and then the devs could run `hg > status` themselves. Acknowledged. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:66: print('Dirty / uncommitted changes in repository!') On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > Using print as a function only seems to work under Python2 if you give it one > argument. If someone later adds another one, it will print a tuple. The correct > way to use print function would be to add a `from __future__ import > print_function` as the first import of the module. Acknowledged. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:94: print('Detected incoming changesets:') On 2017/08/08 18:10:12, Vasily Kuznetsov wrote: > Here we're printing the incoming changesets but actually it would probably be > enough to just print the list of outdated repositories. Acknowledged. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:106: input = raw_input On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > AFAIK, this compatibility trick doesn't work (under Python3) if you do it inside > a function. Have you tried to run this code with Python3? I tested a snippet of this, but not in function-scope. You are right about it not working in a function though - maybe i should ignore the python3-compatibility for now? https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:110: print('The above error/s has/have been detected within the repositories.') On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > I know that you took this from the ticket, but I'm wondering if this line and > the following one actually add value. Imagine we have some outgoing commits. > What the user will see is: > > Detected outgoing changesets: > > changeset 1 > changeset 2 > > The above error/s has/have been detected within the repositories. > You might want to check whether this is ok or not. > Are you sure about continuing the release-process? > > This is a lot of text, but it's not very informative. How about replacing the > first two lines of the scary message with something like "If you proceed with > the release, they will be included in the release and pushed.": > > Detected outgoing changesets: > > changeset 1 > changeset 2 > > If you proceed with the release, they will be included in the release and > pushed. > Are you sure about continuing the release-process? > > Would not this be better? (I posted it to the ticket) I like it (and it was actually me, who added this message to the ticket - so it's still only me to blame ;) ) - maybe we should indeed wait for a response of those actually doing the releases. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:113: while True: On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > What do you think about rewriting this loop, and the return statement like this: > > while True: > choice = input('Please choose (yes / no): ').lower() > if choice == 'yes': > return True > if choice == 'no': > return False > > It seems a bit more clear to me. What do you think? Acknowledged. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:115: if choice.lower() not in ('yes', 'no'): On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > ('yes', 'no') should be a set (this is error A102 in flake8-eyeo plugin) This is removed while following your proposed approach above https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:127: repo_checks = ( On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > I think the new code in this function is unnecessarily flexible (which is ok) > and rather hard to follow as a result (this is not ok, given that we don't need > the flexibility). What if we add a `sanity_check` function instead: > > function sanity_check(baseDir, downloadsRepo): > if repo_has_incoming(): > return False > if repo_has_uncommitted(): > return False > if repo_has_outgoing(): > return continue_with_outgoing() # I renamed `ask_to_continue`. > > and then in `run` we'll have: > > if not sanity_check(): > print('Release aborted.') > return 1 > > The whole thing reads almost like English and is about the same number of lines. > What do you think? Acknowledged. I'll also rename the function according to your proposal
https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:106: input = raw_input On 2017/08/09 08:12:34, tlucas wrote: > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > AFAIK, this compatibility trick doesn't work (under Python3) if you do it > inside > > a function. Have you tried to run this code with Python3? > > I tested a snippet of this, but not in function-scope. You are right about it > not working in a function though - maybe i should ignore the > python3-compatibility for now? Yes, I think it would be the best to ignore Python3 compatibility in this case -- it adds complexity for kind of no good reason at the moment. As for the print function, I would say, if you add `from __future__ import print_function` to the top, I'd be cool with that. And then we will address Python3 compatibility of the built tools all together in the ticket you've just created.
Patch Set 3 * Changed to the new agreed error-message * Do not print local changes on dirty rep * remove python3-input compatibility * enhance readability / maintainability https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:11: On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:12, Vasily Kuznetsov wrote: > > There's no need for this empty line. The imports format is normally[1]: > stdlib, > > 3-rd party libraries, application/library specific, with the groups being > > separated by empty lines. My interpretation of PEP8 is that there should be no > > empty lines within groups, otherwise it becomes confusing. > > > > [1]: https://www.python.org/dev/peps/pep-0008/#imports > > Acknowledged. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:52: uncommitted = False On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:12, Vasily Kuznetsov wrote: > > You're spending quite some effort reformatting the changes that `hg status` > > outputs. Have you checked with the users of this script if this has value for > > them? It just seems to me that a message along the lines of "repo is dirty -- > no > > release for you" would be quite sufficient and then the devs could run `hg > > status` themselves. > > Acknowledged. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:66: print('Dirty / uncommitted changes in repository!') On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > Using print as a function only seems to work under Python2 if you give it one > > argument. If someone later adds another one, it will print a tuple. The > correct > > way to use print function would be to add a `from __future__ import > > print_function` as the first import of the module. > > Acknowledged. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:94: print('Detected incoming changesets:') On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:12, Vasily Kuznetsov wrote: > > Here we're printing the incoming changesets but actually it would probably be > > enough to just print the list of outdated repositories. > > Acknowledged. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:106: input = raw_input On 2017/08/09 17:09:09, Vasily Kuznetsov wrote: > On 2017/08/09 08:12:34, tlucas wrote: > > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > > AFAIK, this compatibility trick doesn't work (under Python3) if you do it > > inside > > > a function. Have you tried to run this code with Python3? > > > > I tested a snippet of this, but not in function-scope. You are right about it > > not working in a function though - maybe i should ignore the > > python3-compatibility for now? > > Yes, I think it would be the best to ignore Python3 compatibility in this case > -- it adds complexity for kind of no good reason at the moment. As for the print > function, I would say, if you add `from __future__ import print_function` to the > top, I'd be cool with that. > > And then we will address Python3 compatibility of the built tools all together > in the ticket you've just created. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:110: print('The above error/s has/have been detected within the repositories.') On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > I know that you took this from the ticket, but I'm wondering if this line and > > the following one actually add value. Imagine we have some outgoing commits. > > What the user will see is: > > > > Detected outgoing changesets: > > > > changeset 1 > > changeset 2 > > > > The above error/s has/have been detected within the repositories. > > You might want to check whether this is ok or not. > > Are you sure about continuing the release-process? > > > > This is a lot of text, but it's not very informative. How about replacing the > > first two lines of the scary message with something like "If you proceed with > > the release, they will be included in the release and pushed.": > > > > Detected outgoing changesets: > > > > changeset 1 > > changeset 2 > > > > If you proceed with the release, they will be included in the release and > > pushed. > > Are you sure about continuing the release-process? > > > > Would not this be better? (I posted it to the ticket) > > I like it (and it was actually me, who added this message to the ticket - so > it's still only me to blame ;) ) - maybe we should indeed wait for a response of > those actually doing the releases. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:113: while True: On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > What do you think about rewriting this loop, and the return statement like > this: > > > > while True: > > choice = input('Please choose (yes / no): ').lower() > > if choice == 'yes': > > return True > > if choice == 'no': > > return False > > > > It seems a bit more clear to me. What do you think? > > Acknowledged. Done. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:115: if choice.lower() not in ('yes', 'no'): On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > ('yes', 'no') should be a set (this is error A102 in flake8-eyeo plugin) > > This is removed while following your proposed approach above Done. Kept the reminder on what to specifically enter. https://codereview.adblockplus.org/29508667/diff/29508684/releaseAutomation.p... releaseAutomation.py:127: repo_checks = ( On 2017/08/09 08:12:35, tlucas wrote: > On 2017/08/08 18:10:11, Vasily Kuznetsov wrote: > > I think the new code in this function is unnecessarily flexible (which is ok) > > and rather hard to follow as a result (this is not ok, given that we don't > need > > the flexibility). What if we add a `sanity_check` function instead: > > > > function sanity_check(baseDir, downloadsRepo): > > if repo_has_incoming(): > > return False > > if repo_has_uncommitted(): > > return False > > if repo_has_outgoing(): > > return continue_with_outgoing() # I renamed `ask_to_continue`. > > > > and then in `run` we'll have: > > > > if not sanity_check(): > > print('Release aborted.') > > return 1 > > > > The whole thing reads almost like English and is about the same number of > lines. > > What do you think? > > Acknowledged. I'll also rename the function according to your proposal Done.
https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:4: from __future__ import print_function I would add a blank line below __future__ imports. https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:66: return False What is if an unexpected exit code occurs? I think we should still raise the exception in that case. try: ... except subprocess.CalledProcessError as e: if e.returncode == 1: return False raise Same below. https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:106: def can_safely_release(): Do you really need a function here? Wouldn't this be equivalent to following? can_safely_release = (repo_has_uncommitted() and repo_has_incoming() and repo_has_outgoing() and continue_with_outgoing()) if not can_safely_release: ...
Patch Set 4 * Raising Exception on unexpected return-codes from hg-calls * Commented on the "can_safely_release" concern, please see below https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:4: from __future__ import print_function On 2017/08/15 15:54:45, Sebastian Noack wrote: > I would add a blank line below __future__ imports. Done. https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:66: return False On 2017/08/15 15:54:45, Sebastian Noack wrote: > What is if an unexpected exit code occurs? I think we should still raise the > exception in that case. > > try: > ... > except subprocess.CalledProcessError as e: > if e.returncode == 1: > return False > raise > > Same below. Done. https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:106: def can_safely_release(): On 2017/08/15 15:54:45, Sebastian Noack wrote: > Do you really need a function here? Wouldn't this be equivalent to following? > > can_safely_release = (repo_has_uncommitted() and > repo_has_incoming() and > repo_has_outgoing() and > continue_with_outgoing()) > if not can_safely_release: > ... I don't, but the equivalent would be: can_safely_release = (not repo_has_uncommitted() and not repo_has_incoming() and ( not repo_has_outgoing() or continue_with_outgoing())) Imho the if-switches (and the indented following logic) make it more clear how "can_safely_release" should behave. If you don't mind, i would like to keep this.
https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:106: def can_safely_release(): On 2017/08/16 08:38:15, tlucas wrote: > On 2017/08/15 15:54:45, Sebastian Noack wrote: > > Do you really need a function here? Wouldn't this be equivalent to following? > > > > can_safely_release = (repo_has_uncommitted() and > > repo_has_incoming() and > > repo_has_outgoing() and > > continue_with_outgoing()) > > if not can_safely_release: > > ... > > I don't, but the equivalent would be: > > can_safely_release = (not repo_has_uncommitted() and > not repo_has_incoming() and ( > not repo_has_outgoing() or > continue_with_outgoing())) > > Imho the if-switches (and the indented following logic) make it more clear how > "can_safely_release" should behave. If you don't mind, i would like to keep > this. I guess, it is OK to keep the function. But then there is no reason IMO to nest this function into the run() function, as it doesn't access any nested variables. So mind moving it to the top-level?
Patch Set 5 Moved sanity-check-function to top-level https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29516572/releaseAutomation.p... releaseAutomation.py:106: def can_safely_release(): On 2017/08/16 08:48:35, Sebastian Noack wrote: > On 2017/08/16 08:38:15, tlucas wrote: > > On 2017/08/15 15:54:45, Sebastian Noack wrote: > > > Do you really need a function here? Wouldn't this be equivalent to > following? > > > > > > can_safely_release = (repo_has_uncommitted() and > > > repo_has_incoming() and > > > repo_has_outgoing() and > > > continue_with_outgoing()) > > > if not can_safely_release: > > > ... > > > > I don't, but the equivalent would be: > > > > can_safely_release = (not repo_has_uncommitted() and > > not repo_has_incoming() and ( > > not repo_has_outgoing() or > > continue_with_outgoing())) > > > > Imho the if-switches (and the indented following logic) make it more clear how > > "can_safely_release" should behave. If you don't mind, i would like to keep > > this. > > I guess, it is OK to keep the function. But then there is no reason IMO to nest > this function into the run() function, as it doesn't access any nested > variables. So mind moving it to the top-level? Agreed, Done.
https://codereview.adblockplus.org/29508667/diff/29517557/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517557/releaseAutomation.p... releaseAutomation.py:4: from __future__ import print_function There should be a blank line below the license disclaimer. https://codereview.adblockplus.org/29508667/diff/29517557/releaseAutomation.p... releaseAutomation.py:107: # run repository-checks and bail out early, in case the user does not Perhaps this comment should rather be a docstring?
Patch Set 6 Addressed Sebastian's comments on Patch Set 5 (newline / docstring) https://codereview.adblockplus.org/29508667/diff/29517557/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517557/releaseAutomation.p... releaseAutomation.py:4: from __future__ import print_function On 2017/08/16 09:16:13, Sebastian Noack wrote: > There should be a blank line below the license disclaimer. Done. https://codereview.adblockplus.org/29508667/diff/29517557/releaseAutomation.p... releaseAutomation.py:107: # run repository-checks and bail out early, in case the user does not On 2017/08/16 09:16:13, Sebastian Noack wrote: > Perhaps this comment should rather be a docstring? Done, also reduced the sentence to a minimum.
https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:104: print('Please answer "yes" or "no"!') This message seems redundant. If we don't return and jump back to the head of the loop 'Please choose (yes / no): ' is already printed, again.
Patch Set 7 Remove obsolete print https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:104: print('Please answer "yes" or "no"!') On 2017/08/16 10:33:29, Sebastian Noack wrote: > This message seems redundant. If we don't return and jump back to the head of > the loop 'Please choose (yes / no): ' is already printed, again. Right, missed that. Done.
LGTM
https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:81: incoming = True This seems inconsistent compared to repo_has_outgoing() above, I guess you want `return True` without any additional variables here as well? https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:90: """Asks the user if he wants to continue despite facing warnings""" Please formulate this in a gender-neutral way, "they want" - we might have female developers run this as well ;) https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:94: print('Are you sure about continuing the release-process?') Nit: it's "release process" https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:97: choice = raw_input('Please choose (yes / no): ').lower() Maybe strip whitespace as well?
Hey Wladimir, thank you for the review! I think we should discuss a bit more about handling the incoming / outgoing changesets, please see below. Also @ all of you, please feel free to remove yourself from the reviewers (since everyone commented so far, i don't feel comfortable doing this myself) Best, Tristan https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:81: incoming = True On 2017/08/16 10:46:09, Wladimir Palant wrote: > This seems inconsistent compared to repo_has_outgoing() above, I guess you want > `return True` without any additional variables here as well? If i returned immediately on the first occurrence of incoming changesets the user would not be notified about incoming changesets of repositories, that might be checked later. (e.g. if both the extension itself and the downloads-repository have incoming changesets). Imho that missing information would not be user-friendly and i would rather check for outgoing changesets for all involved repositories as well - what do you think about this? https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:90: """Asks the user if he wants to continue despite facing warnings""" On 2017/08/16 10:46:09, Wladimir Palant wrote: > Please formulate this in a gender-neutral way, "they want" - we might have > female developers run this as well ;) Alright - i didn't want to leave anyone out :) https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:94: print('Are you sure about continuing the release-process?') On 2017/08/16 10:46:08, Wladimir Palant wrote: > Nit: it's "release process" Acknowledged. https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:97: choice = raw_input('Please choose (yes / no): ').lower() On 2017/08/16 10:46:08, Wladimir Palant wrote: > Maybe strip whitespace as well? Acknowledged.
https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:81: incoming = True On 2017/08/16 11:05:19, tlucas wrote: > If i returned immediately on the first occurrence of incoming changesets the > user would not be notified about incoming changesets of repositories, that might > be checked later. I see, I overlooked that difference between the two functions. However, does this even work? This function requires a list of repositories as parameter, yet the caller isn't passing in any parameters.
Patch Set 8 * Re-added lost parameters to callers / callees * addressed comments from Wladimir https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.py File releaseAutomation.py (right): https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:81: incoming = True On 2017/08/16 11:11:46, Wladimir Palant wrote: > On 2017/08/16 11:05:19, tlucas wrote: > > If i returned immediately on the first occurrence of incoming changesets the > > user would not be notified about incoming changesets of repositories, that > might > > be checked later. > > I see, I overlooked that difference between the two functions. However, does > this even work? This function requires a list of repositories as parameter, yet > the caller isn't passing in any parameters. You are right, re-added the correct parameters to the callers / callees https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:90: """Asks the user if he wants to continue despite facing warnings""" On 2017/08/16 11:05:18, tlucas wrote: > On 2017/08/16 10:46:09, Wladimir Palant wrote: > > Please formulate this in a gender-neutral way, "they want" - we might have > > female developers run this as well ;) > > Alright - i didn't want to leave anyone out :) Done. https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:94: print('Are you sure about continuing the release-process?') On 2017/08/16 11:05:18, tlucas wrote: > On 2017/08/16 10:46:08, Wladimir Palant wrote: > > Nit: it's "release process" > > Acknowledged. Done. https://codereview.adblockplus.org/29508667/diff/29517565/releaseAutomation.p... releaseAutomation.py:97: choice = raw_input('Please choose (yes / no): ').lower() On 2017/08/16 11:05:18, tlucas wrote: > On 2017/08/16 10:46:08, Wladimir Palant wrote: > > Maybe strip whitespace as well? > > Acknowledged. Done.
LGTM Now is there any easy way to also make errors in the ensure_dependencies.py call cancel releases?
LGTM
On 2017/08/16 11:47:22, Wladimir Palant wrote: > LGTM > > Now is there any easy way to also make errors in the ensure_dependencies.py call > cancel releases? Without further investigation: I guess, with some slight refactoring, the ensure_dependencies.py functionality *could* be integrated into the releaseAutomation, yes - but i think this should be subject to further evaluation with Sebastian and Dave
Message was sent while issue was closed.
On 2017/08/16 12:04:49, tlucas wrote: > On 2017/08/16 11:47:22, Wladimir Palant wrote: > > LGTM > > > > Now is there any easy way to also make errors in the ensure_dependencies.py > > call cancel releases? > > Without further investigation: I guess, with some slight refactoring, the > ensure_dependencies.py functionality *could* be integrated into the > releaseAutomation, yes - but i think this should be subject to further > evaluation with Sebastian and Dave Well Wladimir is the main user of the release automation script, so no objections from me about making further improvements to it that he would find helpful. We should probably open a separate issue for that though.
Message was sent while issue was closed.
On 2017/08/17 13:12:10, kzar wrote: > We should probably open a separate issue for that though. Definitely. This was merely meant as an additional idea to improve build consistency, not really critical however - only worth doing if easy. 
|
