Repository navigation
chore: Restructure the pull request template #658
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,38 +5,50 @@ DO NOT MANUALLY EDIT THIS FILE | |||||
|
|
||||||
| ## Description | ||||||
|
|
||||||
| <!-- Becomes the squash commit message. --> | ||||||
|
|
||||||
| ## Docs | ||||||
|
|
||||||
| <!-- | ||||||
| Link any relevant nightly docs pages this changes or adds. | ||||||
| If a parent issue covers the docs, link that issue instead. | ||||||
| "None" if none. | ||||||
| --> | ||||||
|
|
||||||
| ## Release note | ||||||
|
|
||||||
| <!-- | ||||||
| A snippet for the release notes, plus the `release-note` label, or `release-note/action-required` if users must do something on upgrade. | ||||||
| If a parent issue has the release note, link that issue instead and skip the label here. | ||||||
| "None" if not user-facing. | ||||||
|
|
||||||
| For bugs worth mentioning: | ||||||
| "Previously, <what went wrong>. With this release, <how it was fixed>." | ||||||
| --> | ||||||
|
|
||||||
| ## Checklist | ||||||
|
|
||||||
| <!-- Delete what doesn't apply. --> | ||||||
|
|
||||||
| ### Author | ||||||
|
|
||||||
| - [ ] AI review done, findings addressed | ||||||
| - [ ] OpenShift compatible | ||||||
|
Member
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/
Suggested change
|
||||||
| - [ ] Works offline / air-gapped | ||||||
| - [ ] CRD changes approved, all new fields documented per the [style guide](https://docs.stackable.tech/home/nightly/contributor/docs/style-guide) | ||||||
| - [ ] Helm chart installs, operator works | ||||||
| - [ ] Integration tests passed (non-trivial changes) | ||||||
| - [ ] Changelog updated | ||||||
|
|
||||||
| ### Reviewer | ||||||
|
|
||||||
| - [ ] 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 | ||||||
|
Member
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?
Member
Author
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?
Member
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.
Member
Author
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. |
||||||
|
|
||||||
| ### Acceptance | ||||||
|
|
||||||
| - [ ] Check for release notes and docs | ||||||
| - [ ] Deprecation: `type/deprecation` label, added to the [deprecation schedule](https://github.com/orgs/stackabletech/projects/44/views/1) | ||||||
| - [ ] Experimental: `type/experimental` label, added to the [experimental tracker](https://github.com/orgs/stackabletech/projects/47) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?