chore: Misc cleanup for Container Image Resolver #499
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR performs miscellaneous cleanup and refactoring for the container image resolution logic.
- Updated the CLI flag description for image pulling behavior.
- Refactored and simplified the container image resolution workflows, reorganizing imports and error handling.
- Renamed
imageStrtoimageRef, switched config to a value type, and updated tests to use the newisLocalFileflag.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scan.go | Updated the description of the --image-no-remote flag |
| pkg/readers/container_image_resolve_workflow.go | Reorganized imports and rewrote image resolution workflows |
| pkg/readers/container_image_reader.go | Renamed fields, switched config to a value type, and improved errors |
| pkg/readers/container_image_reader_test.go | Adjusted tests to assert isLocalFile instead of using checkPathExists |
Comments suppressed due to low confidence (3)
pkg/readers/container_image_resolve_workflow.go:194
- The loop in
getScalibrImagereturns on the first non-unsupported error or non-nil image, preventing later workflows from running. Instead, only return on a valid image or an irrecoverable error, and continue onimageResolverUnsupportedError.
workflow := []imageResolutionWorkflowFunc{
pkg/readers/container_image_reader_test.go:77
- [nitpick] The test name
TestCheckPathExistsno longer matches its behavior (it now assertsisLocalFile). Consider renaming it to reflect that it's testing local-file detection.
func TestCheckPathExists(t *testing.T) {
pkg/readers/container_image_resolve_workflow.go:20
imageFromLocalDockerImageCatalogcontains significant logic branches (skipping local files, listing images, saving, temp files). Consider adding dedicated unit tests to cover success, unsupported, and error paths.
func (c containerImageReader) imageFromLocalDockerImageCatalog(ctx context.Context) (*scalibrlayerimage.Image, error) {
| // create tem directory in /tmp for storing `POSIX tar archive` in file | ||
| tempTarFile, err := os.CreateTemp(os.TempDir(), "image-data-*.tar") | ||
|
|
||
| image, err := c.imageFromLocalTarFile(ctx) |
There was a problem hiding this comment.
Calling imageFromLocalTarFile here ignores the temporary tar file just created; it will load the original imageRef instead of the saved tarball. Replace with scalibrlayerimage.FromTarball(tempTarFile.Name(), ...) or adjust the helper to accept a file path.
Suggested change
| image, err := c.imageFromLocalTarFile(ctx) | |
| image, err := scalibrlayerimage.FromTarball(tempTarFile.Name(), scalibrlayerimage.DefaultConfig()) |
Copilot uses AI. Check for mistakes.
KunalSin9h
approved these changes
May 29, 2025
abhisek
added a commit
that referenced
this pull request
May 29, 2025
* refactor: convert current image resolution into workflow pattern * feat: image from local tarball folder * feat: image from local docker catalog * refacot: decompose docker image catalog resolver into multiple functions * refactor: using utilit function for logging and return error * feat: remove tem tar dir after image obj is created * refactor: handle empty nil error in log and error funciton * removed local image not supported test * fix: error fmt.Errorf with non-constant values * go mod tidy * fix: linter unreachable code * refactor: removed unwanted parameter * test: added scenerio for different image scan operations * fix: added scenario into all.sh * fix: missed .sh extension for one entry * fix: test bad file path for local tar scan * remvoed logger and error combined function * refactor: using context form top of tree * feat: using custom error for image resolution unsupported * refactor: returning unsupported workflow error for each docker api fail * feat: added no-remote flag for disable remote fetch * feat: --image-no-remote * fix: test, creating temp files witn \/ causing issue * chore: Misc cleanup for Container Image Resolver (#499) * chore: Misc cleanup * fix: Bug with docker image resolver * fix: Error msg * chore: Improve debug logging for docker enumeration * chore: Improve debug logging for docker enumeration --------- Co-authored-by: Abhisek Datta <abhisek.datta@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.