dev guidelines: updates and new things I learned #320
Conversation
| @@ -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"` | |||
There was a problem hiding this comment.
Does this apply to all imports or only to imports that convey versions like v1 or v1alpha1?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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")
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
No description provided.