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.

Sign up for GitHub

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

Add Technical and style guide to the contribution guide #2250

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Add Technical and style guide to the contribution guide #2250

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 19 additions & 0 deletions docs/developer-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,25 @@ You can undeploy Katib v1beta1 manifests from a Kubernetes cluster as follows:
make undeploy
```

## Technical and style guide

The following guidelines apply primarily to Katib,
but other projects like [Training Operator](https://github.com/kubeflow/training-operator) might also adhere to them.

## Go Development

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make a section about Golang here since Katib has various components written in Go, Python, Typescript?

Suggested change
## Golang Development

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

When coding:

- Follow [effective go](https://go.dev/doc/effective_go) guidelines.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add that developer should use gofmt to format their code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to focus on mentioning only items that it is difficult to automatically detect in CI.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to say that developer can run ./hack/verify-gofmt.sh to format their Go codes. In that case, we can reduce number of changes that contributors need to do when they create PRs.

Like in K8s developer guides: https://github.com/kubernetes/community/blob/d48200bb58cfb877e4cd77043db1e9eb6d4c8e4e/contributors/devel/development.md#presubmission-verification

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense.
As an alternative, I would suggest using make check. WDYT?

katib/Makefile

Line 31 in 4617346

check: generated-codes go-mod fmt vet lint

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that even better, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

- Run locally [`make check`](https://github.com/kubeflow/katib/blob/46173463027e4fd2e604e25d7075b2b31a702049/Makefile#L31)
to verify if changes follow best practices before submitting PRs.

Testing:

- Use [`cmp.Diff`](https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff) instead of `reflect.Equal`, to provide useful comparisons.
- Define test cases as maps instead of slices to avoid dependencies on the running order.
Map key should be equal to the test case name.

## Modify controller APIs

If you want to modify Katib controller APIs, you have to
Expand Down