Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple zot registry from satellite #23

Merged
merged 15 commits into from
Jun 25, 2024
Merged

Decouple zot registry from satellite #23

merged 15 commits into from
Jun 25, 2024

Conversation

OneFlyingBanana
Copy link
Collaborator

@OneFlyingBanana OneFlyingBanana commented May 28, 2024

Added makefile to build and run zot and zli depending on host's OS and architecture (MacOS/Linux and ARM/X86)
Added source code for zot/zli as tar.gz
Removed double execution of satellite in main.go
Removed launch of zot in satellite.go

Summary by CodeRabbit

  • New Features

    • Introduced Harbor Satellite, an extension for decentralized registries at edge locations with local storage of images.
    • Added configuration options for using a custom registry or built-in Zot registry.
  • Bug Fixes

    • Minor formatting correction in the .gitignore file.
  • Documentation

    • Expanded README with detailed information on Harbor Satellite's purpose, architecture, and use cases.
    • Updated authors and provided community support channels.
  • Chores

    • Updated .gitignore to exclude the .env file from version control.

Included 2 possible implementations :
- using sync.Map optimised for high concurrency read/writes
- using base map with RWMutex lock/unlock better performing for low-concurrency read/writes
Removed concurrency handling
Added hash map comparison of local + fetched images
Added cli to ask user to choose between remote fetcher and file fetcher
Added barebones files of remote fetcher and file fetcher with test image lists
Modified image list comparison to work via hash map comparison
Setup of skeleton files for file fetcher and http fetcher
Updated user feedback to be more concise
Moved List call in-memory-store to not proc when hashes are equal
Reworked main's user interface to automatically recognise and validate URL or filepath
Reworked http-fetcher to accept  full URLs
Added images.json test file
http-fetcher :
Now using correct Harbor v2 API
Local image list is based on tag list retrieved via API
file-fetcher :
Created JSON struct with name, digest and repository URL
This data, with optional tag name, can be used to make docker pull commands using only url + digest
Features :
- Check user input for local filepath or URL
- Retrieve list of images via URL or local file and store them in-memory
- Check for changes in remote URL or local file and update in-memory store accordingly
- Launch local Zot registry
- Replicate images using in-memory store list to local registry

Main missing features/elements :
- Delete images from local repository if removed from in-memory store
- Unit + integration tests
- Optimization and refactoring
Added makefile to build and run zot and zli depending on host's OS and architecture (MacOS/Linux and ARM/Intel)
Added source code for zot/zli as tar.gz
Removed double execution of satellite in main.go
Removed launch of zot in satellite.go
Copy link

coderabbitai bot commented May 28, 2024

Warning

Review failed

The pull request is closed.

Walkthrough

The recent updates to the Harbor Satellite system enhance functionality across several key areas. Major additions include handling configuration through the .env file, implementing new registry and replicator types, and improving the file fetching mechanism. Enhancements to various store functionalities, together with changes to the main instantiation logic, form a robust baseline for managing image replication and storage, supporting the decentralized registry architecture at edge locations.

Changes

Files Change Summary
.env Updated for HARBOR_USERNAME, HARBOR_PASSWORD, and ZOT_URL configurations.
.gitignore Added .env to ignore list and fixed .DS_Store formatting issue.
image-list/images.json Included details about registry URL, repositories, and associated images.
internal/replicate/replicate.go Added new types (ImageInfo, Repository, RegistryInfo); modified Replicate; introduced functions for handling image replication.
internal/satellite/satellite.go Adjusted tick rate, replication logic, and error handling; added debugging print statement.
internal/store/file-fetch.go Introduced functionality for fetching and parsing image data from a file.
internal/store/http-fetch.go Added support for fetching image lists and digests using HTTP.
internal/store/in-memory-store.go Added in-memory store logic including methods for listing, adding, and removing images; introduced support for different fetcher types.
main.go Enhanced main function with user input handling for URLs/paths; instantiated new components and loaded .env configurations.
registry/config.json Introduced registry system configuration settings.
registry/makefile Added targets for compiling, running, cleaning, and managing binaries for different OS/architecture combinations.
README.md Expanded documentation on Harbor Satellite, its architecture and implementation; added author contributions and community support comments.
config.toml Introduced options for using built-in or custom registries, specifying URLs/paths, and demo URLs.
go.mod Updated module name, dependency versions, and added new dependencies.
registry/launch-registry.go Introduced LaunchRegistry() function to start the registry server with configurations from config.json.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Main
    participant EnvLoader
    participant Store
    participant Replicator
    participant Registry

    User->>Main: Start application
    Main->>EnvLoader: Load .env
    EnvLoader-->>Main: Configurations loaded
    Main->>Store: Initialize store
    Main->>Replicator: Initialize replicator
    Main->>Registry: Launch registry
    Registry-->>Main: Registry launched
    User->>Main: Request image replication
    Main->>Replicator: Replicate requested image
    Replicator->>Store: Fetch image data
    Store-->>Replicator: Image data fetched
    Replicator-->>User: Image replicated
Loading

Poem

In lines of code, changes bloom,
For images, now there's room. 🌌
With configs set and tasks aligned,
Edge devices will not lag behind.
Replicate and store anew,
Harbor Satellite, we trust in you! 🚀


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Vad1mo Vad1mo marked this pull request as ready for review May 28, 2024 12:39
@Vad1mo
Copy link
Contributor

Vad1mo commented May 28, 2024

@coderabbitai full review

