Add contributor and reviewer expectations #133
Conversation
|
/hold |
|
/lgtm |
initially i did that but then i realized lots of repos link to this file so it felt simpler, and not totally incorrect to keep the name the same probably gotta update other repos too tho so if you feel like the filename is totally wrong, im happy to change it. i felt neutral so left it whaddaya think @vdemeester ? |
|
I feel neutral about it too, just wanted to bring it up 😛 |
|
/lgtm I am 👍 for it, I think it would be great to point the documentation to new contributors to help them to contribute according to the project standards. I think it would be great if we can add further CI checks to "enforce" this instead of relying on the reviewers. For a start there should be plenty of git commit format scripts out there. Maybe that's possible for the others recommendations of this document |
💯 |
| [these best practices](https://chris.beams.io/posts/git-commit/), specifically: | ||
| * Follow [commit messages best practices](https://chris.beams.io/posts/git-commit/): | ||
| 1. Separate subject from body with a blank line | ||
| 2. Limit the subject line to 50 characters |
There was a problem hiding this comment.
I don't think I've been great about following this one myself. How strongly do we care? Should we enforce this in CI?
There was a problem hiding this comment.
I agree, it would be a good use of the reviewers time if this can be automated.
There was a problem hiding this comment.
I am 💯 for enforcing this in CI (and any of the other items in this list as well!)
tbh I'm a bit surprised that github doesn't flag it, but to answer your question about why this is important, its mostly b/c tools (like github!) usually treat shorter subject lines better.
for example:
GitHub defaults to the commit message subject as the PR title and truncates it when too long (sorry @dlorenc for singling you out here):
standards.md
Outdated
| * When using cmp.Diff the argument order is always (want, got) and in the error message include (-want +got) | ||
| (and/or use a lib like [PrintWantGot](https://github.com/tektoncd/pipeline/blob/master/test/diff/print.go)) | ||
| * Table driven tests: do not use the same test for success and fail cases if the logic is different | ||
| (e.g. do not have two sets of test logic, gated with `wantErr`) |
There was a problem hiding this comment.
This one is probably so common because the gotests tool generates tests this way: https://github.com/cweill/gotests
How strongly do we feel? I agree that it's a bad idea for complex tests, but find it nice for simpler ones.
There was a problem hiding this comment.
whoa ive never seen https://github.com/cweill/gotests before, thats pretty cool!
I agree that it's a bad idea for complex tests, but find it nice for simpler ones.
I find it consistently hard to work with. I think it's not a huge thing, but also the alternative (separate cases) is so much easier to understand and reason about that I think its worth it
- When im updating some code (esp when im not familiar with it) i often look at the test cases to try to understand the behavior. When the error cases and success cases are handled in the same test, it's hard to tell which are which, esp since folks often end up jumbling them together - forcing a certain order would HELP but we'd have to try to maintain that, feels simpler to have separate tests, and then also (2)
Some examples of the test cases mixed:
- https://github.com/tektoncd/triggers/blob/dd1aff6c87e66ad3e0051977fe58bf6b390dd749/pkg/interceptors/github/github_test.go#L50-L104
- https://github.com/tektoncd/triggers/blob/03e6e3244690325b44c0cd7f16a44d26e4723bf2/pkg/interceptors/bitbucket/bitbucket_test.go#L51-L104
- Adding more complexity to the tests validation itself is a) harder to understand and b) easier to make mistakes.
For example I feel like I need to really stop and pause to understand these:
if tc.wantErr && err == nil {
t.Fatal("Expected error but found nil instead")
} else if !tc.wantErr && err != nil {
t.Fatalf("Received unexpected error ( %#v )", err)
}
if (err != nil) != tt.wantErr {
t.Errorf("processTriggerSpec() error = %v. wantErr %v", err, tt.wantErr)
return
}
-
As a bonus I'll mention that this kind of thing in general is a "code smell": https://martinfowler.com/bliki/FlagArgument.html
-
Bonus bonus, cuz i just can't stop: in addition to the wantErr flag, there's often another value which the presence of depends on the value of wantErr, which is even more confusing and points toward it being cleaner to have 2 totally different interfaces (or tests in this case),
for example in this test https://github.com/tektoncd/pipeline/blob/2973b6e3b24dc1f1db94b65c7c8bccf1d84dd038/pkg/pullrequest/scm_test.go#L114 when wantErr is true, want is always the empty string
And then there's this one, check out the subtle job that return is doing:
if err != nil {
if !tt.wantErr {
t.Errorf("Interceptor.ExecuteTrigger() error = %v, wantErr %v", err, tt.wantErr)
}
return
}
got, err := ioutil.ReadAll(resp.Body)
if err != nil {
t.Fatalf("error reading response body %v", err)
}
if diff := cmp.Diff(tt.want, got); diff != "" {
t.Errorf("Interceptor.ExecuteTrigger (-want, +got) = %s", diff)
}
The return is making sure that if wantErr is true, the test continues no further - which I would say is not obvious at first glance and makes the test more complex
- Whoops I can't stop!!!! Sometimes it shows up and it's not even doing anything, for example in this https://github.com/tektoncd/triggers/blob/288a03f34e408d029ee2563f1cd980b2f99959a9/cmd/triggerRun/cmd/root_test.go#L181-L234 there is no reason for wantErr b/c there are no test cases that use it (and in fact really no reason for a table driven test at all)
standards.md
Outdated
| ### Go | ||
| ## API Design | ||
|
|
||
| * Avoid kubernetes specific feature in our APIs as much as possible |
There was a problem hiding this comment.
This could use some clarification - I'm not sure what would be a k8-specific feature and what wouldn't be.
There was a problem hiding this comment.
hm that's a good point. I'm not sure how to define it except with examples, e.g. podTemplate (b/c the pod is an implementation detail)
Maybe it's something like: exposing details of the underlying, non-tekton, k8s resources that are created to realize TaskRuns and PipelineRuns? (e.g. attributes of pods, pvcs)
@imjasonh i think you've been looking into this in the context of conformance, do you have any ideas how we can make this more clear?
There was a problem hiding this comment.
Now that we've got https://github.com/tektoncd/community/blob/master/design-principles.md#conformance I'll just removed this API Design section entirely and link there instead! Thanks @jerop :D
standards.md
Outdated
|
|
||
| ### Go | ||
| ## API Design |
There was a problem hiding this comment.
Should we link to the more comprehensive k8s API conventions?
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md
Some of this stuff is great to keep in mind. One of my favorites:
Think twice about bool fields. Many ideas start as boolean but eventually trend towards a small set of mutually exclusive options. Plan for future expansions by describing the policy options explicitly as a string type alias (e.g. TerminationMessagePolicy).
standards.md
Outdated
| * Read through linked issues and pull requests, including the discussions | ||
| * Reconciler changes: | ||
| * Avoid adding functions to the reconciler struct | ||
| * Avoid adding functions to the reconciler package |
There was a problem hiding this comment.
These two are definitely pipeline-specific, and sort of look like a general smell with the reconciler package :)
There was a problem hiding this comment.
we now have at least 3 projects with reconciler (pipelines, triggers, chains - i think?) and i believe the operator is about to have a reconciler as well, so i feel like it's worth calling them out
sort of look like a general smell with the reconciler package :)
do you mean the underlying packages we're using? id say this is a general smell with how ppl tend to work with existing code, preferring to add more to packages and structs that already exist vs creating new ones. in particular since the reconciler is at the heart of so many of our changes, folks tend to apply this approach to the reconciler
There was a problem hiding this comment.
Thanks, I have a couple of minor comments, but it looks great :)
Perhaps over time we could add a section about golang specific recommendations, for instance best practices on using interfaces (@vdemeester), data structures (sets of strings, appending to slices).
standards.md
Outdated
| * New functionality must be designed and approved, may require a TEP | ||
| * Bugs should be reported in detail | ||
| * Release notes filled in for user visible changes (bugs + features), | ||
| or removed if not applicable (refactoring, updating tests) |
There was a problem hiding this comment.
NIT: I believe with the release notes plugin now for no release notes we need to set "NONE" into the release notes block, but it may be ok to not go into this level of detail here.
There was a problem hiding this comment.
(although, the plugin is "only" enabled on pipeline, cli and operator so far)
There was a problem hiding this comment.
I've added a note about this also! Over time as we automate more maybe it will make sense to clearly identify which parts are automated and link to supporting docs + config
standards.md
Outdated
| * New features (and often/whenever possible bug fixes) have one or both of: | ||
| * Examples (i.e. yaml tests) | ||
| * End to end tests | ||
| * Reconciler tests |
There was a problem hiding this comment.
NIT: perhaps mention that this is only for project with a reconciler
There was a problem hiding this comment.
I'm gonna move this around a bit:
- put "tests" under "code"
- add a "reconciler" specific section under "code"
standards.md
Outdated
| * Examples (i.e. yaml tests) | ||
| * End to end tests | ||
| * Reconciler tests | ||
| * New types have: |
standards.md
Outdated
| * Validation + validation tests | ||
| * Unit tests: | ||
| * Coverage should remain the same or increase | ||
| * Test exported functions only, in isolation: |
There was a problem hiding this comment.
I understand that we want to enforce behaviour on the exported functions, while internal functions may change, however in some cases testing private functions can help bring much more coverage and reduce test complexity.
Of course if we change an internal function, tests will have to change with it.
There was a problem hiding this comment.
in some cases testing private functions can help bring much more coverage and reduce test complexity. Of course if we change an internal function, tests will have to change with it.
I'm sure there are exceptions but I'd say that 9/10 times when you find yourself doing that, if you made the private function public in a new package dedicated to whatever that function is doing, you'll be happy with the result
standards.md
Outdated
| * If we don’t definitely need the feature, don’t add it | ||
| * Must be backed up with [CI/CD specific use cases](https://github.com/tektoncd/community/blob/master/roadmap.md#mission-and-vision) | ||
| * Prefer keeping the API simple: | ||
| * If you can avoid adding a new feature, don’t add it |
There was a problem hiding this comment.
Perhaps here would refer to TEPs instead? e.g. say that any new feature should have been previously discussed and agreed upon in a TEP.
There was a problem hiding this comment.
we can probably eventually link to the API design principles that @jerop is working on which I think include this
There was a problem hiding this comment.
Updating the beginning of the doc to reference TEPs and design principles instead now
So much progress we've made :D
| ## Functionality | ||
|
|
||
| * It should be safe to cut a release at any time, i.e. merging this PR should not | ||
| put us into an unreleasable state |
| * It should be safe to cut a release at any time, i.e. merging this PR should not | ||
| put us into an unreleasable state | ||
| * When incrementally adding new features, this may mean that a release could contain | ||
| a partial feature, i.e. the type specification only but no functionality |
There was a problem hiding this comment.
It may be worth adding something about how we document this.
If a feature includes a new API or CRD we might want to document that it is still not functional?
standards.md
Outdated
|
|
||
| ## Content | ||
|
|
||
| * Whenever logic is added that uses an image that wasn’t used before, the image used should |
standards.md
Outdated
| * Avoid adding functions to the reconciler struct | ||
| * Avoid adding functions to the reconciler package | ||
| * Return an error if you want the change to be re-queued, otherwise do not | ||
| (better yet, use [permanent errors](https://github.com/tektoncd/pipeline/issues/2474)) |
There was a problem hiding this comment.
The PR is now merged, perhaps we could link to https://github.com/knative/pkg/blob/5358179e7499b1a3a4581d9d3673f391240ec86d/controller/controller.go#L516-L521 ?
There was a problem hiding this comment.
https://github.com/knative/pkg/blob/master/injection/README.md#generated-reconciler-responsibilities seems like it might be good to link to also
There was a problem hiding this comment.
wish there were docs on permanent errors :(
This builds on the [API Design section](https://github.com/tektoncd/community/pull/133/files#diff-11ec7b2edcebbbc9374c04b87a8e7dd9R80) of the [Tekton contributor and reviewer expectations](tektoncd#133). The goal of these principles is to provide a basis for design choices and help in making tradeoffs. The design principles are reusability, simplicity, flexibility and conformance. The principles were initially proposed in [this Google Doc](https://docs.google.com/document/d/1_iqwslxAOx-SPYosnTFS5kOZcmxcBWpuIULZHQHP85Q/edit?usp=sharing) that's visible to members of tekton-dev@.
This builds on the [API Design section](https://github.com/tektoncd/community/pull/133/files#diff-11ec7b2edcebbbc9374c04b87a8e7dd9R80) of the [Tekton contributor and reviewer expectations](tektoncd#133). The goal of these principles is to provide a basis for design choices and help in making tradeoffs. The design principles are reusability, simplicity, flexibility and conformance. The principles were initially proposed in [this Google Doc](https://docs.google.com/document/d/1_iqwslxAOx-SPYosnTFS5kOZcmxcBWpuIULZHQHP85Q/edit?usp=sharing) that's visible to members of tekton-dev@.
This builds on the [API Design section](https://github.com/tektoncd/community/pull/133/files#diff-11ec7b2edcebbbc9374c04b87a8e7dd9R80) of the [Tekton contributor and reviewer expectations](tektoncd#133). The goal of these principles is to provide a basis for design choices and help in making tradeoffs. The design principles are reusability, simplicity, flexibility and conformance. The principles were initially proposed in [this Google Doc](https://docs.google.com/document/d/1_iqwslxAOx-SPYosnTFS5kOZcmxcBWpuIULZHQHP85Q/edit?usp=sharing) that's visible to members of tekton-dev@.
This builds on the [API Design section](https://github.com/tektoncd/community/pull/133/files#diff-11ec7b2edcebbbc9374c04b87a8e7dd9R80) of the [Tekton contributor and reviewer expectations](tektoncd#133). The goal of these principles is to provide a basis for design choices and help in making tradeoffs. The design principles are reusability, simplicity, flexibility and conformance. The principles were initially proposed in [this Google Doc](https://docs.google.com/document/d/1_iqwslxAOx-SPYosnTFS5kOZcmxcBWpuIULZHQHP85Q/edit?usp=sharing) that's visible to members of tekton-dev@.
This builds on the [API Design section](https://github.com/tektoncd/community/pull/133/files#diff-11ec7b2edcebbbc9374c04b87a8e7dd9R80) of the [Tekton contributor and reviewer expectations](tektoncd#133). The goal of these principles is to provide a basis for design choices and help in making tradeoffs. The design principles are reusability, simplicity, flexibility and conformance. The principles were initially proposed in [this Google Doc](https://docs.google.com/document/d/1_iqwslxAOx-SPYosnTFS5kOZcmxcBWpuIULZHQHP85Q/edit?usp=sharing) that's visible to members of tekton-dev@.
This builds on the [API Design section](https://github.com/tektoncd/community/pull/133/files#diff-11ec7b2edcebbbc9374c04b87a8e7dd9R80) of the [Tekton contributor and reviewer expectations](#133). The goal of these principles is to provide a basis for design choices and help in making tradeoffs. The design principles are reusability, simplicity, flexibility and conformance. The principles were initially proposed in [this Google Doc](https://docs.google.com/document/d/1_iqwslxAOx-SPYosnTFS5kOZcmxcBWpuIULZHQHP85Q/edit?usp=sharing) that's visible to members of tekton-dev@.
These started out as just being for Tekton Pipelines, but @dibyom pointed out they make sense for triggers too, so let's see if we want to adopt these for all the tektoncd projects or if we just want to constrain it to triggers + pipelines. The goal of these guidelines is to make sure that contributors understand what is expected in their submissions and also so that a reviewer (especially if someone wants to join and start reviewing who hasn't reviewed as part of this project before) will know when they approve + lgtm what they are expected to have checked.
standards.md
Outdated
| * Try to make formatting look as good as possible (use preview mode to check) | ||
| * Follow [content](https://github.com/tektoncd/website/blob/master/content/en/doc-con-content.md) | ||
| and [formatting](https://github.com/tektoncd/website/blob/master/content/en/doc-con-formatting.md) guidelines | ||
| * Should explain thoroughly how the new feature works | ||
|
|
There was a problem hiding this comment.
| * Ensure that all links and references are valid | |
There was a problem hiding this comment.
It would be good if we could make sure none of the links on the docs are broken.
There was a problem hiding this comment.
good call! i think we had some automation that tried to do that at some point but i wouldnt be surprised if we turned it off b/c i think it was kinda flakey :S
but yeah, even manually checking links is something i try to remember to when reviewing :S
090f899 to
b002491
Compare
|
Okay I think at this point I've responded to everyone's feedback: thanks for your input and patience! PTAL again @vdemeester @dibyom @chmouel @dlorenc @sthaha @afrittoli @popcor255 🙏 |
7e583b5 to
ee0ba17
Compare
standards.md
Outdated
| * Should explain thoroughly how the new feature works | ||
| * Ensure that all links and references are valid |
There was a problem hiding this comment.
i think the docs should include not only a code snippet but also a reference to an end-to-end example that uses the feature
There was a problem hiding this comment.
yes. yes it should. 😅
tektoncd/pipeline#1288
There was a problem hiding this comment.
Okay I've tried to capture this, let me know what you think! We can always add to it (esp. if we are able to add some code snippet testing automation one day... :D)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Since first creating this PR we've had some really useful improvements that I've tried to encorporate: * The TEP process! * Design principles now have their own doc at https://github.com/tektoncd/community/blob/master/design-principles.md#conformance * release-notes plugin automation via Prow * Permanent errors support was merged Also made improvements from feedback such as: * Putting the reconciler/controller specific recommendations into their own block * Requiring links and references in docs be validated (thanks @popcor255 !)
ee0ba17 to
e4897c5
Compare
|
I'm thinking maybe we could merge this ~end of the week? Please let me know if you'd like more time to review. And we can always iterate on it! |
|
wow, this is awesome, looks great 👍 🎉 🎉 🎉 |
|
Anyone mind giving this an /lgtm ? @pritidesai @jerop @vdemeester @popcor255 @dibyom @afrittoli @sthaha @dlorenc @chmouel |
|
/lgtm |
|
/lgtm @bobcatfish |
|
/lgtm |
|
/lgtm |
|
/hold cancel |
Relates to: tektoncd#128 Signed-off-by: Luke Hinds <lhinds@redhat.com>
These started out as just being for Tekton Pipelines, discussed in the productivity
working group, but @dibyom pointed out they make sense for triggers too,
so let's see if we want to adopt these for all the tektoncd projects or if we just want to
constrain it to triggers + pipelines.
The goal of these guidelines is to make sure that contributors
understand what is expected in their submissions and also so that a
reviewer (especially if someone wants to join and start reviewing who
hasn't reviewed as part of this project before) will know when they
approve + lgtm what they are expected to have checked.
Would like to hear feedback from OWNERS (approvers and reviewers) across all Tektoncd projects, both:
(I skipped some repos where the OWNERS are already covered via other lists)