Update and cleanup to use modern Go language features#115
Conversation
There was a problem hiding this comment.
Pull request overview
Modernizes the codebase to use newer Go language/library features and removes legacy shims, improving readability and reducing custom utility code.
Changes:
- Replaces
interface{}withanyacross pullspec, utils, and resolver code paths. - Adopts standard library helpers (e.g.,
slices.Contains,slices.Reverse) and Go 1.22+ loop semantics to simplify code. - Removes
io/ioutilusage in favor ofosequivalents in tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/pullspec/pullspec.go | Migrates to any, replaces custom slice helpers with slices, and simplifies loop constructs. |
| pkg/pullspec/heuristic.go | Updates template helper signature and map type to any. |
| pkg/imageresolver/skopeo.go | Switches JSON map type to any and uses integer range for retry looping. |
| pkg/imageresolver/script_test.go | Replaces ioutil temp dir + file writes with os equivalents. |
| internal/utils/vars_test.go | Replaces ioutil temp file + writes with os equivalents. |
| internal/utils/lens_test.go | Updates test fixtures/expectations to any-typed containers. |
| internal/utils/lens.go | Migrates to any and simplifies internal slice/function-building logic. |
| internal/utils/error.go | Updates NewError variadic args to ...any. |
| cmd/pinning/pinning_test.go | Replaces ioutil calls and updates JSON container types to any. |
Comments suppressed due to low confidence (2)
pkg/pullspec/pullspec.go:556
- Log message typo: "relateImage" reads like a misspelling of "relatedImage"/"related image". Consider correcting it to keep logs searchable/consistent.
log.Printf("%s - Set relateImage %s (from %s): %s\n", csv.path, p.Name(), p.String(), p.Image())
relatedImages = append(relatedImages, p.AsYamlObject())
cmd/pinning/pinning_test.go:59
- Test setup ignores errors from os.MkdirTemp, os.WriteFile, and imageresolver.GetResolver. These failures can lead to confusing downstream test errors; please check/expect these errors explicitly.
dir, _ = os.MkdirTemp("", "script")
manifestDir, _ = os.MkdirTemp("", "pinning_test_")
csvFilePath = filepath.Join(manifestDir, "clusterserviceversion.yaml")
resolverScript := filepath.Join(dir, "resolver.sh")
os.WriteFile(resolverScript, []byte(`#!/bin/bash
if [ "$1" == "registry.example.com/eggs:9.8" ]; then
echo -n "2"
exit 0
fi
if [ "$1" == "registry.example.com/maps/spam-operator:1.2" ]; then
echo -n "1"
exit 0
fi
exit 1
`), 0700)
resolver, _ = imageresolver.GetResolver(imageresolver.ResolverScript, map[string]string{
"path": resolverScript,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
35a4171 to
59113c5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cmd/pinning/pinning_test.go:55
- The errors from os.MkdirTemp and os.WriteFile are ignored here. In tests, this can hide setup failures and lead to confusing downstream errors; please capture the returned errors and assert success (e.g., with Expect(err).To(Succeed())).
dir, _ = os.MkdirTemp("", "script")
manifestDir, _ = os.MkdirTemp("", "pinning_test_")
csvFilePath = filepath.Join(manifestDir, "clusterserviceversion.yaml")
resolverScript := filepath.Join(dir, "resolver.sh")
os.WriteFile(resolverScript, []byte(`#!/bin/bash
if [ "$1" == "registry.example.com/eggs:9.8" ]; then
echo -n "2"
exit 0
fi
if [ "$1" == "registry.example.com/maps/spam-operator:1.2" ]; then
echo -n "1"
exit 0
fi
exit 1
`), 0700)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
59113c5 to
7b3ef9f
Compare
Signed-off-by: Caleb Xu <caxu@redhat.com>
Signed-off-by: Caleb Xu <caxu@redhat.com>
Signed-off-by: Caleb Xu <caxu@redhat.com>
Signed-off-by: Caleb Xu <caxu@redhat.com>
Signed-off-by: Caleb Xu <caxu@redhat.com>
7b3ef9f to
5f819e1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
It may be easier to review each commit in this PR individually. This PR covers:
anyinstead ofinterface{}(more readable/intentional)sliceslibrary to remove custom shim types that provide a customContainsimplementationio/ioutilpackage with modern equivalents