Copy link

coderabbitai bot commented May 28, 2024

Actions Performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Outside diff range and nitpick comments (1)
internal/store/http-fetch.go (1)

34-83: The List method is well-implemented with comprehensive handling of HTTP requests, authentication, and JSON parsing. Consider adding more detailed logging for production environments to aid in troubleshooting.

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between a5d592f and 70e2ed7.
Files ignored due to path filters (4)
  • .DS_Store is excluded by !**/.DS_Store
  • go.mod is excluded by !**/*.mod
  • go.sum is excluded by !**/*.sum
  • registry/zot-2.0.4.tar.gz is excluded by !**/*.gz
Files selected for processing (11)
  • .env (1 hunks)
  • .gitignore (1 hunks)
  • image-list/images.json (1 hunks)
  • internal/replicate/replicate.go (1 hunks)
  • internal/satellite/satellite.go (3 hunks)
  • internal/store/file-fetch.go (1 hunks)
  • internal/store/http-fetch.go (1 hunks)
  • internal/store/in-memory-store.go (1 hunks)
  • main.go (2 hunks)
  • registry/config.json (1 hunks)
  • registry/makefile (1 hunks)
Files skipped from review due to trivial changes (4)
  • .env
  • .gitignore
  • image-list/images.json
  • registry/config.json
Additional comments not posted (13)
internal/satellite/satellite.go (2)

17-22: Constructor implementation looks good and correctly initializes the Satellite instance.


37-44: Handling of no images to replicate and the debugging print statement are appropriate for testing. Ensure to remove or adjust the debugging output in production.

Also applies to: 49-49

internal/store/file-fetch.go (2)

31-44: The implementation of FileImageListFetcher correctly handles the construction of the absolute path and returns a properly initialized FileImageList instance.


47-73: The List method is well-implemented with proper error handling and JSON parsing to construct the list of images.

registry/makefile (2)

8-31: The build target is well-structured with appropriate handling for different OS and architecture combinations. The use of conditional statements and variable assignments is correctly implemented.


58-76: The run target is correctly implemented with proper checks for OS and architecture, ensuring the appropriate binary is executed. The error handling for unsupported configurations is also well-handled.

internal/store/http-fetch.go (1)

85-127: The GetDigest method is correctly implemented, effectively fetching and parsing the manifest to extract the digest. Error handling is appropriately managed.

internal/replicate/replicate.go (1)

53-71: The getPullSource method effectively constructs the source URL or file path based on the input and environment variables. Error handling and logging are appropriately managed.

main.go (2)

71-140: The handling of user input is robust, with comprehensive checks for URL validity and file existence. The use of environment variables to decompose and store URL components is a clever approach.


142-152: The instantiation of components and the loading of the .env file are correctly implemented. This setup ensures that all necessary configurations are loaded before the components are used.

internal/store/in-memory-store.go (3)

37-41: The constructor function NewInMemoryStore correctly initializes an inMemoryStore instance. Good use of the map initialization.


172-182: The Remove method handles the removal of images well. It's good to see defensive programming with checks for existence before removal.


234-276: The GetLocalDigest function constructs a URL and fetches a digest. However, the URL construction process is not robust against malformed inputs. Ensure that USER_INPUT is sanitized before use.

Comment on lines 44 to 148
imageList, err := s.fetcher.List(ctx)
if err != nil {
return nil, err
}

// Handle File and Remote fetcher types differently
switch s.fetcher.Type() {
case "File":
for _, img := range imageList {
// Check if the image already exists in the store
if _, exists := s.images[img.Name]; !exists {
// Add the image to the store
s.AddImage(ctx, img.Name)
change = true
} else {
fmt.Printf("Image %s already exists in the store\n", img.Name)
}
}

// Iterate over s.images and remove any image that is not found in imageList
for image := range s.images {
found := false
for _, img := range imageList {
if img.Name == image {
found = true
break
}
}
if !found {
s.RemoveImage(ctx, image)
change = true
}
}

// Print out the entire store for debugging purposes
fmt.Println("Current store:")
for image := range s.images {
fmt.Printf("Image: %s\n", image)
}

case "Remote":
// Trim the imageList elements to remove the project name from the image reference
for i, img := range imageList {
parts := strings.Split(img.Name, "/")
if len(parts) > 1 {
// Take the second part as the new Reference
imageList[i].Name = parts[1]
}
}
// iterate over imageList and call GetDigest for each tag
for _, img := range imageList {
// Split the image reference to get the tag
tagParts := strings.Split(img.Name, ":")
// Check if there is a tag part, min length is 1 char
if len(tagParts) < 2 {
fmt.Println("No tag part found in the image reference")
}
// Use the last part as the tag
tag := tagParts[len(tagParts)-1]
// Get the digest for the tag
digest, err := s.fetcher.GetDigest(ctx, tag)
if err != nil {
return nil, err
}

// Check if the image exists and matches the digest
if !(s.checkImageAndDigest(digest, img.Name)) {
change = true
}

}

// Create imageMap filled with all images from remote imageList
imageMap := make(map[string]bool)
for _, img := range imageList {
imageMap[img.Name] = true
}

// Iterate over in memory store and remove any image that is not found in imageMap
for digest, image := range s.images {
if _, exists := imageMap[image]; !exists {
s.Remove(ctx, digest, image)
change = true
}
}
// Print out the entire store for debugging purposes
fmt.Println("Current store:")
for digest, imageRef := range s.images {
fmt.Printf("Digest: %s, Image: %s\n", digest, imageRef)
}

}
if change {
fmt.Println("Changes detected in the store")
change = false
return imageList, nil
} else {
fmt.Println("No changes detected in the store")
return nil, nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The List method is quite complex and handles different fetcher types. Consider refactoring to improve readability and maintainability:

  1. Split the handling of "File" and "Remote" fetcher types into separate methods.
  2. Use early returns to reduce nesting and improve clarity.

Comment on lines 192 to 232
// checkImageAndDigest checks if the image exists in the store and if the digest matches the image reference
func (s *inMemoryStore) checkImageAndDigest(digest string, image string) bool {

// Check if the received image exists in the store
for storeDigest, storeImage := range s.images {
if storeImage == image {
// Image exists, now check if the digest matches
if storeDigest == digest {
// Digest exists and matches the current image's
fmt.Printf("Digest for image %s exists in the store and matches remote digest\n", image)
// TODO: Add support for multiple repositories
// Remove what comes before the ":" in image and set it to tag cariable
tag := strings.Split(image, ":")[1]
localRegistryDigest, err := GetLocalDigest(context.Background(), tag)
if err != nil {
fmt.Println("Error getting digest from local registry:", err)
return false
} else {
if digest == localRegistryDigest {
fmt.Printf("Digest for image %s in the store matches the local registry\n", image)
return true
} else {
fmt.Printf("Digest %s for image %s in the store does not match the local registry digest %s \n", storeDigest, image, localRegistryDigest)
return false
}
}
} else {
// Digest exists but does not match the current image reference
fmt.Printf("Digest for image %s exists in the store but does not match the current image reference\n", image)
s.Remove(context.Background(), storeDigest, storeImage)
s.Add(context.Background(), digest, image)
return false
}
}
}

// If the image doesn't exist in the store, add it
fmt.Printf("Image \"%s\" does not exist in the store\n", image)
s.Add(context.Background(), digest, image)
return false
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checkImageAndDigest method is overly complex and does too much. It checks for existence, matches digests, and even modifies the store. Consider breaking this method into smaller, more focused methods.

Comment on lines 25 to 26
// Temporarily set to faster tick rate for testing purposes
ticker := time.NewTicker(2 * time.Second)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making the tick rate configurable via an environment variable or a parameter to support different environments without changing the code.

Also applies to: 49-49

Comment on lines +75 to +77
func (client *FileImageList) GetDigest(ctx context.Context, tag string) (string, error) {
return "Not implemented yet", nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GetDigest method is not implemented yet. Consider implementing it or creating a tracking issue to ensure it's addressed.

Would you like me to help by implementing this method or should I open a GitHub issue to track this task?

Comment on lines 39 to 51
func (r *BasicReplicator) Replicate(ctx context.Context, image string) error {

// TODO: Implement deletion of images from the local registry that are not present in the source registry
// Probably use crane.Catalog to get a list of images in the local registry and compare to incoming image list

source := getPullSource(image)
fmt.Println("Source:", source)

if source != "" {
CopyImage(source)
}
return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Replicate method is well-structured with clear logging and conditional execution. Consider addressing the TODO regarding the deletion of images not present in the source registry.

Would you like me to help by implementing the deletion logic or should I open a GitHub issue to track this task?

Now correctly passing the ZOT_BINARY and ZLI_BINARY from build to _build
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 70e2ed7 and a4189aa.
Files selected for processing (1)
  • registry/makefile (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • registry/makefile

Switched to using crane.Digest to retrieve an image's digest. This simplifies the code and ensures consistency of all stored digests.
These digests can also be used to pull their images, as opposed to the manifest digests retrieved via the v2 API
Updated the README.md to more closely resemble the proposal in goharbor's community repo.
Updated MAKEFILE
Updated images.json to use a correct digest
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 4, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a4189aa and 0c459a8.

Files selected for processing (6)
  • README.md (2 hunks)
  • image-list/images.json (1 hunks)
  • internal/replicate/replicate.go (1 hunks)
  • internal/store/http-fetch.go (1 hunks)
  • internal/store/in-memory-store.go (1 hunks)
  • registry/makefile (1 hunks)
Files not reviewed due to errors (4)
  • internal/store/http-fetch.go (no review received)
  • internal/replicate/replicate.go (no review received)
  • internal/store/in-memory-store.go (no review received)
  • README.md (no review received)
Files skipped from review due to trivial changes (1)
  • image-list/images.json
Additional context used
golangci-lint
internal/store/http-fetch.go

36-36: undefined: Image (typecheck)


75-75: undefined: Image (typecheck)


79-79: undefined: Image (typecheck)

LanguageTool
README.md

[style] ~10-~10: The phrase ‘in recent years’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others. (IN_RECENT_STYLE)
Context: ...egistry is unavailable. ## Background In recent years, containers have extended beyond their ...


[duplication] ~31-~31: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...ries is not operationally feasible with Harbor - Harbor would be too similar to a simple regist...


[grammar] ~34-~34: The usual collocation for “independently” is “of”, not “from”. Did you mean “independently of”? (INDEPENDENTLY_FROM_OF)
Context: ...ht and will be able to keep functioning independently from Harbor instances. ## Compatibility Co...


[grammar] ~60-~60: Uncountable nouns are usually not used with an indefinite article. Use simply “direct access”. (A_UNCOUNTABLE)
Context: ...ull required images directly from it. _(A direct access from edge device to the remote registry...


[uncategorized] ~64-~64: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text. (EN_WORD_COHERENCY)
Context: ...onnnectivity, these devices need to run containerised images but cannot pull from a central H...


[uncategorized] ~64-~64: It appears that hyphens are missing in the adjective “up-to-date”. (UP_TO_DATE_HYPHEN)
Context: ... central Harbor registry and thus store up to date images locally._ ![Use Case #1](docs/i...


[uncategorized] ~73-~73: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text. (EN_WORD_COHERENCY)
Context: ...nt amount of IoT devices needing to run containerised applications, a single local registry i...


[uncategorized] ~73-~73: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case. (AMOUNTOF_TO_NUMBEROF)
Context: ...ght not be able to handle the increased amount of demands from edge devices. The solut...


[style] ~73-~73: As a shorter alternative for ‘able to’, consider using “can”. (BE_ABLE_TO)
Context: ...several registries to several nodes who are able to automatically replicate images across e...


[style] ~79-~79: Consider using more formal and concise wording here. (RESPONSIBLE_FOR)
Context: ... The stateless satellite component will be in charge of configuring the local OCI compliant reg...


[style] ~92-~92: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...imize pull requests to the registry. 3. By directly referencing the registry. ...


[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...directly referencing** the registry. 4. By directly referencing the registry a...

Additional comments not posted (4)
registry/makefile (4)

9-28: The conditional build logic for different OS and architectures looks good.


54-72: The run logic for different OS and architectures is consistent and well-handled.


74-76: The cleanup commands are appropriate and should effectively remove the necessary files and directories.


78-79: The empty target correctly handles the removal of the root directory.

Comment on lines 30 to 52
_build:
@test -d $(BIN) || mkdir -p $(BIN)
@if [ ! -f $(BIN)/$(ZOT_BINARY) ]; then \
tar -xzvf $(SOURCE_FILE); \
cd $(EXTRACTED_DIR) && make binary ; \
cp $(BIN)/$(ZOT_BINARY) ../$(BIN)/$(ZOT_BINARY); \
cp ../config.json ../$(BIN)/config.json; \
rm -rf ../$(EXTRACTED_DIR); \
echo "---------------------------------------------------------------------------" ; \
else \
echo "$(ZOT_BINARY) already exists, skipping build."; \
echo "---------------------------------------------------------------------------" ; \
fi

@if [ ! -f $(BIN)/$(ZLI_BINARY) ]; then \
tar -xzvf $(SOURCE_FILE); \
cd $(EXTRACTED_DIR) && make cli; \
cp $(BIN)/$(ZLI_BINARY) ../$(BIN)/$(ZLI_BINARY); \
cp ../config.json ../$(BIN)/config.json; \
rm -rf ../$(EXTRACTED_DIR); \
else \
echo "$(ZLI_BINARY) already exists, skipping build."; \
fi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a cleanup step in case of build failure to ensure no residual files are left.

+ trap "rm -rf $(EXTRACTED_DIR)" EXIT
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
_build:
@test -d $(BIN) || mkdir -p $(BIN)
@if [ ! -f $(BIN)/$(ZOT_BINARY) ]; then \
tar -xzvf $(SOURCE_FILE); \
cd $(EXTRACTED_DIR) && make binary ; \
cp $(BIN)/$(ZOT_BINARY) ../$(BIN)/$(ZOT_BINARY); \
cp ../config.json ../$(BIN)/config.json; \
rm -rf ../$(EXTRACTED_DIR); \
echo "---------------------------------------------------------------------------" ; \
else \
echo "$(ZOT_BINARY) already exists, skipping build."; \
echo "---------------------------------------------------------------------------" ; \
fi
@if [ ! -f $(BIN)/$(ZLI_BINARY) ]; then \
tar -xzvf $(SOURCE_FILE); \
cd $(EXTRACTED_DIR) && make cli; \
cp $(BIN)/$(ZLI_BINARY) ../$(BIN)/$(ZLI_BINARY); \
cp ../config.json ../$(BIN)/config.json; \
rm -rf ../$(EXTRACTED_DIR); \
else \
echo "$(ZLI_BINARY) already exists, skipping build."; \
fi
_build:
@test -d $(BIN) || mkdir -p $(BIN)
@if [ ! -f $(BIN)/$(ZOT_BINARY) ]; then \
trap "rm -rf $(EXTRACTED_DIR)" EXIT; \
tar -xzvf $(SOURCE_FILE); \
cd $(EXTRACTED_DIR) && make binary ; \
cp $(BIN)/$(ZOT_BINARY) ../$(BIN)/$(ZOT_BINARY); \
cp ../config.json ../$(BIN)/config.json; \
rm -rf ../$(EXTRACTED_DIR); \
echo "---------------------------------------------------------------------------" ; \
else \
echo "$(ZOT_BINARY) already exists, skipping build."; \
echo "---------------------------------------------------------------------------" ; \
fi
@if [ ! -f $(BIN)/$(ZLI_BINARY) ]; then \
trap "rm -rf $(EXTRACTED_DIR)" EXIT; \
tar -xzvf $(SOURCE_FILE); \
cd $(EXTRACTED_DIR) && make cli; \
cp $(BIN)/$(ZLI_BINARY) ../$(BIN)/$(ZLI_BINARY); \
cp ../config.json ../$(BIN)/config.json; \
rm -rf ../$(EXTRACTED_DIR); \
else \
echo "$(ZLI_BINARY) already exists, skipping build."; \
fi

Satellite configuration now uses config.toml and Viper instead of CLI interaction with user
Registry now incorporated via package and is optional
Satellite can now delete images in local registry to match in-memory store
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Outside diff range and nitpick comments (1)
main.go (1)

Line range hint 41-164: The run function is very complex and lengthy, consider refactoring to improve maintainability and readability. Specifically, the nested blocks in the if bringOwnRegistry condition could be simplified or broken into smaller functions.

Tools
golangci-lint

84-84: if bringOwnRegistry has complex nested blocks (complexity: 5) (nestif)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0c459a8 and f0f4943.

Files ignored due to path filters (4)
  • .DS_Store is excluded by !**/.DS_Store
  • docs/.DS_Store is excluded by !**/.DS_Store
  • go.sum is excluded by !**/*.sum
  • registry/.DS_Store is excluded by !**/.DS_Store
