action: Document errors properly #5
Conversation
The action currently fails with very little explanation as to why it does so. Add help text that describes the causes of failure for most of the common cases. Signed-off-by: Carles Cufi <[email protected]>
carlescufi
force-pushed
the
add-error-messages
branch
from
065a9e2
to
867a297
Compare
| die(f'Unable to cherry-pick commit {usha}. This means that the upstream ' | ||
| f'commit does not apply cleanly into the NCS fork. This can happen ' | ||
| f'if you modified an upstream commit in order to resolve conflicts, ' | ||
| f'but this is now allowed. Instead, revert any [nrf noup] commits that ' |
There was a problem hiding this comment.
| f'but this is now allowed. Instead, revert any [nrf noup] commits that ' | |
| f'but this is not allowed. Instead, revert any [nrf noup] commits that ' |
?
| f'but this is now allowed. Instead, revert any [nrf noup] commits that ' | ||
| f'may be causing the conflict and cherry-pick any additional prior ' | ||
| f'commits from upstream that may be needed in order to avoid a merge ' | ||
| f'conflict') |
There was a problem hiding this comment.
| f'conflict') | |
| f'conflict. Then you can re-apply the reverted [nrf noup] commits.') |
action.py
Outdated
| revert = m.group(1) | ||
| tag = m.group(2) | ||
| if not tag: | ||
| die(f'{sha}: Title does not contain a sauce tag') | ||
| die(f'{sha}: Title does not contain a sauce tag ' | ||
| f'(mergeup|fromtree|fromlist|noup)') |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'd add:
| f'(mergeup|fromtree|fromlist|noup)') | |
| f'([mergeup] |[fromtree] |[fromlist] |[noup]) ') |
There was a problem hiding this comment.
If [ ] is used, then nrf should be added. And it should be the same in L217 and L222.
|
|
||
| if revert: | ||
| if tag == 'mergeup': | ||
| die('Mergeup cannot be reverted') | ||
| die('Mergeup commits cannot be reverted') |
There was a problem hiding this comment.
I know reverting merge commits can be tricky if not on the HEAD.
Should we explicitly ban them instead of just omitting it from
https://nordicsemi.atlassian.net/wiki/spaces/NCS/pages/108201225/Pull+Requests#revert
There was a problem hiding this comment.
In reality merge commits are only ever handled by Vestavind in upmerges, because they are only ever allowed as part of an upmerge. So for the "general public" this has no effect whatsoever.
If [ ] is used, then nrf should be added. And it should be the same in L217 and L222. Co-authored-by: Kristoffer Rist Skøien <[email protected]>
| @@ -244,7 +248,7 @@ def check_commit(urepo, ubranch, target, sha, merge): | |||
|
|
|||
| match = re.search(regex, body, re.MULTILINE) | |||
| if not match: | |||
| die(f'{sha}: fromlist commit missing an Upstream PR reference') | |||
| die(f'{sha}: fromlist commit missing an "Upstream PR #:" reference') | |||
There was a problem hiding this comment.
| die(f'{sha}: fromlist commit missing an "Upstream PR #:" reference') | |
| die(f'{sha}: fromlist commit message missing an "Upstream PR #:" reference' | |
| f'See [doc](https://nordicsemi.atlassian.net/wiki/spaces/NCS/pages/108201225/Pull+Requests#fromlist-ref) for explanation.') | |
There was a problem hiding this comment.
The reference to the Development Model exists already in the die() function implementation. I will add the other change.
There was a problem hiding this comment.
that is a very long document. i added anchors for relevant sections.
'fromlist-ref'
'revert'
|
|
||
| if revert: | ||
| if tag == 'mergeup': | ||
| die('Mergeup cannot be reverted') | ||
| die('Mergeup commits cannot be reverted') |
There was a problem hiding this comment.
| die('Mergeup commits cannot be reverted') | |
| die('Mergeup commits cannot be reverted. See [documentation](https://nordicsemi.atlassian.net/wiki/spaces/NCS/pages/108201225/Pull+Requests#revert).') |
The action currently fails with very little explanation as to why it does so. Add help text that describes the causes of failure for most of the common cases.