Refactor and Simplify Codebase for Harbor Satellite #30
Conversation
- This commit adds simplified functions to handle file processing of url - Also handles both processing in separate functions Signed-off-by: bupd <[email protected]>
WalkthroughThis update focuses on enhancing the codebase of the Harbor Satellite by refactoring complex logic, improving error handling, and streamlining the flow for better clarity and maintainability. Key changes include modifying configuration flags, creating new modular functions in Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- config.toml (1 hunks)
- main.go (4 hunks)
Additional context used
golangci-lint
main.go
157-157: ineffectual assignment to fetcher
(ineffassign)
140-140: SA4009: argument fetcher is overwritten before first use
(staticcheck)
Additional comments not posted (4)
config.toml (2)
2-2: Correctness: Ensure the registry configuration change is intentional.The
bring_own_registryflag has been changed tofalse. Ensure this change aligns with the intended configuration.
8-9: Correctness: Verify the URL change from local to remote registry.The
url_or_filevalue has been updated to a remote URL. Ensure this change aligns with the intended configuration and update any dependent documentation or code accordingly.main.go (2)
34-34: Error Handling: Improve log message clarity.Consider providing more context in the log message to aid debugging.
- log.Fatalf("Error running satellite: %v", err) + log.Fatalf("Error running satellite in main function: %v", err)
164-164: Logging: Enhance log message clarity.Consider providing more context in the log message to aid debugging.
- fmt.Println("Input is a valid URL.") + fmt.Println("Successfully parsed input as a valid URL.")
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (3)
internal/store/file-fetch.go (2)
Line range hint
31-41: Improve error handling inFileImageListFetcher.Instead of returning
nilon error, return an error to provide better error handling.func FileImageListFetcher(relativePath string) (*FileImageList, error) { // Get the current working directory dir, err := os.Getwd() if err != nil { return nil, fmt.Errorf("error getting current directory: %w", err) } // Construct the absolute path from the relative path absPath := filepath.Join(dir, relativePath) return &FileImageList{ Path: absPath, }, nil }
Line range hint
43-62: Add more error context and logging inList.Adding more error context and logging can help with better debugging.
func (client *FileImageList) List(ctx context.Context) ([]Image, error) { var images []Image // Read the file data, err := os.ReadFile(client.Path) if err != nil { return nil, fmt.Errorf("error reading file %s: %w", client.Path, err) } var imageData ImageData // Parse the JSON data err = json.Unmarshal(data, &imageData) if err != nil { return nil, fmt.Errorf("error unmarshaling JSON data from file %s: %w", client.Path, err) } // Iterate over the repositories for _, repo := range imageData.Repositories { // Iterate over the images in each repository for _, image := range repo.Images { // Add each "name" value to the images slice images = append(images, Image{Name: image.Name}) } } return images, nil }internal/store/http-fetch.go (1)
74-82: Consider using a more secure method for handling credentials.Using environment variables for credentials is not secure. Consider using a more secure method, such as a secrets management service.
func createAuthHeader() (string, error) { // Use a secrets management service to retrieve credentials securely username := retrieveSecret("HARBOR_USERNAME") password := retrieveSecret("HARBOR_PASSWORD") if username == "" || password == "" { return "", fmt.Errorf("failed to retrieve credentials from secrets management service") } auth := base64.StdEncoding.EncodeToString([]byte(username + ":" + password)) return "Basic " + auth, nil }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- internal/store/file-fetch.go (1 hunks)
- internal/store/http-fetch.go (1 hunks)
- internal/store/in-memory-store.go (2 hunks)
- main.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- main.go
Additional comments not posted (6)
internal/store/file-fetch.go (2)
27-29: LGTM!The
SourceTypefunction is correct and straightforward.
Line range hint
64-66: Add a TODO comment for future implementation.Consider adding a TODO comment to indicate that this function needs to be implemented in the future.
func (client *FileImageList) GetDigest(ctx context.Context, tag string) (string, error) { // TODO: Implement GetDigest for FileImageList return "Not implemented yet", nil }internal/store/http-fetch.go (3)
29-31: LGTM!The
NewRemoteImageSourcefunction is correct and straightforward.
34-36: LGTM!The
SourceTypefunction is correct and straightforward.
123-126: LGTM!The
cleanImageReferencefunction is correct and straightforward.internal/store/in-memory-store.go (1)
31-33: LGTM!The
SourceTypefunction is correct and straightforward.
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
internal/store/in-memory-store.go (2)
Line range hint
244-244: Refactor complex logic for clarity and simplicity.The
checkImageAndDigestfunction contains complex logic that may benefit from refactoring.Consider breaking down the function into smaller, more manageable functions to enhance readability and maintainability.
Tools
golangci-lint
81-81: Error return value of
s.AddImageis not checked(errcheck)
98-98: Error return value of
s.RemoveImageis not checked(errcheck)
161-161: Error return value of
s.Removeis not checked(errcheck)
58-58: ineffectual assignment to err
(ineffassign)
60-60: ineffectual assignment to err
(ineffassign)
Line range hint
244-244: Enhance error handling and logging.The
GetLocalDigestfunction can benefit from additional error handling and logging for better debugging and maintainability.Consider adding more detailed error messages and logging the URL being fetched.
- return "", fmt.Errorf("failed to get digest using crane: %w", err) + return "", fmt.Errorf("failed to get digest using crane for URL %s: %w", url, err)Tools
golangci-lint
81-81: Error return value of
s.AddImageis not checked(errcheck)
98-98: Error return value of
s.RemoveImageis not checked(errcheck)
161-161: Error return value of
s.Removeis not checked(errcheck)
58-58: ineffectual assignment to err
(ineffassign)
60-60: ineffectual assignment to err
(ineffassign)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- internal/store/file-fetch.go (2 hunks)
- internal/store/http-fetch.go (1 hunks)
- internal/store/in-memory-store.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- internal/store/file-fetch.go
- internal/store/http-fetch.go
Additional context used
golangci-lint
internal/store/in-memory-store.go
81-81: Error return value of
s.AddImageis not checked(errcheck)
98-98: Error return value of
s.RemoveImageis not checked(errcheck)
161-161: Error return value of
s.Removeis not checked(errcheck)
58-58: ineffectual assignment to err
(ineffassign)
60-60: ineffectual assignment to err
(ineffassign)
Additional comments not posted (1)
internal/store/in-memory-store.go (1)
Line range hint
244-244: LGTM!The
removeHostNamefunction is correct and efficient.Tools
golangci-lint
81-81: Error return value of
s.AddImageis not checked(errcheck)
98-98: Error return value of
s.RemoveImageis not checked(errcheck)
161-161: Error return value of
s.Removeis not checked(errcheck)
58-58: ineffectual assignment to err
(ineffassign)
60-60: ineffectual assignment to err
(ineffassign)
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
internal/store/in-memory-store.go (1)
Line range hint
231-264: Check error return values and improve efficiency.
- The error return value of
s.Removeands.Addis not checked.- The error return value of
fmt.Errorfis not checked.- if err := s.Remove(context.Background(), storeDigest, storeImage); err != nil { - fmt.Errorf("Error: %w", err) - return false - } - s.Add(context.Background(), digest, image) + if err := s.Remove(context.Background(), storeDigest, storeImage); err != nil { + return false, fmt.Errorf("Error: %w", err) + } + if err := s.Add(context.Background(), digest, image); err != nil { + return false, err + }Tools
golangci-lint
258-258: Error return value of
fmt.Errorfis not checked(errcheck)
262-262: Error return value of
s.Addis not checked(errcheck)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- internal/store/in-memory-store.go (4 hunks)
Additional context used
golangci-lint
internal/store/in-memory-store.go
164-164: Error return value of
s.Removeis not checked(errcheck)
258-258: Error return value of
fmt.Errorfis not checked(errcheck)
262-262: Error return value of
s.Addis not checked(errcheck)
Additional comments not posted (5)
internal/store/in-memory-store.go (5)
38-44: LGTM!The
NewInMemoryStorefunction correctly initializes an in-memory store with anImageFetcher.
183-190: LGTM!The
getImageListfunction correctly converts the in-memory store to a list of Image structs.
Line range hint
192-205: LGTM!The
Addfunction correctly adds an image and its digest to the store.
Line range hint
212-225: LGTM!The
Removefunction correctly removes an image and its digest from the store.
Line range hint
266-280: LGTM!The
GetLocalDigestfunction correctly retrieves the digest of an image from a local registry using the crane library.Tools
golangci-lint
164-164: Error return value of
s.Removeis not checked(errcheck)
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- internal/replicate/replicate.go (2 hunks)
- internal/store/in-memory-store.go (4 hunks)
Additional context used
Learnings (1)
internal/store/in-memory-store.go (1)
Learnt from: bupd PR: container-registry/harbor-satellite#30 File: internal/store/in-memory-store.go:226-229 Timestamp: 2024-07-03T00:26:50.260Z Learning: The `RemoveImage` function in `internal/store/in-memory-store.go` should return an error to maintain consistency with other functions.
golangci-lint
internal/store/in-memory-store.go
101-101: Error return value of
s.RemoveImageis not checked(errcheck)
265-265: Error return value of
fmt.Errorfis not checked(errcheck)
269-269: Error return value of
s.Addis not checked(errcheck)
Additional comments not posted (26)
internal/replicate/replicate.go (15)
16-21: LGTM! The Replicator interface is well-defined.The interface methods
ReplicateandDeleteExtraImagesare clear and concise.
22-23: LGTM! The BasicReplicator struct is well-implemented.The struct correctly implements the Replicator interface.
25-28: LGTM! The ImageInfo struct is well-defined.The struct correctly holds the name of an image.
30-35: LGTM! The Repository struct is well-defined.The struct correctly holds the repository name and associated images.
36-41: LGTM! The RegistryInfo struct is well-defined.The struct correctly holds the registry URL and repositories information.
42-45: LGTM! The NewReplicator function is well-implemented.The function correctly returns a new BasicReplicator instance.
47-53: Verify the error handling in the Replicate function.The error handling for the
getPullSourcefunction should be verified to ensure all error cases are handled correctly.
56-63: LGTM! The stripPrefix function is well-implemented.The function correctly removes the prefix from the image name.
Line range hint
64-91: Verify the error handling in the DeleteExtraImages function.The error handling for the
crane.ListTagsandcrane.Deletefunctions should be verified to ensure all error cases are handled correctly.
93-110: Verify the error handling in the getPullSource function.The error handling for the
getFileInfofunction should be verified to ensure all error cases are handled correctly.
112-126: Verify the error handling in the getFileInfo function.The error handling for the
os.ReadFileandjson.Unmarshalfunctions should be verified to ensure all error cases are handled correctly.
128-150: Verify the error handling in the CopyImage function.The error handling for the
crane.Pullandcrane.Pushfunctions should be verified to ensure all error cases are handled correctly.
156-163: LGTM! The removeHostName function is well-implemented.The function correctly removes the hostname from the image name.
165-172: LGTM! The getEnvRegistryPath function is well-implemented.The function correctly constructs the local registry URL from environment variables.
174-181: Verify the error handling in the getWorkingDir function.The error handling for the
os.Getwdfunction should be verified to ensure all error cases are handled correctly.internal/store/in-memory-store.go (11)
12-15: LGTM! The Image struct is well-defined.The struct correctly represents a container image with its digest and name.
18-23: LGTM! The inMemoryStore struct is well-defined.The struct correctly stores images in memory and uses an ImageFetcher to manage images.
24-30: LGTM! The Storer interface is well-defined.The interface methods for listing, adding, and removing images are clear and concise.
31-37: LGTM! The ImageFetcher interface is well-defined.The interface methods for listing images, getting digests, and retrieving the source type are clear and concise.
38-45: LGTM! The NewInMemoryStore function is well-implemented.The function correctly returns a new in-memory store instance with the given ImageFetcher.
46-75: Verify the error handling in the List function.The error handling for the
fetcher.List,handleFileSource, andhandleRemoteSourcefunctions should be verified to ensure all error cases are handled correctly.
77-119: Verify the error handling in the handleFileSource function.The error handling for the
AddImageandRemoveImagefunctions should be verified to ensure all error cases are handled correctly.Tools
golangci-lint
101-101: Error return value of
s.RemoveImageis not checked(errcheck)
121-184: Verify the error handling in the handleRemoteSource function.The error handling for the
fetcher.GetDigest,Remove, andAddfunctions should be verified to ensure all error cases are handled correctly.
186-193: LGTM! The getImageList function is well-implemented.The function correctly converts the in-memory store to a list of Image structs.
Line range hint
194-208: Verify the error handling in the Add function.The error handling for the
Addfunction should be verified to ensure all error cases are handled correctly.Tools
golangci-lint
101-101: Error return value of
s.RemoveImageis not checked(errcheck)
Line range hint
215-228: Verify the error handling in the Remove function.The error handling for the
Removefunction should be verified to ensure all error cases are handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- internal/store/in-memory-store.go (4 hunks)
Additional context used
Learnings (1)
internal/store/in-memory-store.go (1)
Learnt from: bupd PR: container-registry/harbor-satellite#30 File: internal/store/in-memory-store.go:226-229 Timestamp: 2024-07-03T00:26:50.260Z Learning: The `RemoveImage` function in `internal/store/in-memory-store.go` should return an error to maintain consistency with other functions.
golangci-lint
internal/store/in-memory-store.go
101-101: Error return value of
s.RemoveImageis not checked(errcheck)
272-272: Error return value of
fmt.Errorfis not checked(errcheck)
276-276: Error return value of
s.Addis not checked(errcheck)
Additional comments not posted (2)
internal/store/in-memory-store.go (2)
35-35: LGTM! The renaming ofType()toSourceType()improves clarity.The change aligns with naming conventions and improves readability.
54-66: LGTM! Improved error handling and use ofSourceType.The changes enhance the function's robustness and clarity.
|
@Vad1mo ready for review! |
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- config.toml (1 hunks)
- internal/replicate/replicate.go (4 hunks)
- internal/satellite/satellite.go (3 hunks)
- internal/store/http-fetch.go (2 hunks)
- internal/store/in-memory-store.go (5 hunks)
Files skipped from review as they are similar to previous changes (4)
- config.toml
- internal/replicate/replicate.go
- internal/store/http-fetch.go
- internal/store/in-memory-store.go
Additional comments not posted (3)
internal/satellite/satellite.go (3)
28-28: Ensure consistent error handling.The added error handling is consistent with the rest of the method. Good practice to return the error immediately.
40-43: LGTM! Error handling added forDeleteExtraImages.The added error handling for
s.replicator.DeleteExtraImagesfollows the same pattern as other error checks in the method.
69-72: LGTM! Error handling added forDeleteExtraImages.The added error handling for
s.replicator.DeleteExtraImagesfollows the same pattern as other error checks in the method.
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- config.toml (1 hunks)
- internal/replicate/replicate.go (4 hunks)
- internal/satellite/satellite.go (3 hunks)
- internal/store/file-fetch.go (2 hunks)
- internal/store/http-fetch.go (2 hunks)
- internal/store/in-memory-store.go (6 hunks)
Files skipped from review as they are similar to previous changes (6)
- config.toml
- internal/replicate/replicate.go
- internal/satellite/satellite.go
- internal/store/file-fetch.go
- internal/store/http-fetch.go
- internal/store/in-memory-store.go
|
Ready to merge! |
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/e2e/satellite_test.go (2 hunks)
Additional comments not posted (2)
test/e2e/satellite_test.go (2)
110-110: Verify the availability of the new image tag.Ensure that
busybox:1.36is a valid and available image tag.
154-154: Verify the availability of the new image tag.Ensure that
busybox:1.36is a valid and available image tag.
Overview:
The current Harbor Satellite codebase is characterized by complex logic and a lack of clear flow, making it challenging for future contributors to understand its functionality and make necessary modifications. The intricacies of the existing code structure hinder productivity and increase the likelihood of introducing errors during updates or enhancements.
Changes Made:
Modularization:
Refactored large, monolithic functions into smaller, well-defined functions to improve clarity and reusability.
Improved Naming Conventions:
Updated variable and function names to be more descriptive and reflective of their roles in the code.
Simplified Logic:
Rewrote complex conditional statements and loops to streamline the control flow.
Enhanced Documentation:
Added detailed inline comments and created a separate document outlining the new code flow and logic.
Expected Outcome:
Fixes: #29
Summary by CodeRabbit
New Features
Bug Fixes
Satellitecomponent to prevent failures during replication.Refactor
Tests