Files selected for processing (11)
  • .env (1 hunks)
  • README.md (2 hunks)
  • config.toml (1 hunks)
  • go.mod (1 hunks)
  • internal/replicate/replicate.go (1 hunks)
  • internal/satellite/satellite.go (2 hunks)
  • internal/store/http-fetch.go (1 hunks)
  • internal/store/in-memory-store.go (1 hunks)
  • main.go (3 hunks)
  • registry/config.json (1 hunks)
  • registry/launch-registry.go (1 hunks)
Files not summarized due to errors (1)
  • internal/satellite/satellite.go: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (3)
  • .env
  • config.toml
  • registry/config.json
Additional context used
golangci-lint
internal/store/http-fetch.go

75-75: Consider pre-allocating images (prealloc)

main.go

40-40: Function 'run' has too many statements (61 > 40) (funlen)


84-84: if bringOwnRegistry has complex nested blocks (complexity: 5) (nestif)

internal/store/in-memory-store.go

41-41: Function 'List' has too many statements (58 > 40) (funlen)


207-207: if storeImage == image has complex nested blocks (complexity: 9) (nestif)


152-152: ineffectual assignment to change (ineffassign)

LanguageTool
README.md

[style] ~10-~10: The phrase ‘in recent years’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others. (IN_RECENT_STYLE)
Context: ...egistry is unavailable. ## Background In recent years, containers have extended beyond their ...


