Skip to content
This repository has been archived by the owner on Nov 25, 2022. It is now read-only.

Synapse: Link to the contributing guide if newsfragment check fails #82

Closed
wants to merge 1 commit into from

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jul 30, 2020

There are now multiple cases of community contributors being confused about what to do when the newsfragment check fails:

When python -m towncrier.check --compare-with=origin/develop fails, it exits with error code 1, otherwise exit code 0 when the check succeeds. We use this to echo a link to the changelog contributing guide when the check fails, to hopefully allow contributors to be informed and solve the issue themselves.

This was originally a Synapse PR, until I realized that the pipeline calls a bash script instead, which does its own checks. Thus we'd want to print the guide on those failing as well.

@@ -33,7 +33,7 @@ steps:
- label: ":newspaper: Newsfile"
command:
- "python -m pip install tox"
- "scripts-dev/check-newsfragment"
- "scripts-dev/check-newsfragment || echo 'Please see the contributing guide for help: https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.md#changelog'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this will exit with a zero exitcode on failure, so the buildstep won't reject invalid newsfragments.

tbh I think you were probably right the first time: it belongs in the script, not here.

Copy link
Member Author

@anoadragon453 anoadragon453 Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script has set -e, so any non-zero exit on a command will exit the script with a nonzero exit code. And the script specifically specifies exit 1 for any errors it encounters.

Putting it in the script would require some refactoring of the script to cover all error cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting it in the script would require some refactoring of the script to cover all error cases.

Maybe? It's not actually obvious to me we want to print this out every time the script exits non-zero.

The script already has user-friendly error messages for the cases where it's the user's fault rather than something else. I suggest it makes more sense to improve those errors than it does to add more text here.

If the goal is to avoid repeating the "Please see the guide..." text, just stick that in a variable in the script so that it can be referenced in each error message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. We indeed wouldn't want Please see the contributing guide... to appear if git fetch -q origin develop fails.

I think that a link to the contributing guide being included on the tail end of the each of the errors in the script does make sense though, especially if the user has made multiple errors. They should read the guide and fix all of them instead of going through each via trial-and-error.

I'll try to modify the script instead, including detected tox's error code inside the script so the link text output is limited to that failing, rather than other random bits of the script. Thanks for bearing with me on this 🙂

@anoadragon453
Copy link
Member Author

Superseded by matrix-org/synapse#8024

@anoadragon453 anoadragon453 deleted the anoa/synapse_newsfragment_help branch August 3, 2020 20:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants