Add workflow to guarantee changelog is updated #4949
Conversation
|
My hesitation here is that not every change may warrant a changelog note. If I update a docstring does that belong in the changelog? Docs feel even less vital to note. Do we need to document documentation? Imagine a typo fix. Do we note that? |
|
@RollingStar I can remove the documentation watch part, so it only triggers on a code change. Also, this is meant to be a flag; we can always ignore it when merging. But it'll be a failing test so that the maintainers can see that and think, 'Does this need a changelog update or is it fine?' |
|
Odds are a changelog should be left. As long as maintainers can merge without updating the changelog when needed, it's fine. Would it go in here? |
|
Yes, it would. Maintainers can bypass checks if we want to merge so it should be fine. It would just be a failure in that section that makes you think about it. |
|
In my humble opinion, I don't think this workflow is necessary at all. Usually we don't merge PR's when a changelog is missing/required. I don't feel that it's forgotten often or at all since I'm working with beets. Contributors usually get notified by us reviewers that they should add the changelog and then we are ready for the merge. Also: We do have the checkboxes in our PR template that tell us maintainers to remind the contributor to supply a changelog :-) |
|
If you don't think that it's needed, then I can close the PR. This is just a backup; getting the repository to move more of the maintaining work and focus required onto automation. We obviously have a bit of a slow process in getting PRs merged and making releases, so putting as much of the work involved on the computer is, I feel, a good goal we could aim for. In short: code hard, brain hurty, computer good. |
|
Automation, checklists, and procedures are better than remembering. It will help the project scale and make reviewing easier for more reviewers. If we merge this, we should update the submit template to say something like ✅ Updated changelog (big changes must be noted in the changelog after they are approved but before they are merged) Revisions welcome. I want to let people know that we generally want a changelog, but it doesn't need to be completed right from the start. It feels like features and changes go through some revisions, so making the changelog off the bat can waste time. |
|
I think making that change to the PR template would be helpful. And I think that making as much of the possible work automated will help the repository in the long term. Things like linters and the formatting tools, as well as the actions I'm writing, will help keep the standards of the PRs and the repository high without reviewing these PRs being as much of a chore. |
|
I surely agree that automating as much as possible is good, but I don't see the value of this right now since there still is manual intervention required for disabling the workflow in some cases where we don't want a changelog. And I never had a problem with people forgetting changelogs or checking the box but not doing it or something like that. Usually that is, at least in my experience, not a problem we have. I agree that removing it for "docs changes" is the right thing to do. And also, I probably might have to see this in action to grasp its real value, so please go ahead and merge it :-) Before we merge it: I agree that the template for PRs could be improved. I have some suggestions for a couple of things in there already for a long time. Where does that template live? Can we include that change within this PR? Or is that somewhere else (excuse my lack of GitHub knowledge)? |
Looks like it goes in .github: |
|
@JOJ0 what do you mean? There's no manual intervention required at all. It's a check that passes, or it doesn't. You notice it and then decide to ignore it or not. There's nothing you need to disable. |
|
Ah ok then I have to ignore a red. Ok yeah got it... |
|
Is that a problem for you? |
No, well, not sure, but please let's go ahead with it and let's give it a try. Then I'll really see :-) I'll try to draft some new PR template Markdown today. Should we include that right into this PR? I think it would fit in here. |
My first take on a PR template overhaul. I think I went a little over the top - please both of you help me getting this down in a more concise way! Thanks! Update: Removed the markdown I just posted here. It doesn't make sense since I added a lot of stuff as an html comment which you wouldn't be able to read anyway. I'll add it as a commit! Update 2: Commited to your repo @Serene-Arc !! Watch out! :-) So yeah, again: I'm up for any improvement suggestions / shorter versions. I think it's rather huge and chances are people see it as clutter when editing and get rid of it and forget about it too soon in the process of a PR's life-cycle. What do you both think? Honest opinions please! :-) |
|
Very nice idea! I don't object to the "failing check" approach, but one other option here is to instead have the Action post a comment on the PR thread. That's a little "softer" than a failing check (and would make it easier to see if there are other CI failures), but it would still prompt people to explicitly consider whether they want to add a changelog entry. (Again, though, totally cool if folks prefer the more straightforward failing-check version, and we just have to remember to ignore that failure in some cases.) |
|
I think the issue with that is when the comment would be posted. If we do the above thing of adding to the PR template that the changelog should be changed at the end of the review process, I'm not sure how to make an action that would do that. I don't know if there's a way to automatically tell when the 'end' of that process is for every PR. |
|
Ah, that's an interesting point. One "good-enough" option would be to simply check once, when the PR is first opened… I realize this has one big disadvantage over "continuously" checking on every commit, but it could be a good-enough solution:
|
|
Ah I like that suggestion. Only notify on open that there might be a thing called changelog! And we'd keep all the green/red meaning - I just realized on my own PR how dependent I am on those lights - all green just means all green to my brain it seems - I didnt really know when you asked me that recently @Serene-Arc ☝️😆🤣 |
Serene-Arc
force-pushed
the
changelog_workflow
branch
from
96705f1 to
48695ba
Compare
Serene-Arc
force-pushed
the
changelog_workflow
branch
from
6065664 to
d6dbcfd
Compare
Serene-Arc
force-pushed
the
changelog_workflow
branch
from
d6dbcfd to
8d7cb5c
Compare
Whoops! I think my edit of the PR template is gone. You might have force-pushed over it. No worries, I'll see if I can find it locally on my machine. I wanted to give it a retake anyway, it was way too verbose. I can do better! ;-) |
|
Oh no! I'm so sorry. I had to do a bunch of weird git stuff to test out because GitHub doesn't run the workflows in every case. |
|
No worries at all @Serene-Arc, happens :-) I found that commit in my local clone. Will submit a better version of it later today. |
- Some notes only visible to the contributor while editing. - Some tiny fixes in the existing texts in braces. - One of the "invisible" notes encourages to remove those help-texts to help uncluttering.
There was a problem hiding this comment.
Please a quick review of my updated template, also @Serene-Arc, does that make sense? I tried my best to stay concise, and hope people find it not too long for quickly giving it a read while opening a PR.
|
|
||
| - [ ] Documentation. (If you've added a new command-line flag, for example, find the appropriate page under `docs/` to describe it.) | ||
| - [ ] Changelog. (Add an entry to `docs/changelog.rst` to the bottom of one of the lists near the top of the document.) | ||
| - [ ] Tests. (Very much encouraged but not strictly required.) |
There was a problem hiding this comment.
Please @RollingStar prove-read my changes to the PR template. Does it address the things you mentioned properly? Thanks!
Co-authored-by: J0J0 Todos <[email protected]>
|
Looks good to me @JOJ0 ! Should be plenty for new contributors. |
|
Fixed a typo and reordered something in the PR template. IMHO we are good here. Push the button! :-) |
Since we're getting into the spirit of automated tools and making it easier for the maintainers to review PRs, I present this humble workflow.
It's simple: if the docs or the code is changed, then the workflow checks that the changelog file has been changed. If it hasn't, failed check.
This is meant to be a simple thing that we can use to make sure that every PR actually updates the changelog. Minimum intervention and just another check that the machine can do itself.