-
Notifications
You must be signed in to change notification settings - Fork 3
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #13 from mjudeikis/paperwork
📖 License, contrib guide, owners & nits
- Loading branch information
Showing
6 changed files
with
485 additions
and
5 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,232 @@ | ||
# Contributing to tmc | ||
|
||
tmc is [Apache 2.0 licensed](https://github.com/kcp-dev/contrib-tmc/tree/main/LICENSE) and we accept contributions via | ||
GitHub pull requests. | ||
|
||
Please read the following guide if you're interested in contributing to tmc. | ||
|
||
## Certificate of Origin | ||
|
||
By contributing to this project you agree to the Developer Certificate of | ||
Origin (DCO). This document was created by the Linux Kernel community and is a | ||
simple statement that you, as a contributor, have the legal right to make the | ||
contribution. See the [DCO](https://github.com/kcp-dev/contrib-tmc/tree/main/DCO) file for details. | ||
|
||
## Getting started | ||
|
||
### Prerequisites | ||
|
||
1. Clone this repository. | ||
2. [Install Go](https://golang.org/doc/install) (1.19+). | ||
3. Install [kubectl](https://kubernetes.io/docs/tasks/tools/#kubectl). | ||
|
||
### Build & verify | ||
1. In one terminal, build and start `tmc`: | ||
``` | ||
go run ./cmd/tmc start | ||
``` | ||
|
||
2. In another terminal, tell `kubectl` where to find the kubeconfig: | ||
|
||
``` | ||
export KUBECONFIG=.tmc/admin.kubeconfig | ||
``` | ||
|
||
3. Confirm you can connect to `tmc`: | ||
|
||
``` | ||
kubectl api-resources | ||
``` | ||
|
||
## Finding areas to contribute | ||
|
||
Starting to participate in a new project can sometimes be overwhelming, and you may not know where to begin. Fortunately, we are here to help! We track all of our tasks here in GitHub, and we label our issues to categorize them. Here are a couple of handy links to check out: | ||
|
||
* [Good first issue](https://github.com/kcp-dev/contrib-tmc/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) issues | ||
* [Help wanted](https://github.com/kcp-dev/contrib-tmc/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22) issues | ||
|
||
You're certainly not limited to only these kinds of issues, though! If you're comfortable, please feel free to try working on anything that is open. | ||
|
||
We do use the assignee feature in GitHub for issues. If you find an unassigned issue, comment asking if you can be assigned, and ideally wait for a maintainer to respond. If you find an assigned issue and you want to work on it or help out, please reach out to the assignee first. | ||
|
||
Sometimes you might get an amazing idea and start working on a huge amount of code. We love and encourage excitement like this, but we do ask that before you embarking on a giant pull request, please reach out to the community first for an initial discussion. You could [file an issue](https://github.com/kcp-dev/contrib-tmc/issues/new/choose), send a discussion to our [mailing list](https://groups.google.com/g/kcp-dev), and/or join one of our [community meetings](https://github.com/kcp-dev/contrib-tmc/issues?q=is%3Aissue+is%3Aopen+label%3Acommunity-meeting). For now we are still part of KCP community and we are using their community meeting for now. | ||
|
||
Finally, we welcome and value all types of contributions, beyond "just code"! Other types include triaging bugs, tracking down and fixing flaky tests, improving our documentation, helping answer community questions, proposing and reviewing designs, etc. | ||
|
||
|
||
## Priorities & milestones | ||
|
||
We prioritize issues and features both synchronously (during community meetings) and asynchronously (Slack/GitHub conversations). | ||
|
||
We group issues together into milestones. Each milestone represents a set of new features and bug fixes that we want users to try out. We aim for each milestone to take about a month from start to finish. | ||
|
||
You can see the [current list of milestones](https://github.com/kcp-dev/contrib-tmc/milestones?direction=asc&sort=due_date&state=open) in GitHub. | ||
|
||
For a given issue or pull request, its milestone may be: | ||
|
||
- **unset/unassigned**: we haven't looked at this yet, or if we have, we aren't sure if we want to do it and it needs more community discussion | ||
- **assigned to a named milestone** | ||
- **assigned to `TBD`** - we have looked at this, decided that it is important and we eventually would like to do it, but we aren't sure exactly when | ||
|
||
If you are confident about the target milestone for your issue or PR, please set it. If you don’t have permissions, please ask & we’ll set it for you. | ||
|
||
|
||
### Story tasks | ||
|
||
Story tasks in an epic should generally represent an independent chunk of work that can be implemented. These don't necessarily need to be copied to standalone GitHub issues; it's ok if we just track the story in the epic as a task. On a case by case basis, if a story seems large enough that it warrants its own issue, we can discuss creating one. | ||
|
||
Please tag yourself using your GitHub handle next to a story task you plan to work on. If you don't have permission to do this, please let us know by either commenting on the issue, or reaching out in Slack, and we'll assist you. | ||
|
||
When you open a PR for a story task, please edit the epic description and add a link to the PR next to your task. | ||
|
||
When the PR has been merged, please make sure the task is checked off in the epic. | ||
|
||
## Tracking work | ||
|
||
### Issue status and project board | ||
|
||
We use the Github projects beta for project management, compare [our project board](https://github.com/orgs/kcp-dev/projects/15). Please add issues and PRs into the kcp project and update the status (new, in-progress, ...) for those you are actively working on. | ||
|
||
### Unplanned/untracked work | ||
If you find yourself working on something that is unplanned and/or untracked (i.e., not an open GitHub issue or story task in an epic), that's 100% ok, but we'd like to track this type of work too! Please file a new issue for it, and when you have a PR ready, mark the PR as fixing the issue. | ||
|
||
|
||
## Coding guidelines & conventions | ||
|
||
- Always be clear about what clients or client configs target. Never use an unqualified `client`. Instead, always qualify. For example: | ||
- `rootClient` | ||
- `orgClient` | ||
- `pclusterClient` | ||
- `rootKcpClient` | ||
- `orgKubeClient` | ||
- Configs intended for `NewForConfig` (i.e. today often called "admin workspace config") should uniformly be called `clusterConfig` | ||
- Note: with org workspaces, `kcp` will no longer default clients to the "root" ("admin") logical cluster | ||
- Note 2: sometimes we use clients for same purpose, but this can be harder to read | ||
- Cluster-aware clients should follow similar naming conventions: | ||
- `crdClusterClient` | ||
- `kcpClusterClient` | ||
- `tmcClusterClient` | ||
- `kubeClusterClient` | ||
- `clusterName` is a kcp term. It is **NOT** a name of a physical cluster. If we mean the latter, use `pclusterName` or similar. | ||
- In the syncer: upstream = kcp, downstream = pcluster. Depending on direction, "from" and "to" can have different meanings. `source` and `sink` are synonyms for upstream and downstream. | ||
- Qualify "namespace"s in code that handle up- and downstream, e.g. `upstreamNamespace`, `downstreamNamespace`, and also `upstreamObj`, `downstreamObj`. | ||
- Logging: | ||
- Use the `fmt.Sprintf("%s|%s/%s", clusterName, namespace, name` syntax. | ||
- Default log-level is 2. | ||
- Controllers should generally log (a) **one** line (not more) non-error progress per item with `klog.V(2)` (b) actions like create/update/delete via `klog.V(3)` and (c) skipped actions, i.e. what was not done for reasons via `klog.V(4)`. | ||
- When orgs land: `clusterName` or `fooClusterName` is always the fully qualified value that you can stick into obj.ObjectMeta.ClusterName. It's not necessarily the `(Cluster)Workspace.Name` from the object. For the latter, use `workspaceName` or `orgName`. | ||
- Generally do `klog.Errorf` or `return err`, but not both together. If you need to make it clear where an error came from, you can wrap it. | ||
- New features start under a feature-gate (`--feature-gate GateName=true`). (At some point in the future), new feature-gates are off by default *at least* until the APIs are promoted to beta (we are not there before we have reached MVP). | ||
- Feature-gated code can be incomplete. Also their e2e coverage can be incomplete. **We do not compromise on unit tests**. Every feature-gated code needs full unit tests as every other code-path. | ||
- Go Proverbs are good guidelines for style: https://go-proverbs.github.io/ – watch https://www.youtube.com/watch?v=PAAkCSZUG1c. | ||
- We use Testify's [require](https://pkg.go.dev/github.com/stretchr/testify/require) a | ||
lot in tests, and avoid | ||
[assert](https://pkg.go.dev/github.com/stretchr/testify/assert). | ||
|
||
Note this subtle distinction of nested `require` statements: | ||
```Golang | ||
require.Eventually(t, func() bool { | ||
foos, err := client.List(...) | ||
require.NoError(err) // fail fast, including failing require.Eventually immediately | ||
return someCondition(foos) | ||
}, ...) | ||
``` | ||
and | ||
```Golang | ||
require.Eventually(t, func() bool { | ||
foos, err := client.List(...) | ||
if err != nil { | ||
return false // keep trying | ||
} | ||
return someCondition(foos) | ||
}, ...) | ||
``` | ||
The first fails fast on every client error. The second ignores client errors and keeps trying. Either | ||
has its place, depending on whether the client error is to be expected (e.g. because of asynchronicity making the resource available), | ||
or signals a real test problem. | ||
|
||
### Using Kubebuilder CRD Validation Annotations | ||
|
||
All of the built-in types for `tmc-kcp` are `CustomResourceDefinitions`, and we generate YAML spec for them from our Go types using [kubebuilder](https://github.com/kubernetes-sigs/kubebuilder). | ||
|
||
When adding a field that requires validation, custom annotations are used to translate this logic into the generated OpenAPI spec. [This doc](https://book.kubebuilder.io/reference/markers/crd-validation.html) gives an overview of possible validations. These annotations map directly to concepts in the [OpenAPI Spec](https://swagger.io/specification/#data-type-format) so, for instance, the `format` of strings is defined there, not in kubebuilder. Furthermore, Kubernetes has forked the OpenAPI project [here](https://github.com/kubernetes/kube-openapi/tree/master/pkg/validation) and extends more formats in the extensions-apiserver [here](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types_jsonschema.go#L27). | ||
|
||
### Getting your PR Merged | ||
|
||
The `tmc` project uses `OWNERS` files to denote the collaborators who can assist you in getting your PR merged. There | ||
are two roles: reviewer and approver. Merging a PR requires sign off from both a reviewer and an approver. | ||
|
||
|
||
## Continuous Integration | ||
|
||
tmc uses a combination of [GitHub Actions](https://help.github.com/en/actions/automating-your-workflow-with-github-actions) | ||
|
||
## Debugging flakes | ||
|
||
Tests that sometimes pass and sometimes fail are known as flakes. Sometimes, there is only an issue with the test, while | ||
other times, there is an actual bug in the main code. Regardless of the root cause, it's important to try to eliminate | ||
as many flakes as possible. | ||
|
||
### Unit test flakes | ||
If you're trying to debug a unit test flake, you can try to do something like this: | ||
|
||
```shell | ||
go test -race ./pkg/reconciler/apis/apibinding -run TestReconcileBinding -count 100 -failfast | ||
``` | ||
|
||
This tests one specific package, running only a single test case by name, 100 times in a row. It fails as soon as it | ||
encounters any failure. If this passes, it may still be possible there is a flake somewhere, so you may need to run | ||
it a few times to be certain. If it fails, that's a great sign - you've been able to reproduce it locally. Now you | ||
need to dig into the test condition that is failing. Work backwards from the condition and try to determine if the | ||
condition is correct, and if it should be that way all the time. Look at the code under test and see if there are any | ||
reasons things might not be deterministic. | ||
|
||
### End to end test flakes | ||
|
||
Debugging an end-to-end (e2e) test that is flaky can be a bit trickier than a unit test. Our e2e tests run in one of | ||
two modes: | ||
|
||
1. Tests share a single kcp server | ||
2. Tests in a package share a single kcp server | ||
|
||
The `e2e-shared-server` CI job uses mode 1, and the `e2e` CI job uses mode 2. | ||
|
||
There are also a handful of tests that require a fully isolated kcp server, because they manipulate some configuration | ||
aspects that are system-wide and would break all the other tests. These tests run in both `e2e` and `e2e-shared-server`, | ||
separate from the other kcp instance(s). | ||
|
||
You can use the same `run`, `-count`, and `-failfast` settings from the unit test section above for trying to reproduce | ||
e2e flakes locally. Additionally, if you would like to operate in mode 1 (all tests share a single kcp server), you can | ||
start a kcp instance locally in a separate terminal or tab: | ||
|
||
```shell | ||
bin/test-server | ||
``` | ||
|
||
Then, to have your test use that shared kcp server, you add `-args --use-default-kcp-server` to your `go test` run: | ||
|
||
```shell | ||
go test ./test/e2e/apibinding -count 20 -failfast -args --use-default-kcp-server | ||
``` | ||
## Community Roles | ||
|
||
### Reviewers | ||
|
||
Reviewers are responsible for reviewing code for correctness and adherence to standards. Oftentimes reviewers will | ||
be able to advise on code efficiency and style as it relates to golang or project conventions as well as other considerations | ||
that might not be obvious to the contributor. | ||
|
||
### Approvers | ||
|
||
Approvers are responsible for sign-off on the acceptance of the contribution. In essence, approval indicates that the | ||
change is desired and good for the project, aligns with code, api, and system conventions, and appears to follow all required | ||
process including adequate testing, documentation, follow ups, or notifications to other areas who might be interested | ||
or affected by the change. | ||
|
||
Approvers are also reviewers. | ||
|
||
### Management of `OWNERS` files | ||
|
||
If a reviewer or approver no longer wishes to be in their current role it is requested that a PR | ||
be opened to update the `OWNERS` file. `OWNERS` files may be periodically reviewed and updated based on project activity | ||
or feedback to ensure an acceptable contributor experience is maintained. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
Developer Certificate of Origin | ||
Version 1.1 | ||
|
||
Copyright (C) 2004, 2006 The Linux Foundation and its contributors. | ||
|
||
Everyone is permitted to copy and distribute verbatim copies of this | ||
license document, but changing it is not allowed. | ||
|
||
|
||
Developer's Certificate of Origin 1.1 | ||
|
||
By making a contribution to this project, I certify that: | ||
|
||
(a) The contribution was created in whole or in part by me and I | ||
have the right to submit it under the open source license | ||
indicated in the file; or | ||
|
||
(b) The contribution is based upon previous work that, to the best | ||
of my knowledge, is covered under an appropriate open source | ||
license and I have the right under that license to submit that | ||
work with modifications, whether created in whole or in part | ||
by me, under the same open source license (unless I am | ||
permitted to submit under a different license), as indicated | ||
in the file; or | ||
|
||
(c) The contribution was provided directly to me by some other | ||
person who certified (a), (b) or (c) and I have not modified | ||
it. | ||
|
||
(d) I understand and agree that this project and the contribution | ||
are public and that a record of the contribution (including all | ||
personal information I submit with it, including my sign-off) is | ||
maintained indefinitely and may be redistributed consistent with | ||
this project or the open source license(s) involved. |
Oops, something went wrong.