[uncategorized] ~27-~27: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...s in poor/no coverage areas could prove problematic since : - Harbor wasn't designed to ru...


[duplication] ~31-~31: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...ries is not operationally feasible with Harbor - Harbor would be too similar to a simple regist...


[grammar] ~34-~34: The usual collocation for “independently” is “of”, not “from”. Did you mean “independently of”? (INDEPENDENTLY_FROM_OF)
Context: ...ht and will be able to keep functioning independently from Harbor instances. ## Compatibility Co...


[uncategorized] ~59-~59: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... then be accessible to other local edge devices who can pull required images directly f...


[grammar] ~60-~60: Uncountable nouns are usually not used with an indefinite article. Use simply “direct access”. (A_UNCOUNTABLE)
Context: ...ull required images directly from it. _(A direct access from edge device to the remote registry...


[uncategorized] ~64-~64: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text. (EN_WORD_COHERENCY)
Context: ...onnnectivity, these devices need to run containerised images but cannot pull from a central H...


[uncategorized] ~64-~64: It appears that hyphens are missing in the adjective “up-to-date”. (UP_TO_DATE_HYPHEN)
Context: ... central Harbor registry and thus store up to date images locally._ ![Use Case #1](docs/i...


[uncategorized] ~73-~73: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text. (EN_WORD_COHERENCY)
Context: ...nt amount of IoT devices needing to run containerised applications, a single local registry i...


[uncategorized] ~73-~73: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case. (AMOUNTOF_TO_NUMBEROF)
Context: ...ght not be able to handle the increased amount of demands from edge devices. The solut...


[uncategorized] ~73-~73: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...to deploy several registries to several nodes who are able to automatically replicate...


[style] ~73-~73: As a shorter alternative for ‘able to’, consider using “can”. (BE_ABLE_TO)
Context: ...several registries to several nodes who are able to automatically replicate images across e...


[style] ~79-~79: Consider using more formal and concise wording here. (RESPONSIBLE_FOR)
Context: ... The stateless satellite component will be in charge of configuring the local OCI compliant reg...


[style] ~92-~92: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...imize pull requests to the registry. 3. By directly referencing the registry. ...


[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...directly referencing** the registry. 4. By directly referencing the registry a...

Additional comments not posted (16)
registry/launch-registry.go (1)

9-26: The implementation of LaunchRegistry function looks clean and well-structured. It properly handles command execution and error reporting.

internal/satellite/satellite.go (2)

17-22: The NewSatellite constructor is implemented correctly, initializing the Satellite struct effectively with the provided parameters.


Line range hint 43-68: Consider making the tick rate configurable via an environment variable or a parameter to support different environments without changing the code.

internal/store/http-fetch.go (1)

87-109: The GetDigest method is well-implemented, handling URL manipulations and digest fetching effectively.

internal/replicate/replicate.go (6)

18-20: The addition of the Replicate method to the Replicator interface is crucial for the new functionality. Ensure all implementations of this interface are updated accordingly.


24-36: New data structures (ImageInfo, Repository, RegistryInfo) are well-defined with appropriate JSON tags for serialization. These are essential for handling the structured data of images and repositories.


38-40: The NewReplicator function correctly instantiates BasicReplicator. This factory pattern is good for decoupling the creation of objects from their implementation.


59-97: The DeleteExtraImages method has been significantly refactored. It now uses environment variables to construct the local registry URL and handles image deletion based on a comparison with a provided list. Ensure the environment variables (ZOT_URL, HOST, etc.) are documented and validated before use to prevent runtime errors.


121-144: The getFileInfo function properly handles file operations and JSON unmarshaling. It's important to ensure that the input file path is secure and that there are no potential path traversal vulnerabilities.


146-196: The CopyImage function is critical for the replication process. It correctly handles authentication and pushes images to the destination registry. However, consider handling potential failures in removing the temporary directory more gracefully.

README.md (1)

Line range hint 1-103: The README provides a comprehensive overview of the Harbor Satellite project, including its goals, architecture, and use cases. The documentation is clear and well-structured, making it easy for new contributors to understand the project's scope and objectives.

Tools
LanguageTool

[style] ~10-~10: The phrase ‘in recent years’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others. (IN_RECENT_STYLE)
Context: ...egistry is unavailable. ## Background In recent years, containers have extended beyond their ...


[uncategorized] ~27-~27: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...s in poor/no coverage areas could prove problematic since : - Harbor wasn't designed to ru...


[duplication] ~31-~31: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...ries is not operationally feasible with Harbor - Harbor would be too similar to a simple regist...


[grammar] ~34-~34: The usual collocation for “independently” is “of”, not “from”. Did you mean “independently of”? (INDEPENDENTLY_FROM_OF)
Context: ...ht and will be able to keep functioning independently from Harbor instances. ## Compatibility Co...


[uncategorized] ~59-~59: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... then be accessible to other local edge devices who can pull required images directly f...


[grammar] ~60-~60: Uncountable nouns are usually not used with an indefinite article. Use simply “direct access”. (A_UNCOUNTABLE)
Context: ...ull required images directly from it. _(A direct access from edge device to the remote registry...


[uncategorized] ~64-~64: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text. (EN_WORD_COHERENCY)
Context: ...onnnectivity, these devices need to run containerised images but cannot pull from a central H...


[uncategorized] ~64-~64: It appears that hyphens are missing in the adjective “up-to-date”. (UP_TO_DATE_HYPHEN)
Context: ... central Harbor registry and thus store up to date images locally._ ![Use Case #1](docs/i...


[uncategorized] ~73-~73: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text. (EN_WORD_COHERENCY)
Context: ...nt amount of IoT devices needing to run containerised applications, a single local registry i...


[uncategorized] ~73-~73: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case. (AMOUNTOF_TO_NUMBEROF)
Context: ...ght not be able to handle the increased amount of demands from edge devices. The solut...


[uncategorized] ~73-~73: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...to deploy several registries to several nodes who are able to automatically replicate...


[style] ~73-~73: As a shorter alternative for ‘able to’, consider using “can”. (BE_ABLE_TO)
Context: ...several registries to several nodes who are able to automatically replicate images across e...


[style] ~79-~79: Consider using more formal and concise wording here. (RESPONSIBLE_FOR)
Context: ... The stateless satellite component will be in charge of configuring the local OCI compliant reg...


[style] ~92-~92: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...imize pull requests to the registry. 3. By directly referencing the registry. ...


[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...directly referencing** the registry. 4. By directly referencing the registry a...

internal/store/in-memory-store.go (2)

34-39: The NewInMemoryStore function correctly initializes an in-memory store with a given fetcher. This encapsulation allows for flexible data fetching strategies.


181-191: The Remove method handles the deletion of images from the store. Ensure that error handling is robust and that all edge cases are considered, such as attempting to remove non-existent images.

go.mod (3)

1-1: Updated module name and Go version align with project restructuring and modern Go features.

Also applies to: 3-3


19-441: Numerous indirect dependencies added, likely as sub-dependencies of updated modules.


7-12: Dependencies updated and added, including zotregistry.dev/zot and github.com/spf13/viper, to support new functionality.

Also applies to: 16-16

Comment on lines +36 to +85
func (client *RemoteImageList) List(ctx context.Context) ([]Image, error) {
// Construct the URL for fetching tags
url := client.BaseURL + "/tags/list"

// Encode credentials for Basic Authentication
username := os.Getenv("HARBOR_USERNAME")
password := os.Getenv("HARBOR_PASSWORD")
auth := base64.StdEncoding.EncodeToString([]byte(username + ":" + password))

// Create a new HTTP request
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
}

// Set the Authorization header
req.Header.Set("Authorization", "Basic "+auth)

// Send the request
httpClient := &http.Client{}
resp, err := httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("failed to fetch tags: %w", err)
}
defer resp.Body.Close()

// Read the response body
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}

// Unmarshal the JSON response
var tagListResponse TagListResponse
if err := json.Unmarshal(body, &tagListResponse); err != nil {
return nil, fmt.Errorf("failed to unmarshal JSON response: %w", err)
}

// Prepare a slice to store the images
var images []Image

// Iterate over the tags and construct the image references
for _, tag := range tagListResponse.Tags {
images = append(images, Image{
Name: fmt.Sprintf("%s:%s", tagListResponse.Name, tag),
})
}
fmt.Println("Fetched", len(images), "images :", images)
return images, nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider pre-allocating the images slice to improve memory usage and performance.

- var images []Image
+ images := make([]Image, 0, len(tagListResponse.Tags))
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (client *RemoteImageList) List(ctx context.Context) ([]Image, error) {
// Construct the URL for fetching tags
url := client.BaseURL + "/tags/list"
// Encode credentials for Basic Authentication
username := os.Getenv("HARBOR_USERNAME")
password := os.Getenv("HARBOR_PASSWORD")
auth := base64.StdEncoding.EncodeToString([]byte(username + ":" + password))
// Create a new HTTP request
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
}
// Set the Authorization header
req.Header.Set("Authorization", "Basic "+auth)
// Send the request
httpClient := &http.Client{}
resp, err := httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("failed to fetch tags: %w", err)
}
defer resp.Body.Close()
// Read the response body
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
// Unmarshal the JSON response
var tagListResponse TagListResponse
if err := json.Unmarshal(body, &tagListResponse); err != nil {
return nil, fmt.Errorf("failed to unmarshal JSON response: %w", err)
}
// Prepare a slice to store the images
var images []Image
// Iterate over the tags and construct the image references
for _, tag := range tagListResponse.Tags {
images = append(images, Image{
Name: fmt.Sprintf("%s:%s", tagListResponse.Name, tag),
})
}
fmt.Println("Fetched", len(images), "images :", images)
return images, nil
}
func (client *RemoteImageList) List(ctx context.Context) ([]Image, error) {
// Construct the URL for fetching tags
url := client.BaseURL + "/tags/list"
// Encode credentials for Basic Authentication
username := os.Getenv("HARBOR_USERNAME")
password := os.Getenv("HARBOR_PASSWORD")
auth := base64.StdEncoding.EncodeToString([]byte(username + ":" + password))
// Create a new HTTP request
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, fmt.Errorf("failed to create request: %w", err)
}
// Set the Authorization header
req.Header.Set("Authorization", "Basic "+auth)
// Send the request
httpClient := &http.Client{}
resp, err := httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("failed to fetch tags: %w", err)
}
defer resp.Body.Close()
// Read the response body
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
// Unmarshal the JSON response
var tagListResponse TagListResponse
if err := json.Unmarshal(body, &tagListResponse); err != nil {
return nil, fmt.Errorf("failed to unmarshal JSON response: %w", err)
}
// Prepare a slice to store the images
images := make([]Image, 0, len(tagListResponse.Tags))
// Iterate over the tags and construct the image references
for _, tag := range tagListResponse.Tags {
images = append(images, Image{
Name: fmt.Sprintf("%s:%s", tagListResponse.Name, tag),
})
}
fmt.Println("Fetched", len(images), "images :", images)
return images, nil
}
Tools
golangci-lint

