Repository navigation
Conversation
| - [ ] Useful comments and logging | ||
| - [ ] Tests added | ||
| - [ ] Docs added or updated per the [style guide](https://docs.stackable.tech/home/nightly/contributor/docs/style-guide) | ||
| - [ ] Cargo.toml only references git tags |
There was a problem hiding this comment.
In the cases we need to reference something else (commit hash for a fix), what happens here? Does the reviewer not tick it, or edit the description and strike it through?
There was a problem hiding this comment.
I don't know. This has been in here for ages :) Not from me (I think). I'm happy to change it, I took this unchanged. Suggestions?
There was a problem hiding this comment.
Maybe we could add some guidance like if an item can't be ticked, explain in a comment. That saves the reviewer from having to edit the description to add strike-throughs.
There was a problem hiding this comment.
That's at the top: "" and we had a previous version of that forever "- Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant"
And I've done that sometimes to just delete everything that is irrelevant.
I'm very happy to make it clearer if you have an idea how.
| - [ ] Links to generated (nightly) docs added | ||
| - [ ] Release note snippet added | ||
| - [ ] AI review done, findings addressed | ||
| - [ ] OpenShift compatible |
There was a problem hiding this comment.
Maybe one for here is code style aligns with https://docs.stackable.tech/home/stable/contributor/code-style-guide/
| - [ ] OpenShift compatible | |
| - [ ] Code style aligns with the [style guide](https://docs.stackable.tech/home/stable/contributor/code-style-guide/) |
| - [ ] Changes need to be "offline" compatible | ||
| - [ ] Links to generated (nightly) docs added | ||
| - [ ] Release note snippet added | ||
| - [ ] AI review done, findings addressed |
There was a problem hiding this comment.
Is there guidance on this for consistency?
Is it a matter of "review this code", or is it a particular skill, or checklist of things?
There was a problem hiding this comment.
Just that we have our internal skill for that - I didn't want to "enforce" anything (yet at least). Happy to hear opinions. At the on-site we didn't come to a conlusion if I remember correctly.
There was a problem hiding this comment.
Ok, so if I understand correctly, this can be left unchecked if not done?
I think we just need some added guidance around unchecked items still being allowed to be merged.
It is mentioned to remove items that don't apply, but this might apply but not get done, and in some cases that is ok?
NickLarsenNZ
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
NickLarsenNZ
left a comment
NickLarsenNZ
left a comment
There was a problem hiding this comment.
Approving anyway, but I left some comments for things I think need improving while we're here.
NickLarsenNZ
moved this from Development: In Review
to Development: Track
in Stackable Engineering
|
I don't want to stall this, or make you change things carried over from before. Just thought it a good chance to make changes while there is a PR open. I approve anyway in case you want to get this in, and leave other things for a follow up. |
|
Honestly: This is not urgent and we've lived with the old version for X years so I think it's fine if we take some time during the overhaul now. I really appreciate the comments and I'm happy to change all of this. It should be useful for you and the others. If everyone ends up ignoring it that's not helpful. So happy to do more iterations until we're happy. |
The old template was mostly ignored (well, at least not the boxes ticked).
I want it more obvious that docs & release notes need to be added, a lot of the other things are more "reminders"
That plus the mandatory local AI review.