Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enrich the contributor guides #263

Merged
merged 1 commit into from
Jan 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
<!-- Thanks for sending a pull request!
<!--
Thanks for sending a pull request!
Please read the CLA carefully before submitting your contribution to Mercari.
Under any circumstances, by submitting your contribution, you are deemed to accept and agree to be bound by the terms and conditions of the CLA.
https://www.mercari.com/cla/

Also, please read the contributor guide, which contains some general rules and suggestions:
https://github.com/mercari/tortoise/blob/main/docs/contributor-guide.md
-->

#### What this PR does / why we need it:
Expand Down
2 changes: 2 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,7 @@ linters-settings:
skip-generated: false
custom-order: true
linters:
# We only configure a few linters now,
# if you want to enforce other rules, please open an issue and discuss.
enable:
- gci
70 changes: 52 additions & 18 deletions docs/contributor-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,39 +2,57 @@

<img alt="Tortoise" src="images/climbing.jpg" width="400px"/>

This document helps you to climb the road to the contributor.
This document helps you in climbing the road to the contributor.

### Coding guide
**Please discuss changes before editing this page, even minor ones.**

You **must** follow the linter configured in the repository.
Avoid using `//nolint` as much as possible even if you just want to ignore very tiny thing.
(broken windows theory)
### Rules

You also can run the following command and the linters fix some problems automatically.
- You **must** adhere to the linters configured in the repository.
Avoid using `//nolint` except in extraordinary circumstances. Strive for compliance even with minor issues.
- You **must** write unit tests for any new or modified functionality.
Almost all changes should be accompanied by corresponding tests to ensure behavior is as expected.
- Bugs always arise from insufficient or incorrect tests.
You **must** update or add tests when fixing bugs to ensure the fix is valid.
If you didn’t add a test, you didn’t fix the bug.
- **Do not** test anything manually in your Kubernetes cluster.
Instead, you **must** implement all testing in the e2e tests.
- **Do not** bring any breaking change in Tortoise CRD.
However, you **may** bring breaking changes in Golang functions or types within the repository - we're not developping the library and don't have to care much about downstream dependencies.

```
make lint-fix
```
### Suggestion

- Enforce code-style issues with linters. When you want someone to fix a code-style, you can likely find a linter to detect such code-style issues in [golangci-lint](https://golangci-lint.run/usage/linters/).

#### Useful Golang official documents

We only configure a few linters now,
if you want to enforce something further, please open the issue and discuss.
Contributors are expected to adhere to these official guidelines.
Some of these recommendations are enforced by linters, while others are not.

- [Go Wiki: Go Code Review Comments](https://go.dev/wiki/CodeReviewComments).
- [Go Wiki: Go Test Comments](https://go.dev/wiki/TestComments)
- [Effective Go](https://go.dev/doc/effective_go)

### Testing

You **must** implement the unit test (or add the test cases to existing tests)
when you change something.
The following command runs all tests (both unit tests and e2e tests) in the repository.

Almost all behavior changes likely have to have an addition/change in testing.
```shell
make test
```

#### The integration tests
#### E2e tests

Each Webhook and the controller have the integration tests.
- [/controllers/tortoise_controller_test.go](../controllers/tortoise_controller_test.go)
- [/api/autoscaling/v2/horizontalpodautoscaler_webhook_test.go](../api/autoscaling/v2/horizontalpodautoscaler_webhook_test.go)
- [/api/v1beta3/tortoise_webhook_test.go](../api/v1beta3/tortoise_webhook_test.go)

If you implement a major feature, that is something achieved by combining multiple services,
or you implement something in webhooks, you **must** add a new test case in them.
Their test suite are mostly defined in Yaml files to add test cases easier, e.g., [/controllers/testdata/](../controllers/testdata/).

Specifically about the controller's e2e test, it only simulates one reconciliation to simplify each test case.
For example, if you expect Tortoise is changed to state-a in one reconciliation and to state-b in next reconciliation,
you have to write two tests, one per reconciliation.

#### Debuggable Integration Test

Expand All @@ -56,4 +74,20 @@ $ rm -rf /var/folders/3r/38tmpwm105d59kp_zlfbch0h0000gp/T/k8s_test_framework_127
```

(This debuggable integration test is inspired by [zoetrope/test-controller](https://github.com/zoetrope/test-controller/tree/39902a6510642370973063afa7bcefe2997b7387)
licensed under [MIT License](https://github.com/zoetrope/test-controller/blob/39902a6510642370973063afa7bcefe2997b7387/LICENSE))
licensed under [MIT License](https://github.com/zoetrope/test-controller/blob/39902a6510642370973063afa7bcefe2997b7387/LICENSE) ❤️ )

### Linters

The following command runs all enabled linters.

```shell
make lint
```

Some linters can fix some problems automatically by the following command.

```shell
make lint-fix
```

See [.golangci.yml] about the linters enabled in the repository.
Loading