Add a CODE_REVIEW_CHECKLIST.md #1273
Conversation
stevendanna
force-pushed
the
ssd/pr-checklist
branch
from
fbc1330 to
d2bfdb7
Compare
stevendanna
requested review from
sdelano,
markan,
marcparadise,
ryancragun,
tylercloke and
srenatus
There was a problem hiding this comment.
I like that list. I think it's the right thing not to include overly vague "Does it work in all possible configurations?", but I do wonder if we have a set of first- and second-class deployment options? (I'm thinking about HA, tiered, external PG, external RMQ, and the like)
CODE_REVIEW_CHECKLIST.md
Outdated
| - [ ] Do any new configurables have a sensible default value? | ||
|
|
||
| - [ ] Are any new user-visible settings/features documented? (via a | ||
| linked PR to chef-docs) |
There was a problem hiding this comment.
[pedant] Is "chef-docs" the collective noun for chef-{dev,web}-docs, or am I missing something?
There was a problem hiding this comment.
Just realised the slack channel is called "chef-docs"... not sure if referencing that is a good thing, since that one is internal.
There was a problem hiding this comment.
Let's make this a link to the chef-web-docs repo.
| used by add-ons? If so, has add-on compatibility been | ||
| specifically tested? | ||
|
|
||
| - [ ] Has this PR changed an API response that may break API clients? |
There was a problem hiding this comment.
If we care about API clients (haha, of course we do), would it make sense to enumerate those we support? As in, you can't fix the world's Chef server API clients, but we can take care of chef-client. (Or is {chef-client} that set?)
Also, we're not mentioning API versioning or versioning altogether -- would it make sense to add some item for that? I'd assume the contributed change itself won't bump the version, but the necessity to bump the version might be relevant for code review?
There was a problem hiding this comment.
I think we mention API versioning in the sentence below this? Overall, I think which clients to fix would be a discussion during the PR depending on which APIs were changing.
There was a problem hiding this comment.
Yep, I've read over that bit 😖
There was a problem hiding this comment.
I think at a minimum we care about chef-client, ridley (for berkshelf), and possibly py-chef or chef-api as stretch goals. But the first two are critical.
There was a problem hiding this comment.
@thommay I'm going to merge this without the explicit list, but I think we should continue to have a discussion about what clients we should be testing compat with. Currently API compat is only tested with pedant, but perhaps we could add some "external integration" tests to ensure that we don't break any the core features of these products. To be honest though, right now most bugs concern add-on compat and not REST API compat.
There was a problem hiding this comment.
Yeah, I think that's fine. In general API clients should assume that chef-client (or pedant) is the reference implementation and be bug compatible with that, but it would be good to have some assurance.
Yup, we have another task to define the list of our top-tier deployment scenarios. |
CODE_REVIEW_CHECKLIST.md
Outdated
| specifically tested? | ||
|
|
||
| - [ ] Has this PR changed an API response that may break API clients? | ||
| If so, does this change require and API version bump and has |
CODE_REVIEW_CHECKLIST.md
Outdated
| - [ ] Can any of the logging be removed or moved to a more detailed | ||
| log level? | ||
|
|
||
| - [ ] Are any new logs properly rotated? |
There was a problem hiding this comment.
Is it warranted to have this singled out like that? I mean, any new service could have various ways for filling up the filesystem. But I guess, producing logs is something every service does... 🤔
There was a problem hiding this comment.
I'll remove this one. This list is kind of long and we were looking for way to shorten it a bit. This question was on a list we started a bit ago about things to consider for a new service, but I'm not sure it is appropriate here.
CONTRIBUTING.md
Outdated
|
|
||
| ## Reporting an Issue | ||
|
|
||
| When reporting an issue, please try to include as much detail as |
There was a problem hiding this comment.
This might be better moved to an issue template.
stevendanna
force-pushed
the
ssd/pr-checklist
branch
from
f4e2793 to
997c0d4
Compare
The Chef Server 12.15 release had a number of regressions. As part of the post-mortem for that release, we agreed that a PR checklist may be helpful to have more fruitful discussions during code review. This is a first attempt at such a checklist. Signed-off-by: Steven Danna <[email protected]>
Signed-off-by: Marc Paradise <[email protected]>
stevendanna
force-pushed
the
ssd/pr-checklist
branch
from
997c0d4 to
e510359
Compare
The Chef Server 12.15 release had a number of regressions. As part of
the post-mortem for that release, we agreed that a PR checklist may be
helpful to have more fruitful discussions during code review.
This is a first attempt at such a checklist.
Signed-off-by: Steven Danna [email protected]