75-75: Consider pre-allocating images (prealloc)

Comment on lines +42 to +50
func (r *BasicReplicator) Replicate(ctx context.Context, image string) error {

source := getPullSource(image)

if source != "" {
CopyImage(source)
}
return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Replicate method in BasicReplicator seems to lack error handling after calling CopyImage. Consider adding error checking to ensure robustness.

- CopyImage(source)
+ if err := CopyImage(source); err != nil {
+   return fmt.Errorf("failed to copy image: %w", err)
+ }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (r *BasicReplicator) Replicate(ctx context.Context, image string) error {
source := getPullSource(image)
if source != "" {
CopyImage(source)
}
return nil
}
func (r *BasicReplicator) Replicate(ctx context.Context, image string) error {
source := getPullSource(image)
if source != "" {
if err := CopyImage(source); err != nil {
return fmt.Errorf("failed to copy image: %w", err)
}
}
return nil
}

Comment on lines +99 to +119
func getPullSource(image string) string {
input := os.Getenv("USER_INPUT")
if os.Getenv("SCHEME") == "https://" {
url := os.Getenv("HOST") + "/" + os.Getenv("REGISTRY") + "/" + image
return url
} else {
registryInfo, err := getFileInfo(input)
if err != nil {
return "Error loading file info: " + err.Error()
}
registryURL := registryInfo.RegistryUrl
registryURL = strings.TrimPrefix(registryURL, "https://")
registryURL = strings.TrimSuffix(registryURL, "v2/")

// TODO: Handle multiple repositories
repositoryName := registryInfo.Repositories[0].Repository

return registryURL + repositoryName + "/" + image
}

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getPullSource function uses environment variables to determine the pull source URL. This approach is flexible but requires careful management of environment variables. Consider adding error handling for missing or malformed environment variables.

+ if url == "" {
+   return "", fmt.Errorf("environment variable HOST or REGISTRY not set")
+ }

Committable suggestion was skipped due to low confidence.

Comment on lines +41 to +157
s.AddImage(ctx, img.Name)
change = true
} else {
fmt.Printf("Image %s already exists in the store\n", img.Name)
}
}

