Versions updates #11
Versions updates #11
Conversation
Much cleaner YAML from kustomize, and some cleanup in configuration for golang-ci. Kubernetes 1.30 wasn't fully working in things here yet, it seemed like some wrinkles inside of dependencies. But Kubernetes 1.29 comes with an extra configmap to consider during test setup. Makefile organization was slightly to help with tracking the versions of utilities. Signed-off-by: Justin Kulikauskas <[email protected]>
More formatting and scanning. The github actions files for these checks has been combined, and will run only on push, removing the duplicated checks that appeared in Pull Requests. Also cleans up some oddities found with GOPATH, which resolved other oddities with coverage commands. Signed-off-by: Justin Kulikauskas <[email protected]>
|
Code Coverage Report for this branch (might be slow to update): |
PR Review 🔍
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| on: [push, pull_request] | ||
| on: [push] |
There was a problem hiding this comment.
Suggestion: Consider adding the pull_request event to the on trigger for the GitHub Actions workflow to ensure that checks are also run on pull requests, not just on pushes to the repository. This is important for catching issues before they are merged into the main branch. [enhancement]
| on: [push] | |
| on: [push, pull_request] |
| disable: | ||
| - bodyclose | ||
| - copyloopvar # prefer old style for now, more reliable across various scanners | ||
| - cyclop | ||
| - deadcode #deprecated |
There was a problem hiding this comment.
Suggestion: Re-enable the err113 linter to enforce consistent error handling practices across the codebase. This linter checks for errors that are not created or wrapped with %w in fmt.Errorf, which is a common best practice in Go for error handling. [best practice]
| disable: | |
| - bodyclose | |
| - copyloopvar # prefer old style for now, more reliable across various scanners | |
| - cyclop | |
| - deadcode #deprecated | |
| disable: | |
| - bodyclose | |
| - cyclop | |
| - deadcode #deprecated | |
| - depguard |
| .PHONY: manifests | ||
| manifests: $(CONTROLLER_GEN) $(KUSTOMIZE) ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. |
There was a problem hiding this comment.
Suggestion: Add a clean-up target in the Makefile to remove the generated binaries and other build artifacts. This helps in maintaining a clean workspace and can be useful before rebuilds to ensure that no stale artifacts are used. [maintainability]
| .PHONY: manifests | |
| manifests: $(CONTROLLER_GEN) $(KUSTOMIZE) ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. | |
| .PHONY: clean | |
| clean: | |
| rm -rf $(LOCAL_BIN)/* | |
| echo "Cleaned up build artifacts." | |
| .PHONY: manifests | |
| manifests: $(CONTROLLER_GEN) $(KUSTOMIZE) ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. |
There was a problem hiding this comment.
If it was any more complicated than one directory to rm, I would consider this.
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-go@v5 | ||
| with: | ||
| go-version-file: './go.mod' |
There was a problem hiding this comment.
Suggestion: Add a step to cache dependencies in the GitHub Actions workflow to speed up builds. This can significantly reduce build times by avoiding redundant downloads of dependencies that haven't changed. [performance]
| - uses: actions/checkout@v4 | |
| - uses: actions/setup-go@v5 | |
| with: | |
| go-version-file: './go.mod' | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: actions/cache@v3 | |
| with: | |
| path: | | |
| ~/.cache/go-build | |
| ~/go/pkg/mod | |
| key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} | |
| restore-keys: | | |
| ${{ runner.os }}-go- | |
| - uses: actions/setup-go@v5 | |
| with: | |
| go-version-file: './go.mod' |
There was a problem hiding this comment.
The setup-go action does this automatically based on the go.sum. I considered doing this for the other binaries that are separately installed, but when the go.sum cache is hit (as in, all dependencies to build the program are already cached), the actions take less than 2 minutes each. And that's good enough for me to not worry about the additional cache setup.
| fmt: $(GOFUMPT) $(GCI) | ||
| go mod tidy | ||
| find . -not \( -path "./.go" -prune \) -name "*.go" | xargs $(GOFUMPT) -l -w | ||
| find . -not \( -path "./.go" -prune \) -name "*.go" | xargs $(GCI) write --skip-generated -s standard -s default -s localmodule |
There was a problem hiding this comment.
Suggestion: Use more specific patterns for find commands in the fmt target to avoid unnecessary processing of non-Go files and directories, improving the efficiency of the formatting process. [performance]
| fmt: $(GOFUMPT) $(GCI) | |
| go mod tidy | |
| find . -not \( -path "./.go" -prune \) -name "*.go" | xargs $(GOFUMPT) -l -w | |
| find . -not \( -path "./.go" -prune \) -name "*.go" | xargs $(GCI) write --skip-generated -s standard -s default -s localmodule | |
| fmt: $(GOFUMPT) $(GCI) | |
| go mod tidy | |
| find . -type f -name '*.go' ! -path './.go/*' | xargs $(GOFUMPT) -l -w | |
| find . -type f -name '*.go' ! -path './.go/*' | xargs $(GCI) write --skip-generated -s standard -s default -s localmodule |
There was a problem hiding this comment.
I'm not touching these unless I have to.
| resources: | ||
| - ../crd | ||
| - ../rbac | ||
| - ../manager |
There was a problem hiding this comment.
Suggestion: Replace 'resources' with 'bases' to maintain consistency with kustomize terminology and ensure correct processing of base resources. [maintainability]
| resources: | |
| - ../crd | |
| - ../rbac | |
| - ../manager | |
| bases: | |
| - ../crd | |
| - ../rbac | |
| - ../manager |
| enum: | ||
| - low | ||
| - Low |
There was a problem hiding this comment.
Suggestion: Consider using consistent capitalization for enum values to avoid potential issues with case-sensitive comparisons. The severity field has mixed capitalization for the same values. [best practice]
| enum: | |
| - low | |
| - Low | |
| severity: | |
| enum: | |
| - Low |
| enum: | ||
| - Inform | ||
| - inform | ||
| - Enforce | ||
| - enforce |
There was a problem hiding this comment.
Suggestion: The remediationAction enum values are duplicated with different cases (e.g., 'Inform' and 'inform'). It's recommended to keep enum values consistent to prevent confusion and potential bugs in case-sensitive environments. [bug]
| enum: | |
| - Inform | |
| - inform | |
| - Enforce | |
| - enforce | |
| remediationAction: | |
| enum: | |
| - Inform | |
| - Enforce |
| description: |- | ||
| NamespaceSelector indicates which namespaces on the cluster this policy | ||
| should apply to, when the policy applies to namespaced objects. |
There was a problem hiding this comment.
Suggestion: To improve the clarity and maintainability of the YAML, consider removing the redundant description fields that have been added to properties where the description merely repeats the property name or is overly verbose without adding clarity. [maintainability]
| description: |- | |
| NamespaceSelector indicates which namespaces on the cluster this policy | |
| should apply to, when the policy applies to namespaced objects. | |
| description: |- | |
| Indicates applicable namespaces for the policy. |
| description: |- | ||
| operator represents a key's relationship to a set of values. | ||
| Valid operators are In, NotIn, Exists and DoesNotExist. |
There was a problem hiding this comment.
Suggestion: The description for operator and values in matchExpressions is verbose and could be simplified to enhance readability and maintainability. [enhancement]
| description: |- | |
| operator represents a key's relationship to a set of values. | |
| Valid operators are In, NotIn, Exists and DoesNotExist. | |
| description: |- | |
| Defines the relationship between a key and values. Valid operators: In, NotIn, Exists, DoesNotExist. |
| github.com/gogo/protobuf v1.3.2 // indirect | ||
| github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect | ||
| github.com/golang/protobuf v1.5.3 // indirect | ||
| github.com/golang/protobuf v1.5.4 // indirect |
There was a problem hiding this comment.
Suggestion: Consider using consistent versioning for direct and indirect dependencies to avoid potential incompatibilities. For example, github.com/golang/protobuf and google.golang.org/protobuf are both updated, but they should be kept at compatible versions to prevent issues in serialization or compatibility with other libraries. [maintainability]
AI Suggestions ScoreSome suggestions were very similar, so I did not count them multiple times.
|
Versions updates, and various other housekeeping items.