review/go operator – post PR review

[marcellanz]

As per: https://github.com/cloudstateio/cloudstate/pull/425#issuecomment-693105141

There's still a bit of work to do, but the main thing I want to see here is if the smoke tests are working.

Because of this note, enjoy my comments with caution. I'm pretty sure many points are already known. I collected my comments for the code in this review PR as it was too much work to place them here on every occasion in the (original) PR.

Feel free to pick whatever seems to be helpful or makes sense. There are quite a few missed "If with a short statement" statements, where it cleans up boilerplate code. Also, (deeply) nested if/else statements can be rewritten to get the "happy path" in front of the lines and give the reader a flow of what usually should happen, with returns where it doesn't.

Packages should be part of types, so instead of stores.PostgresStore one might write store.Postgres. Note the singular form of the package name, it's not wrong to have plural package names, but they usually don't have to be.

Readability: I've changed/regrouped sometimes blocks of code I think should stick together. At first removing all those whitespace lines seem to be too much, but sometimes they interrupt the happy path. So if things get prepared and then worked with, this sticks together. This can result in code where no whitespace lines remain for some time, and err returns occur on times where errors return from the happy path. This usually improves readability.

Function/method argument list length seem to be overly large often in the PR. I think it feels unusual to have so many arguments, or it feels unusual to have them grouped as they are often laid out each on one line. This might be a consequence of long type names or not using the package as part of the type. Long argument lists seem to indicate that the code can be refactored to capture the context of the data passed. Argument list now uncomfortably long now.

Variable names: I think many variables in this PR are longer than they needed to be. The code looks like written with a JAVA(?) background carrying with. It's not wrong, just generally unusual from a Go context of view. This applies to many *.go files in this PR. Also, as one can read elsewhere, Kubernetes code and style sometimes might differ from what idiomatic Go code looks likes and it might be here, in a k8s operator, normal too.

So statefulService might just be named service since we're in a method of the StatefulServiceReconciler, what other service would we ever talk about in this context.

Style: I've renamed receivers consistently where appropriate.StatefulServiceResourceConfig is named rc for resource config or c for config but not r. Also, CassandraStore is a store, hence s for its receivers name not c. Picky things, but I think they should be consistently used.

var vs. literal variables: vars are used to indicate that they're initialized later and their zero values are not to be used by default. This shows the reader, that, as said, they're not yet initialized as they get to be initialized by some calls later.

Panics: I think panics have to be commented and I've wondered how the correct way to end an operator is, and if it is by panicking.

Package documentation: packaged should be documented. I've placed TODO where they're missing.

Bugs/Refactorings: I have found no bugs, but I also did not review the operators functionality. I also did not refactor packages but if agreed on can do that. stores/xyz_store.go => store/xyz.go would be the most obvious ones.

goimports where used, as they are common to be used.

Consistency: I (hope) remained things as they where, if they where consistent or brought them into a consistent state. So if a style is used, even not idiomatic Go, it should be consistent. But also, as a counter-example, I made the long arguments lists being on one line, they are not readable this way, but also, their grouping before are non-idiomatic. As mentioned in the code, they can be captured into a type or the like to solve that issue.

About many comments/issues can be read here: https://github.com/golang/go/wiki/CodeReviewComments for self-reviews if needed.

TODO: I've left TODO(review comment) where things are not yet clear.

Again, take whatever seems to be helpful as the original PR stated to have: "still a bit of work to do".

sh..list: Perhaps I broke something, I did try not to. I also did not test the changes – have no access to Circle CI.

linter config used:

GOOS=linux GOARCH=amd64 golangci-lint run -E stylecheck,gosec,goimports,goheader,gocritic,whitespace,godot,exportloopref,unconvert

[pvlugter]

CircleCI is failing on the git diff not being clean, and suggests to make manifests test for the operator. Note that the generated files are committed as well.

And we should figure out how contributors can see the builds.

[marcellanz]

And we should figure out how contributors can see the builds.

I got access to the build in the meantime by just properly login.

[marcellanz]

CircleCI is failing on the git diff not being clean, and suggests to make manifests test for the operator. Note that the generated files are committed as well.

Yes. Types got type comments changed:

   validation:
     openAPIV3Schema:
-      description: StatefulService is the Schema for the statefulservices API
+      description: StatefulService is the Schema for the statefulservices API.

I'll let @jroper decide if that is appropriate to be used this way to the generated CRD definitions.

[jroper]

All the Go operator projects I've seen check in their generated code and YAML. It's not my preference, but I figure consistency with the rest of the ecosystem is the higher priority for easing contribution.