// Iterate over s.images and remove any image that is not found in imageList
for image := range s.images {
found := false
for _, img := range imageList {
if img.Name == image {
found = true
break
}
}
if !found {
s.RemoveImage(ctx, image)
change = true
}
}

// Empty and refill imageList with the contents from s.images
imageList = imageList[:0]
for name, digest := range s.images {
imageList = append(imageList, Image{Name: name, Digest: digest})
}

// Print out the entire store for debugging purposes
fmt.Println("Current store:")
for image := range s.images {
fmt.Printf("Image: %s\n", image)
}

case "Remote":
// Trim the imageList elements to remove the project name from the image reference
for i, img := range imageList {
parts := strings.Split(img.Name, "/")
if len(parts) > 1 {
// Take the second part as the new Reference
imageList[i].Name = parts[1]
}
}
// iterate over imageList and call GetDigest for each tag
for _, img := range imageList {
// Split the image reference to get the tag
tagParts := strings.Split(img.Name, ":")
// Check if there is a tag part, min length is 1 char
if len(tagParts) < 2 {
fmt.Println("No tag part found in the image reference")
}
// Use the last part as the tag
tag := tagParts[len(tagParts)-1]
// Get the digest for the tag
digest, err := s.fetcher.GetDigest(ctx, tag)
if err != nil {
return nil, err
}

// Check if the image exists and matches the digest
if !(s.checkImageAndDigest(digest, img.Name)) {
change = true
}

}

