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

Jump to bottom

dev guidelines: updates and new things I learned #320

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

dev guidelines: updates and new things I learned #320

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
17 changes: 15 additions & 2 deletions site/content/shared/docs/latest/development_guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ title: Coding Guidelines for Carvel
## General Mindset and Sensibilities
### Naming
* Consistent and descriptive, without abandoning golang’s terse style.
* Flag names should be nouns not verbs ( --warnings, not --warn)
* Flag names should be nouns not verbs ( --warnings, not --warn).
* Prefer names that indicate behavior or effect over names that describe
implementation.

### Modularity
* Each Carvel tool is modular and composable, with aggressively limited scope
Expand Down Expand Up @@ -68,6 +70,12 @@ Developers should feel free to add more structure as complexity grows by making
```
if err := foo(); err != nil { //…
```
* Prefer to minimize empty lines:
* One blank line between functions or sections can be helpful, two blank
lines is gratuitous.
* Don't leave blank lines at the bottom of a function or test
* When defining a function with no implementation, keep the `{}` on the same
line as the function declaration.
* Dependencies
* are all vendored locally and version controlled
* `go mod vendor; go mod tidy` are your friends; hack/build.sh runs them.
Expand All @@ -88,6 +96,7 @@ Developers should feel free to add more structure as complexity grows by making
* Generators can be run via hack/gen.sh
* Kapp-controller’s aggregated API server has a separate generator: hack/gen-apiserver.sh
* The CRD yaml is generated via[ a separate script](https://github.com/vmware-tanzu/carvel-kapp-controller/blob/85e814cda7109169809ede1c8a4f211739ad15d2/hack/gen-crds.sh) that is run by hack/build.sh
* Imports: use the alias `metav1` when importing `"k8s.io/apimachinery/pkg/apis/meta/v1"`
Copy link
Member

Choose a reason for hiding this comment

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

Does this apply to all imports or only to imports that convey versions like v1 or v1alpha1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it applies to exactly this (very common in controllers) import. I didn't mean to imply any wildcards in this regex, although i guess there's a similar rule for .../core/v1 . What should i change to make this more clear?

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion would be:

Imports: when the imported package is named after a version, the previous package should be added for more context or something to identify the intent (ex: metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" or regv1 "github.com/google/go-containerregistry/pkg/v1")


## Development Process
### Controller specific workflows
Expand Down Expand Up @@ -127,6 +136,8 @@ We write mainly e2es and units; some tools have [performance tests](https://git
* Test Assets:
* If a test requires additional artifacts or assets they should live in a separate /assets subfolder.
* Test assets should include comments describing how to generate or modify those assets. [example](https://github.com/vmware-tanzu/carvel-kapp-controller/blob/1844e157b6de4048cec3ba0e53fc699d37e9c71e/test/e2e/assets/https-server/certs-for-custom-ca.yml#L9)
* Names: Omit words like "fake" or "mock" from the names of variables or dummies
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for this guideline?
Personally, I find it helpful to know if a variable is a fake or a mock instead of the real implementation.
Now if the value in the variable is not a Test Double, I agree with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's basically this comment from DK: carvel-dev/secretgen-controller#41 (comment)

nit: i typically recommend against using name fake* in test. it just becomes noisy and doesnt add any value. either nsCheck or negativeExclusionCheck

In that example I think Martin Fowler and his friend would've said I was using a 'stub' (a func that ignores its argument and always returns false).

IMO part of the value of a guide like this is that it takes the approver's opinions and makes them available before code review, rather than after. In kpm-land, DK is basically the only approver, and I'm completely content to just take his opinions on things like this and enshrine them. If you're into debates though you-all can hash it out... and maybe his opinion/preference is more nuanced than I'm presenting it here.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree because usually, the Test Double names are used a little bit too freely.
Nevertheless, I think the sentence is too prescriptive. Maybe an example https://github.com/vmware-tanzu/carvel-imgpkg/blob/develop/pkg/imgpkg/bundle/locations_configs_test.go#L19 you can see that we do use the fake word in these variables, and they do convey meaning to the variable informing the reader that the variable will behave like what they expect but it is not a real deal.

made for the test.

### Issues, Branching, Pull Requests, Approval
* Issues (see also, [issue triaging docs](https://www.google.com/url?q=https://github.com/vmware-tanzu/carvel/blob/develop/processes/issue-triage.md&sa=D&source=docs&ust=1634161804254000&usg=AOvVaw3al0fXNnNVR7ynUf-DwcU0) for more info!)
Expand Down Expand Up @@ -174,5 +185,7 @@ Carvel uses semver, x.y.z version structure, with all tools at major version x=0
* Aspirational: a single script that runs linter, no-dirty-files, and unit tests locally

### Release Process
* OSS Releases: We’re mostly trying to use goreleaser, which relies on git tags, but this varies by repo; see the relevant dev.md for details
* OSS Releases: varies by tool, see the relevant dev.md for details. Many of the
command line tools are using [shared
scripts](https://github.com/vmware-tanzu/carvel-release-scripts).
* Vmware releases: have their own process; Vmware employees should see internal docs.