Open
Bug 1345817
Opened 8 years ago
Updated 7 years ago
Tree closure hghook gives confusing error message if "CLOSED TREE" used when treestatus down
Categories
(Developer Services :: Mercurial: hg.mozilla.org, defect)
Developer Services
Mercurial: hg.mozilla.org
Tracking
(Not tracked)
NEW
People
(Reporter: emorley, Unassigned)
Details
Attachments
(1 file)
Split out of bug 1345584.
If treestatus is working and reports that a tree is closed, users can add "CLOSED TREE" to the commit message to override. In this case, the user is shown:
```
remote: FOO is CLOSED! Reason: BAR
remote: But you included the magic words. Hope you had permission!
```
Similarly, in the "tree status isn't working so fail closed by default" case, the user can also add "CLOSED TREE" to override. However in this case they get:
```
remote: ************************** ERROR ****************************
remote: Error accessing https://api.pub.build.mozilla.org/treestatus/trees/comm-esr52-seamonkey :
remote: HTTP Error 404: NOT FOUND
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
```
...even though though the push succeeded.
The error message here:
https://hg.mozilla.org/hgcustom/version-control-tools/file/06154c69e174/hghooks/mozhghooks/treeclosure.py#l56
...needs splitting into two, like occurs here:
https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hghooks/mozhghooks/treeclosure.py#l40
Comment hidden (mozreview-request) |
Assignee: nobody → wkocher
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)
https://reviewboard.mozilla.org/r/183448/#review188694
I'm not asking you to write tests for this because it is probably a bit of work, but do we not have test coverage for this case?
::: hghooks/mozhghooks/treeclosure.py:63
(Diff revision 1)
> - "To push regardless, include \"%s\" in your push comment." % (url, err, magicwords))
> if repo.changectx('tip').description().find(magicwords) == -1:
> + printError("%sTo push regardless, include \"%s\" in your push comment." % (error_text, magicwords))
> return False
> +
> + print "%sBut you included the magic words. Hope you had permission!" % error_text
Shouldn't this be `printError()`?
Attachment #8912065 -
Flags: review?(gps) → review-
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)
https://reviewboard.mozilla.org/r/183448/#review189042
::: hghooks/mozhghooks/treeclosure.py:63
(Diff revision 1)
> - "To push regardless, include \"%s\" in your push comment." % (url, err, magicwords))
> if repo.changectx('tip').description().find(magicwords) == -1:
> + printError("%sTo push regardless, include \"%s\" in your push comment." % (error_text, magicwords))
> return False
> +
> + print "%sBut you included the magic words. Hope you had permission!" % error_text
Not if I'm copying what happened in the non-exception case up around line 46.
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)
https://reviewboard.mozilla.org/r/183448/#review188694
Yes there are and they're now failing. I'll fix them up tomorrow.
Comment hidden (mozreview-request) |
Okay, fixed up the syntax error that was causing the tests to fail. With that fixed, the tests all pass with no changes to them, so I guess this case is totally untested?
Comment 7•7 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #6)
> Okay, fixed up the syntax error that was causing the tests to fail. With
> that fixed, the tests all pass with no changes to them, so I guess this case
> is totally untested?
It is probably totally untested. Not surprising.
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)
https://reviewboard.mozilla.org/r/183448/#review189184
Attachment #8912065 -
Flags: review?(gps) → review+
Updated•7 years ago
|
Assignee: kwierso → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•