// Create imageMap filled with all images from remote imageList
imageMap := make(map[string]bool)
for _, img := range imageList {
imageMap[img.Name] = true
}

// Iterate over in memory store and remove any image that is not found in imageMap
for digest, image := range s.images {
if _, exists := imageMap[image]; !exists {
s.Remove(ctx, digest, image)
change = true
}
}
// Print out the entire store for debugging purposes
fmt.Println("Current store:")
for digest, imageRef := range s.images {
fmt.Printf("Digest: %s, Image: %s\n", digest, imageRef)
}

// Empty and refill imageList with the contents from s.images
imageList = imageList[:0]
for _, name := range s.images {
imageList = append(imageList, Image{Digest: "", Name: name})
}

}
if change {
fmt.Println("Changes detected in the store")
change = false
return imageList, nil
} else {
fmt.Println("No changes detected in the store")
return nil, nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The List method has been refactored to handle different fetcher types and manage the internal image list more dynamically. However, the method is complex and could benefit from further refactoring to improve readability and maintainability.

Consider splitting the handling of "File" and "Remote" fetcher types into separate methods to reduce complexity and improve clarity.

Tools
golangci-lint

41-41: Function 'List' has too many statements (58 > 40) (funlen)


152-152: ineffectual assignment to change (ineffassign)

Comment on lines +201 to +240
// TODO: Rework complicated logic and add support for multiple repositories
// checkImageAndDigest checks if the image exists in the store and if the digest matches the image reference
func (s *inMemoryStore) checkImageAndDigest(digest string, image string) bool {

// Check if the received image exists in the store
for storeDigest, storeImage := range s.images {
if storeImage == image {
// Image exists, now check if the digest matches
if storeDigest == digest {
// Digest exists and matches the current image's
// Remove what comes before the ":" in image and set it to tag variable
tag := strings.Split(image, ":")[1]
localRegistryDigest, err := GetLocalDigest(context.Background(), tag)
if err != nil {
fmt.Println("Error getting digest from local registry:", err)
return false
} else {
// Check if the digest from the local registry matches the digest from the store
if digest == localRegistryDigest {
return true
} else {
return false
}
}
} else {
// Digest exists but does not match the current image reference
s.Remove(context.Background(), storeDigest, storeImage)
s.Add(context.Background(), digest, image)
return false
}
}
}

// Try to add the image to the store
// Add will check if it already exists in the store before adding
// If adding was successful, return true, else return false
err := s.Add(context.Background(), digest, image)
return err != nil

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checkImageAndDigest method is overly complex and does too much. Consider breaking this method into smaller, more focused methods to improve readability and maintainability.

Split the functionality into separate methods for checking existence, matching digests, and modifying the store.

Tools
golangci-lint

207-207: if storeImage == image has complex nested blocks (complexity: 9) (nestif)

Comment on lines +161 to +171
func (s *inMemoryStore) Add(ctx context.Context, digest string, image string) error {
// Check if the image already exists in the store
if _, exists := s.images[digest]; exists {
fmt.Printf("Image: %s, digest: %s already exists in the store.\n", image, digest)
return fmt.Errorf("image %s already exists in the store", image)
} else {
// Add the image and its digest to the store
s.images[digest] = image
fmt.Printf("Successfully added image: %s, digest: %s\n", image, digest)
return nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Add method correctly checks for existing images before adding new ones. However, the error message could be more descriptive.

- return fmt.Errorf("image %s already exists in the store", image)
+ return fmt.Errorf("image %s with digest %s already exists in the store", image, digest)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *inMemoryStore) Add(ctx context.Context, digest string, image string) error {
// Check if the image already exists in the store
if _, exists := s.images[digest]; exists {
fmt.Printf("Image: %s, digest: %s already exists in the store.\n", image, digest)
return fmt.Errorf("image %s already exists in the store", image)
} else {
// Add the image and its digest to the store
s.images[digest] = image
fmt.Printf("Successfully added image: %s, digest: %s\n", image, digest)
return nil
}
func (s *inMemoryStore) Add(ctx context.Context, digest string, image string) error {
// Check if the image already exists in the store
if _, exists := s.images[digest]; exists {
fmt.Printf("Image: %s, digest: %s already exists in the store.\n", image, digest)
return fmt.Errorf("image %s with digest %s already exists in the store", image, digest)
} else {
// Add the image and its digest to the store
s.images[digest] = image
fmt.Printf("Successfully added image: %s, digest: %s\n", image, digest)
return nil
}

@Vad1mo Vad1mo merged commit 8439290 into main Jun 25, 2024
1 check passed
@Vad1mo Vad1mo deleted the decouple-zot-registry branch June 25, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation golang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants