Add Quickstart #60
Add Quickstart #60
Conversation
Signed-off-by: bupd <[email protected]>
- This adds development server for ground control - This commit only focuses on setting the dev server for ground-control - Dagger has a known limitation for hot reloading dev servers. - limitation issue in dagger (dagger/dagger#6990) Signed-off-by: bupd <[email protected]>
WalkthroughThe changes introduce configuration files for the Air tool to facilitate live reloading in Go applications and enhance documentation for setting up the Harbor Satellite project. A new method Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
.air.toml (2)
12-13: Consider environment-specific logging configuration.The current logging configuration is set to "debug" level, which might not be suitable for all environments. Additionally, the logging configuration could benefit from more development-friendly options.
Consider enhancing the logging configuration:
[log] - level = "debug" + # Use environment variable to control log level + level = "${AIR_LOG_LEVEL}" + # Add development-friendly logging options + time = true + main_only = falseYou can set the environment variable in your development environment:
export AIR_LOG_LEVEL=debug
1-13: Document Air configuration in quickstart guide.Since this configuration is crucial for the development setup and works around the Dagger live reload limitation mentioned in the PR objectives, please ensure that:
- The
.air.tomlconfiguration is explained in the quickstart guide- The relationship between Air and Dagger is clearly documented
- Any environment variables or setup requirements are listed
Would you like me to help draft the documentation section for this configuration?
ground-control/.air.toml (1)
12-13: Consider environment-specific logging configurationThe debug log level is fine for development but might generate excessive output. Consider adding a comment to explain this or implementing environment-specific configurations.
Add a comment explaining the log level choice:
[log] + # Debug level is used for development environment + # Consider using "info" for production deployments level = "debug"QUICKSTART.md (2)
5-10: Enhance prerequisites section with specific requirements.The prerequisites section should include:
- Minimum Dagger version number
- Supported operating systems
- System requirements (disk space, memory)
1-100: Overall document structure is good, but needs additional sections.Consider adding these important sections:
- Version compatibility matrix
- Backup and recovery procedures
- Common errors and solutions
- Security best practices
- Resource cleanup instructions
ci/main.go (3)
23-30: Enhance method documentation for better clarity.The current documentation lacks essential details. Consider adding:
- Parameter descriptions (especially the optional
source)- Return value documentation
- Usage examples
- Prerequisites (Air tool requirement)
- Limitations (no live reload support, as mentioned in PR description)
-// build-dev function would start the dev server for the component provided. +// BuildDev starts a development server for the specified component using the Air tool for live reloading. +// +// Parameters: +// - ctx: Context for cancellation and timeout +// - source: Optional directory containing the source code (defaults to "./ground-control") +// - component: Component to run ("satellite" or "ground-control") +// +// Returns: +// - *dagger.Service: Service handle for the running development server +// - error: Any error encountered during setup +// +// Note: Currently, live reloading requires manual restart due to Dagger limitations. +// See: https://github.com/dagger/dagger/issues/XXX +// +// Example: +// service, err := harbor.BuildDev(ctx, dag.Host().Directory("./"), "ground-control")
51-52: Track TODO item for satellite implementation.The TODO comment should be tracked properly to ensure it's not forgotten.
Would you like me to create a GitHub issue to track the satellite implementation task?
56-57: Enhance error message for better user guidance.The error message could be more informative by including usage examples.
- return nil, fmt.Errorf("error: please provide component as either satellite or ground-control") + return nil, fmt.Errorf("invalid component %q: must be either 'satellite' or 'ground-control'", component)README.md (2)
57-60: Improve clarity and consistency in use case descriptions.Several style and grammar improvements are needed:
- Add missing commas after introductory phrases
- Use "number of" instead of "amount of" when referring to countable items
- Maintain consistent spelling between "containerised" and "containerized"
Consider these improvements:
-The stateless satellite component will handle pulling images +The stateless satellite component will handle pulling images, -amount of demands +number of demands -containerised +containerizedAlso applies to: 68-72
🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... then be accessible to other local edge devices who can pull required images directly f...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~59-~59: Uncountable nouns are usually not used with an indefinite article. Use simply “direct access”.
Context: ...ull required images directly from it. _(A direct access from edge device to the remote registry...(A_UNCOUNTABLE)
77-79: Consider formatting improvements for consistency.The use case descriptions would benefit from consistent formatting:
- Use consistent punctuation at the end of list items
- Maintain consistent paragraph spacing between sections
Would you like me to provide specific formatting suggestions for these sections?
🧰 Tools
🪛 LanguageTool
[style] ~78-~78: Consider using more formal and concise wording here.
Context: ... The stateless satellite component will be in charge of configuring the local OCI compliant reg...(RESPONSIBLE_FOR)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.air.toml(1 hunks)QUICKSTART.md(1 hunks)README.md(2 hunks)ci/main.go(1 hunks)ground-control/.air.toml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[typographical] ~17-~17: Consider adding a comma here.
Context: ...decentralized registries. ## QuickStart Please refer to the latest setup instructions ...
(PLEASE_COMMA)
[uncategorized] ~58-~58: Possible missing comma found.
Context: ... then be accessible to other local edge devices who can pull required images directly f...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~59-~59: Uncountable nouns are usually not used with an indefinite article. Use simply “direct access”.
Context: ...ull required images directly from it. _(A direct access from edge device to the remote registry...
(A_UNCOUNTABLE)
[uncategorized] ~63-~63: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text.
Context: ...onnnectivity, these devices need to run containerised images but cannot pull from a central H...
(EN_WORD_COHERENCY)
[uncategorized] ~63-~63: It appears that hyphens are missing in the adjective “up-to-date”.
Context: ... central Harbor registry and thus store up to date images locally._ 
[uncategorized] ~72-~72: Do not mix variants of the same word (‘containerise’ and ‘containerize’) within a single text.
Context: ...nt amount of IoT devices needing to run containerised applications, a single local registry i...
(EN_WORD_COHERENCY)
[uncategorized] ~72-~72: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ght not be able to handle the increased amount of demands from edge devices. The solut...
(AMOUNTOF_TO_NUMBEROF)
[uncategorized] ~72-~72: Possible missing comma found.
Context: ...to deploy several registries to several nodes who are able to automatically replicate...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~72-~72: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...several registries to several nodes who are able to automatically replicate images across e...
(BE_ABLE_TO)
[style] ~78-~78: Consider using more formal and concise wording here.
Context: ... The stateless satellite component will be in charge of configuring the local OCI compliant reg...
(RESPONSIBLE_FOR)
🔇 Additional comments (3)
.air.toml (1)
1-10: 🛠️ Refactor suggestion
Enhance build configuration security and specificity.
The current build configuration has several areas that could be improved for better security and maintainability:
- The binary name "main" is too generic and could conflict with other tools in the PATH
- The build directory "." is ambiguous and could lead to wrong builds if the command is executed from a subdirectory
- The
run_after_buildcommand could affect wrong files if a "main" binary exists elsewhere
Consider applying these improvements:
[build]
- bin = "main"
+ bin = "harbor-satellite-dev"
- dir = "."
+ dir = "${GOPATH}/src/github.com/yourorg/harbor-satellite"
- cmd = "go build -o main ."
+ cmd = "go build -o ./bin/harbor-satellite-dev ."
exclude_dir = ["assets", "tmp", "vendor", "testdata"]
exclude_regex = ["_test.go"]
exclude_unchanged = false
follow_symlink = false
- run_after_build = "chmod +x main"
+ run_after_build = "chmod +x ./bin/harbor-satellite-dev"
include_ext = ["go"]Additionally, consider adding:
- Build tags for development builds
- Clear temporary files after builds
- Version information in the binary
✅ Verification successful
Adjust Air configuration to align with project structure and gitignore
Based on the codebase analysis, here are the recommended changes to the Air configuration:
[build]
- bin = "main"
+ bin = "bin/harbor-satellite"
dir = "."
- cmd = "go build -o main ."
+ cmd = "go build -o bin/harbor-satellite ."
exclude_dir = ["assets", "tmp", "vendor", "testdata"]
exclude_regex = ["_test.go"]
exclude_unchanged = false
follow_symlink = false
- run_after_build = "chmod +x main"
+ run_after_build = "chmod +x bin/harbor-satellite"
include_ext = ["go"]The changes are based on:
- Project's module name is
container-registry.com/harbor-satellite /bindirectory is already in.gitignorefor build outputs- Main package is in the root directory
- More specific binary name that matches the project
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the current directory structure supports the suggested paths
fd -t d "bin$"
Length of output: 16
Script:
#!/bin/bash
# Let's gather more context about the project structure and build setup
# Check for Go module name and structure
cat go.mod 2>/dev/null || echo "No go.mod found"
# Check for any existing build or binary directories
fd -t d "(build|dist|bin|target)$"
# Look for other .air.toml files for consistency
fd -t f ".air.toml$"
# Check for main package location
rg -l "package main" --type go
Length of output: 25699
Script:
#!/bin/bash
# Let's check the main.go content to understand the project structure
cat main.go
# Check if there are any existing build scripts or Makefiles
fd -t f "(Makefile|build\.sh)$"
# Look for any existing build output directories in gitignore
cat .gitignore 2>/dev/null
Length of output: 4862
ground-control/.air.toml (1)
1-13: Verify Air integration with Dagger setup
Since this configuration is part of addressing Dagger's live reload limitations, we should verify:
- That Air's file watching works correctly with the Dagger container setup
- That the included file patterns catch all relevant changes
Let's check for any potential file watching configuration in Dagger:
✅ Verification successful
The previous results show all Go files are within the watched directories, but we need to verify the Dagger integration specifics and any potential conflicts with Air's live reload. Let's examine the Dagger pipeline configuration and development setup.
Air configuration is compatible with the development setup
The Air configuration is correctly set up and won't conflict with the Dagger pipeline:
- The Air config watches all
.gofiles in the project directory, excluding test files and vendor directories - Dagger is used for CI/CD pipelines and testing, not for local development, so there's no conflict with Air's live reload functionality
- The file watching patterns in
.air.tomlcover all Go source files in the project structure - The build command and directory settings are appropriate for the Go project structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for file watching configurations in Dagger files
# and potential conflicts with Air configuration
# Look for Dagger-related file watching configurations
rg -l "watch|reload|live" --type yaml --type go
# Check if there are any Go files outside the watched directories
fd -e go -E "vendor" -E "testdata" -E "tmp"
Length of output: 1539
Script:
#!/bin/bash
# Let's check the Dagger pipeline configuration and any file watching mechanisms
# Search for Dagger pipeline definitions
rg -A 5 "dagger.Pipeline"
# Check for any watch/reload related code in ci/ground_control.go
cat ci/ground_control.go
# Look for development-related configurations
fd -e yaml -e yml
Length of output: 1019
README.md (1)
17-18: LGTM! QuickStart section addition aligns with PR objectives.
The new section effectively directs users to the detailed setup instructions, fulfilling the requirement for better documentation of the local setup process.
🧰 Tools
🪛 LanguageTool
[typographical] ~17-~17: Consider adding a comma here.
Context: ...decentralized registries. ## QuickStart Please refer to the latest setup instructions ...
(PLEASE_COMMA)
| [build] | ||
| bin = "main" | ||
| dir = "." | ||
| cmd = "go build -o main ." | ||
| exclude_dir = ["assets", "tmp", "vendor", "testdata"] | ||
| exclude_regex = ["_test.go"] | ||
| exclude_unchanged = false | ||
| follow_symlink = false | ||
| run_after_build = "chmod +x main" | ||
| include_ext = ["go"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in build configuration
- The binary name "main" is too generic and could lead to conflicts. Consider using a more specific name like "ground-control-dev".
- The
chmod +xcommand should specify explicit permissions (e.g., 755) for better security. - Setting
exclude_unchanged = falsemeans every file change triggers a rebuild, which could impact development performance. - Consider adding more exclude directories like ".git", "node_modules".
Apply these improvements:
[build]
- bin = "main"
+ bin = "ground-control-dev"
dir = "."
- cmd = "go build -o main ."
+ cmd = "go build -o ground-control-dev ."
- exclude_dir = ["assets", "tmp", "vendor", "testdata"]
+ exclude_dir = ["assets", "tmp", "vendor", "testdata", ".git", "node_modules"]
exclude_regex = ["_test.go"]
- exclude_unchanged = false
+ exclude_unchanged = true
follow_symlink = false
- run_after_build = "chmod +x main"
+ run_after_build = "chmod 755 ground-control-dev"
include_ext = ["go"]📝 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.
| [build] | |
| bin = "main" | |
| dir = "." | |
| cmd = "go build -o main ." | |
| exclude_dir = ["assets", "tmp", "vendor", "testdata"] | |
| exclude_regex = ["_test.go"] | |
| exclude_unchanged = false | |
| follow_symlink = false | |
| run_after_build = "chmod +x main" | |
| include_ext = ["go"] | |
| [build] | |
| bin = "ground-control-dev" | |
| dir = "." | |
| cmd = "go build -o ground-control-dev ." | |
| exclude_dir = ["assets", "tmp", "vendor", "testdata", ".git", "node_modules"] | |
| exclude_regex = ["_test.go"] | |
| exclude_unchanged = true | |
| follow_symlink = false | |
| run_after_build = "chmod 755 ground-control-dev" | |
| include_ext = ["go"] |
| Then navigate to the `satellite` directory and verify that `config.toml` is set up correctly: | ||
|
|
||
| ```toml | ||
| # Whether to use the built-in Zot registry or not | ||
| bring_own_registry = false | ||
|
|
||
| # IP address and port of the registry | ||
| own_registry_adr = "127.0.0.1" | ||
| own_registry_port = "8585" | ||
|
|
||
| # URL of remote registry or local file path | ||
| url_or_file = "https://demo.goharbor.io/v2/myproject/album-server" | ||
|
|
||
|
|
||
|
|
||
| # Default path for Zot registry config.json | ||
| zotConfigPath = "./registry/config.json" | ||
|
|
||
| # Set logging level | ||
| log_level = "info" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance configuration documentation with explanations and validation steps.
The configuration section needs:
- Explanations for each configuration option
- Steps to validate the configuration
- A more generic example URL
Apply this diff:
# Whether to use the built-in Zot registry or not
bring_own_registry = false
+# Set to true if you want to use an external registry instead of the built-in Zot registry
# IP address and port of the registry
own_registry_adr = "127.0.0.1"
own_registry_port = "8585"
+# These settings are used when bring_own_registry = true
# URL of remote registry or local file path
-url_or_file = "https://demo.goharbor.io/v2/myproject/album-server"
+url_or_file = "<your_registry_url>/v2/<project>/<repository>"
+# Format: https://registry.example.com/v2/project/repository
# Default path for Zot registry config.json
zotConfigPath = "./registry/config.json"
+# This path must exist and be readable
# Set logging level
log_level = "info"
+# Available levels: debug, info, warn, errorAdd validation steps:
To validate your configuration:
1. Verify file permissions for zotConfigPath
2. Test registry connectivity
3. Confirm port availability
QUICKSTART.md
Outdated
| ### 6. Register the Satellite with Ground Control | ||
| Using `curl` or Postman, make a `POST` request to register the Satellite with Ground Control: | ||
|
|
||
| ```bash | ||
| curl -X POST http://localhost:8080/satellites/register -H "Content-Type: application/json" -d '{ "name": "<satellite_name_here>" }' | ||
| ``` | ||
|
|
||
| The response will include a token string. Set this token in the Satellite `.env` file: | ||
|
|
||
| ```console | ||
| TOKEN=<string_from_ground_control> | ||
| ``` | ||
|
|
||
| ### 7. Start the Satellite | ||
| Run the following Dagger command to start the Satellite service: | ||
|
|
||
| ```bash | ||
| dagger call build-dev --component satellite up | ||
| ``` | ||
|
|
||
| The Satellite service will start on port 9090. Ensure that the `ground_control_url` is correctly set in the Satellite configuration before launching. | ||
|
|
||
|
|
||
| ### 8. Finalize and Run | ||
| After setting the token, you can now run the Satellite. This setup will launch the Satellite in a container with the following exposed ports: | ||
| - **9090** for the Satellite service. | ||
| - **8585** for the Zot registry (if configured). | ||
|
|
||
| With this setup, your Harbor Satellite should be up and running! |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and verification steps to the setup process.
The setup instructions should include:
- Error handling for registration failures
- Health check endpoints
- Troubleshooting steps
- Verification of successful setup
Add the following sections:
### Verification Steps
1. Check Ground Control health:
```bash
curl http://localhost:8080/health- Verify Satellite registration:
curl http://localhost:8080/satellites/status
Troubleshooting
Common issues and solutions:
- Connection refused: Verify ports are not in use
- Registration fails: Check Ground Control logs
- Token issues: Verify token format and permissions
Health Monitoring
Monitor these endpoints:
- Ground Control: http://localhost:8080/health
- Satellite: http://localhost:9090/health
- Zot Registry: http://localhost:8585/health
<!-- This is an auto-generated comment by CodeRabbit -->
QUICKSTART.md
Outdated
| ```bash | ||
| HARBOR_USERNAME=admin | ||
| HARBOR_PASSWORD=Harbor12345 | ||
| HARBOR_URL=https://demo.goharbor.io | ||
|
|
||
| PORT=8080 | ||
| APP_ENV=local | ||
|
|
||
| DB_HOST=localhost | ||
| DB_PORT=5432 | ||
| DB_DATABASE=groundcontrol | ||
| DB_USERNAME=postgres # Customize based on your DB config | ||
| DB_PASSWORD=password # Customize based on your DB config | ||
| ``` |
There was a problem hiding this comment.
Security: Replace sensitive credentials with placeholders.
The example contains actual credentials and URLs which could be security risks if copied directly. Consider:
- Using placeholders instead of actual credentials
- Adding a warning about securing sensitive information
- Mentioning credential management best practices
Apply this diff:
-HARBOR_USERNAME=admin
-HARBOR_PASSWORD=Harbor12345
-HARBOR_URL=https://demo.goharbor.io
+HARBOR_USERNAME=<your_harbor_username>
+HARBOR_PASSWORD=<your_harbor_password>
+HARBOR_URL=<your_harbor_url>
PORT=8080
APP_ENV=local
DB_HOST=localhost
DB_PORT=5432
DB_DATABASE=groundcontrol
-DB_USERNAME=postgres # Customize based on your DB config
-DB_PASSWORD=password # Customize based on your DB config
+DB_USERNAME=<your_db_username>
+DB_PASSWORD=<your_db_password>📝 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.
| ```bash | |
| HARBOR_USERNAME=admin | |
| HARBOR_PASSWORD=Harbor12345 | |
| HARBOR_URL=https://demo.goharbor.io | |
| PORT=8080 | |
| APP_ENV=local | |
| DB_HOST=localhost | |
| DB_PORT=5432 | |
| DB_DATABASE=groundcontrol | |
| DB_USERNAME=postgres # Customize based on your DB config | |
| DB_PASSWORD=password # Customize based on your DB config | |
| ``` | |
| ```bash | |
| HARBOR_USERNAME=<your_harbor_username> | |
| HARBOR_PASSWORD=<your_harbor_password> | |
| HARBOR_URL=<your_harbor_url> | |
| PORT=8080 | |
| APP_ENV=local | |
| DB_HOST=localhost | |
| DB_PORT=5432 | |
| DB_DATABASE=groundcontrol | |
| DB_USERNAME=<your_db_username> | |
| DB_PASSWORD=<your_db_password> | |
| ``` |
ci/main.go
Outdated
| if component == "ground-control" { | ||
| golang = golang. | ||
| WithExec([]string{"ls", "-la"}). | ||
| // WithWorkdir("./ground-control/"). | ||
| // WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithExec([]string{"air", "-c", ".air.toml"}). | ||
| WithExposedPort(8080) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up development setup and improve error handling.
The ground-control setup needs cleanup and improvements:
- Remove debug
lscommands - Remove or implement commented code
- Add validation for Air config
- Use constant for port number
if component == "ground-control" {
golang = golang.
- WithExec([]string{"ls", "-la"}).
- // WithWorkdir("./ground-control/").
- // WithExec([]string{"go", "mod", "download"}).
- WithExec([]string{"ls", "-la"}).
+ WithExec([]string{"test", "-f", ".air.toml"}). // Validate config
WithExec([]string{"air", "-c", ".air.toml"}).
- WithExposedPort(8080)
+ WithExposedPort(8080) // Consider adding a constant for thisCommittable suggestion skipped: line range outside the PR's diff.
ci/main.go
Outdated
| if component == "satellite" || component == "ground-control" { | ||
| golang := dag.Container(). | ||
| From("golang:latest"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithEnvVariable("GOCACHE", "/go/build-cache"). | ||
| WithMountedDirectory(PROJ_MOUNT, source). | ||
| WithWorkdir(PROJ_MOUNT). | ||
| WithExec([]string{"go", "install", "github.com/air-verse/air@latest"}) | ||
|
|
There was a problem hiding this comment.
Improve container setup reliability and consistency.
Several issues need attention:
- Using "latest" tag can lead to non-reproducible builds
- Air tool version should be pinned for consistency
- Missing error handling for Air installation
Apply these changes:
- golang := dag.Container().
- From("golang:latest").
+ golang := dag.Container().
+ From(DEFAULT_GO).
+ WithExec([]string{"go", "install", "github.com/air-verse/[email protected]"}).
+ WithExec([]string{"which", "air"}). // Verify installationCommittable suggestion skipped: line range outside the PR's diff.
ci/main.go
Outdated
| // build-dev function would start the dev server for the component provided. | ||
| func (m *HarborSatellite) BuildDev( | ||
| ctx context.Context, | ||
| // +optional | ||
| // +defaultPath="./ground-control" | ||
| source *dagger.Directory, | ||
| component string, | ||
| ) (*dagger.Service, error) { | ||
| if component == "satellite" || component == "ground-control" { | ||
| golang := dag.Container(). | ||
| From("golang:latest"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithEnvVariable("GOCACHE", "/go/build-cache"). | ||
| WithMountedDirectory(PROJ_MOUNT, source). | ||
| WithWorkdir(PROJ_MOUNT). | ||
| WithExec([]string{"go", "install", "github.com/air-verse/air@latest"}) | ||
|
|
||
| if component == "ground-control" { | ||
| golang = golang. | ||
| WithExec([]string{"ls", "-la"}). | ||
| // WithWorkdir("./ground-control/"). | ||
| // WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithExec([]string{"air", "-c", ".air.toml"}). | ||
| WithExposedPort(8080) | ||
| } | ||
| // to-do: build else part for the satellite | ||
|
|
||
| return golang.AsService(), nil | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("error: please provide component as either satellite or ground-control") | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing Air configuration file and potential setup issues
The verification revealed several concerns:
- The
.air.tomlconfiguration file referenced in the code is missing from both the root and ground-control directories - The ground-control module exists with its own go.mod, but the Air development server setup is incomplete
- The BuildDev method is only defined but not used elsewhere in the codebase
Required actions:
- Add the missing
.air.tomlconfiguration file in the ground-control directory - Ensure the Air development server configuration aligns with the Go module structure
- Consider documenting the BuildDev usage in the codebase
🔗 Analysis chain
Verify Air configuration and method usage.
Let's verify the integration points:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify Air configuration and BuildDev usage
# Check if Air config exists
echo "Checking Air configuration files..."
fd -t f ".air.toml$"
# Look for BuildDev method usage
echo -e "\nChecking BuildDev method usage..."
rg "BuildDev.*ground-control" -t go
Length of output: 224
Script:
#!/bin/bash
# Let's broaden our search to get more context
# Check for any Air-related configuration files with different extensions
echo "Checking for Air configuration files with various extensions..."
fd -t f "air\.(toml|conf|config|yaml|yml)$"
# Look for BuildDev method usage with a broader search
echo -e "\nChecking BuildDev method usage (broader search)..."
rg "BuildDev" -A 3 -B 1 -t go
# Check the ground-control directory structure
echo -e "\nChecking ground-control directory structure..."
fd . ground-control
# Look for go.mod in ground-control
echo -e "\nChecking for go.mod in ground-control..."
fd "go.mod$" ground-control
Length of output: 2711
| A typical use case would work as follows : | ||
| _(A direct access from edge device to the remote registry is still possible when network conditions permit it)._ | ||
| The satellite component may also handle updating container runtime configurations and fetching image lists from Ground Control, a part of Harbor. | ||
| The stateful local regsitry will also need to handle storing and managing data on local volumes. |
There was a problem hiding this comment.
Fix typo in "registry" spelling.
There are instances where "registry" is misspelled as "regsitry".
Apply these corrections:
-regsitry
+registryAlso applies to: 68-68
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
ci/main.go (2)
23-30: Enhance method documentation for better clarity.The current documentation lacks essential details. Consider adding:
- Parameter descriptions and their requirements
- Return value explanation
- Prerequisites (e.g., Air tool requirement)
- Example usage
-// build-dev function would start the dev server for the component provided. +// BuildDev starts a development server for the specified component using the Air tool +// for live reloading. +// +// Parameters: +// - ctx: Context for cancellation and timeout +// - source: Directory containing the source code (defaults to ".") +// - component: Either "satellite" or "ground-control" +// +// Returns: +// - *dagger.Service: Running development server +// - error: If component is invalid or setup fails +// +// Prerequisites: +// - Air tool for live reloading +// - For ground-control: PostgreSQL database (automatically started)
60-66: Improve TODO comment and error handling.The TODO comment lacks detail, and error messages could be more helpful.
- // to-do: build else part for the satellite + // TODO: Implement satellite development server setup + // Requirements: + // - Add Air configuration for satellite + // - Configure appropriate ports and environment + // - Add any satellite-specific dependencies - return nil, fmt.Errorf("error: please provide component as either satellite or ground-control") + return nil, fmt.Errorf("invalid component %q: must be either 'satellite' or 'ground-control'", component)go.mod (1)
Line range hint
474-483: Review replace directives for OpenTelemetry packages.The replace directives for OpenTelemetry packages are using specific versions. This approach:
- Ensures version consistency
- Might make future updates more challenging
- Should be documented with the reason for these specific replacements
Consider adding a comment explaining why these specific versions are required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
ci/main.go(1 hunks)dagger.json(1 hunks)go.mod(5 hunks)ground-control/.env(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ground-control/.env
🧰 Additional context used
🪛 golangci-lint
ci/main.go
32-32: undefined: dag
(typecheck)
34-34: undefined: dag
(typecheck)
🔇 Additional comments (6)
dagger.json (2)
5-5: Verify CI pipeline updates for local development
Since this PR adds local development capabilities, ensure that the "ci" directory contains the necessary pipeline definitions for both CI and local development scenarios.
Let's check the pipeline definitions:
#!/bin/bash
# Description: Verify pipeline definitions for local development
# Check for development-related functions in the CI code
rg -A 10 "func.*Dev|func.*Local" ci/
# Look for any development server configurations
rg -i "development.*server|dev.*mode|local.*environment" ci/3-3: Verify Dagger engine version compatibility
The upgrade from v0.13.3 to v0.14.0 should be verified for compatibility with the local development setup described in the quickstart guide.
Let's verify the version compatibility and check for any breaking changes:
✅ Verification successful
Dagger engine version is up to date and compatible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Dagger version compatibility and release notes
# Check if v0.14.0 is the latest stable version
curl -s https://api.github.com/repos/dagger/dagger/releases | jq -r '.[].tag_name' | head -n 5
# Search for any breaking changes or known issues in the codebase related to Dagger versions
rg -i "dagger.*version|breaking.?change" README.md CHANGELOG.md docs/
# Check if the quickstart guide mentions specific Dagger version requirements
rg -i "dagger.*version|v0\.14\.0" QUICKSTART.md
Length of output: 992
Script:
#!/bin/bash
# Description: Confirm the latest Dagger version and retrieve release notes for v0.14.0
# Fetch the latest stable release tag from GitHub
latest_version=$(curl -s https://api.github.com/repos/dagger/dagger/releases/latest | jq -r '.tag_name')
echo "Latest Dagger version: $latest_version"
# Compare with the current version in dagger.json
current_version=$(grep '"engineVersion"' dagger.json | awk -F '"' '{print $4}')
echo "Current Dagger version in dagger.json: $current_version"
if [ "$latest_version" != "$current_version" ]; then
echo "A newer Dagger version is available: $latest_version"
else
echo "Dagger is up to date."
fi
# Retrieve release notes for v0.14.0
curl -s https://api.github.com/repos/dagger/dagger/releases/tags/$current_version | jq -r '.body'
Length of output: 1910
ci/main.go (1)
32-41:
Fix container setup reliability issues.
Several critical issues need attention:
- Using "latest" tag can lead to non-reproducible builds
- Air tool version should be pinned
- The
dagvariable is undefined
Apply these changes:
- golang := dag.Container().
- From("golang:latest").
+ golang := dag.Container().
+ From(DEFAULT_GO). // Using the constant defined above
+ WithExec([]string{"go", "install", "github.com/air-verse/[email protected]"}).Also, ensure the dag variable is properly initialized. It seems to be missing from the current scope.
🧰 Tools
🪛 golangci-lint
32-32: undefined: dag
(typecheck)
34-34: undefined: dag
(typecheck)
go.mod (3)
396-397: Review OpenTelemetry integration approach.
The addition of OpenTelemetry metric exporters (both gRPC and HTTP) suggests implementation of metrics collection. Ensure that:
- Only one exporter type is used to avoid duplicate metrics
- Proper configuration is in place for the chosen exporter
#!/bin/bash
# Description: Check OpenTelemetry configuration and usage
# Test: Look for OpenTelemetry initialization and configuration
# Search for OpenTelemetry initialization
rg -l "NewMeterProvider|NewTracerProvider"
# Look for exporter configuration
rg -l "otlpmetrichttp.New|otlpmetricgrpc.New"Also applies to: 402-402, 405-405
31-31: Verify compatibility with updated dependencies.
Major version updates for key dependencies:
github.com/99designs/gqlgen→ v0.17.55github.com/adrg/xdg→ v0.5.1github.com/vektah/gqlparser/v2→ v2.5.17
These updates may include breaking changes that need to be addressed.
#!/bin/bash
# Description: Check for potential breaking changes in dependency usage
# Test: Search for usage of updated packages that might need modifications
# Search for gqlgen usage
rg "github.com/99designs/gqlgen" -A 5
# Search for xdg usage
rg "github.com/adrg/xdg" -A 5
# Search for gqlparser usage
rg "github.com/vektah/gqlparser/v2" -A 5Also applies to: 67-67, 376-376
3-5: Verify Go toolchain compatibility.
The Go version (1.22.5) and toolchain version (1.23.2) mismatch might cause compatibility issues. Typically, the toolchain version should match the Go version being used.
| // starts postgres DB container for ground-control. | ||
| func (m *HarborSatellite) Db(ctx context.Context) (*dagger.Service, error) { | ||
| return dag.Container(). | ||
| From("postgres:17"). | ||
| WithEnvVariable("POSTGRES_USER", "postgres"). | ||
| WithEnvVariable("POSTGRES_PASSWORD", "password"). | ||
| WithEnvVariable("POSTGRES_HOST_AUTH_METHOD", "trust"). | ||
| WithEnvVariable("POSTGRES_DB", "groundcontrol"). | ||
| WithExposedPort(5432). | ||
| AsService().Start(ctx) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve database container configuration and security.
Several improvements needed:
- Make credentials configurable
- Pin PostgreSQL version
- Add container health check
func (m *HarborSatellite) Db(ctx context.Context) (*dagger.Service, error) {
+ const POSTGRES_VERSION = "17.2" // Pin specific version
+
return dag.Container().
- From("postgres:17").
+ From("postgres:" + POSTGRES_VERSION).
WithEnvVariable("POSTGRES_USER", "postgres").
WithEnvVariable("POSTGRES_PASSWORD", "password").
WithEnvVariable("POSTGRES_HOST_AUTH_METHOD", "trust").
WithEnvVariable("POSTGRES_DB", "groundcontrol").
WithExposedPort(5432).
+ WithHealthCheck(
+ dag.Container().
+ WithExec([]string{"pg_isready", "-U", "postgres"}),
+ ).
AsService().Start(ctx)
}Consider extracting credentials to environment variables or a configuration file for better security and flexibility.
Committable suggestion skipped: line range outside the PR's diff.
ci/main.go
Outdated
| golang = golang. | ||
| WithWorkdir(PROJ_MOUNT+"/ground-control/sql/schema"). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithServiceBinding("pgservice", db). | ||
| WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/goose@latest"}). | ||
| WithExec([]string{"goose", "postgres", "postgres://postgres:password@pgservice:5432/groundcontrol", "up"}). | ||
| WithWorkdir(PROJ_MOUNT + "/ground-control"). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"air", "-c", ".air.toml"}). | ||
| WithExposedPort(8080) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up development setup and improve reliability.
Several improvements needed:
- Remove debug
lscommands - Pin goose version
- Verify Air configuration file existence
golang = golang.
WithWorkdir(PROJ_MOUNT+"/ground-control/sql/schema").
- WithExec([]string{"ls", "-la"}).
WithServiceBinding("pgservice", db).
- WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/goose@latest"}).
+ WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/[email protected]"}).
WithExec([]string{"goose", "postgres", "postgres://postgres:password@pgservice:5432/groundcontrol", "up"}).
WithWorkdir(PROJ_MOUNT + "/ground-control").
- WithExec([]string{"ls", "-la"}).
+ WithExec([]string{"test", "-f", ".air.toml"}). // Verify config exists
WithExec([]string{"go", "mod", "download"}).
WithExec([]string{"air", "-c", ".air.toml"}).
WithExposedPort(8080)📝 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.
| golang = golang. | |
| WithWorkdir(PROJ_MOUNT+"/ground-control/sql/schema"). | |
| WithExec([]string{"ls", "-la"}). | |
| WithServiceBinding("pgservice", db). | |
| WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/goose@latest"}). | |
| WithExec([]string{"goose", "postgres", "postgres://postgres:password@pgservice:5432/groundcontrol", "up"}). | |
| WithWorkdir(PROJ_MOUNT + "/ground-control"). | |
| WithExec([]string{"ls", "-la"}). | |
| WithExec([]string{"go", "mod", "download"}). | |
| WithExec([]string{"air", "-c", ".air.toml"}). | |
| WithExposedPort(8080) | |
| golang = golang. | |
| WithWorkdir(PROJ_MOUNT+"/ground-control/sql/schema"). | |
| WithServiceBinding("pgservice", db). | |
| WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/[email protected]"}). | |
| WithExec([]string{"goose", "postgres", "postgres://postgres:password@pgservice:5432/groundcontrol", "up"}). | |
| WithWorkdir(PROJ_MOUNT + "/ground-control"). | |
| WithExec([]string{"test", "-f", ".air.toml"}). // Verify config exists | |
| WithExec([]string{"go", "mod", "download"}). | |
| WithExec([]string{"air", "-c", ".air.toml"}). | |
| WithExposedPort(8080) |
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
QUICKSTART.md (1)
5-9: Enhance prerequisites section with specific requirements.The prerequisites section should include:
- Minimum system requirements (CPU, RAM, disk space)
- Required Go version
- Network requirements (ports that need to be available)
- Docker version requirements (if needed)
ci/main.go (1)
Line range hint
1-11: Fix undefineddagvariableThe
dagvariable is used throughout the code but not defined. This needs to be fixed to prevent runtime errors.Add the following at the beginning of the file:
package main import ( "context" "fmt" "log" "log/slog" "os" "strings" "container-registry.com/harbor-satellite/ci/internal/dagger" + + dag "container-registry.com/harbor-satellite/ci/internal/dagger" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
QUICKSTART.md(1 hunks)ci/main.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint
ci/main.go
32-32: undefined: dag
(typecheck)
34-34: undefined: dag
(typecheck)
🔇 Additional comments (5)
QUICKSTART.md (3)
19-32:
Replace sensitive information with secure placeholders.
The configuration contains actual credentials and demo URLs which pose security risks.
Previous review comments about securing sensitive information are still valid. Additionally:
- Add a warning about not using these credentials in production
- Document the required permission scopes for the Harbor credentials
- Add validation steps for testing the configuration
34-54: 🛠️ Refactor suggestion
Add operational validation and troubleshooting steps.
The execution instructions need additional operational details.
Previous review comments about adding health checks and troubleshooting steps are still valid. Additionally:
- Document expected output for successful execution
- Add instructions for viewing logs
- Include steps to verify the service is running correctly
64-82: 🛠️ Refactor suggestion
Enhance configuration documentation.
The configuration section needs better documentation and examples.
Previous review comments about enhancing configuration documentation are still valid. Additionally:
- Document the impact of each configuration option
- Add examples for different setup scenarios
- Include configuration validation steps
ci/main.go (2)
185-192: LGTM!
The helper function is well-implemented with proper input validation and clear error messaging.
114-124:
Improve database container configuration and security
Several improvements needed:
- Pin PostgreSQL to specific minor version
- Make credentials configurable
- Add container health check
Apply this diff:
+const (
+ POSTGRES_VERSION = "17.2"
+ POSTGRES_USER = "postgres"
+ POSTGRES_PASSWORD = "password"
+ POSTGRES_DB = "groundcontrol"
+)
func (m *HarborSatellite) Db(ctx context.Context) (*dagger.Service, error) {
return dag.Container().
- From("postgres:17").
+ From("postgres:" + POSTGRES_VERSION).
- WithEnvVariable("POSTGRES_USER", "postgres").
- WithEnvVariable("POSTGRES_PASSWORD", "password").
+ WithEnvVariable("POSTGRES_USER", POSTGRES_USER).
+ WithEnvVariable("POSTGRES_PASSWORD", POSTGRES_PASSWORD).
WithEnvVariable("POSTGRES_HOST_AUTH_METHOD", "trust").
- WithEnvVariable("POSTGRES_DB", "groundcontrol").
+ WithEnvVariable("POSTGRES_DB", POSTGRES_DB).
WithExposedPort(5432).
+ WithHealthCheck(
+ dag.Container().
+ WithExec([]string{"pg_isready", "-U", POSTGRES_USER}),
+ ).
AsService().Start(ctx)
}Likely invalid or redundant comment.
| ### 6. Register the Satellite with Ground Control | ||
| Using `curl` or Postman, make a `POST` request to register the Satellite with Ground Control: | ||
|
|
||
| ```bash | ||
| curl -X POST http://localhost:8080/satellites/register -H "Content-Type: application/json" -d '{ "name": "<satellite_name_here>" }' | ||
| ``` | ||
|
|
||
| The response will include a token string. Set this token in the Satellite `.env` file: | ||
|
|
||
| ```console | ||
| TOKEN=<string_from_ground_control> | ||
| ``` | ||
|
|
||
| ### 7. Build the Satellite | ||
| Run the following Dagger command to build the Satellite: | ||
|
|
||
| ```bash | ||
| dagger call build-dev --platform "linux/amd64" --component satellite export --path=./satellite-dev | ||
| ``` | ||
|
|
||
| To Run Satellite: | ||
| ```bash | ||
| ./satellite-dev | ||
| ``` | ||
|
|
||
| The Satellite service will start on port 9090. Ensure that the `ground_control_url` is correctly set in the Satellite configuration before launching. | ||
|
|
||
|
|
||
| ### 8. Finalize and Run | ||
| After setting the token, you can now run the Satellite. This setup will launch the Satellite in a container with the following exposed ports: | ||
| - **9090** for the Satellite service. | ||
| - **8585** for the Zot registry (if configured). | ||
|
|
||
| With this setup, your Harbor Satellite should be up and running! |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security considerations and verification steps for registration process.
The registration and execution instructions need enhancement:
- Add error handling for registration failures
- Document token security best practices
- Include steps to verify successful registration
Apply this diff:
curl -X POST http://localhost:8080/satellites/register -H "Content-Type: application/json" -d '{ "name": "<satellite_name_here>" }'
+
+# Verify registration status
+curl -X GET http://localhost:8080/satellites/status
+
+# Store token securely (do not commit to version control)
+echo "TOKEN=<string_from_ground_control>" > .env
+chmod 600 .envAdd a security note:
> **Security Note:** The registration token grants access to Ground Control. Store it securely and never commit it to version control. Rotate tokens periodically following security best practices.| golang := dag.Container(). | ||
| From("golang:latest"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithEnvVariable("GOCACHE", "/go/build-cache"). | ||
| WithMountedDirectory(PROJ_MOUNT, source). | ||
| WithWorkdir(PROJ_MOUNT). | ||
| WithExec([]string{"go", "install", "github.com/air-verse/air@latest"}) | ||
|
|
There was a problem hiding this comment.
Pin container and tool versions for reproducibility
Several improvements needed for container setup:
- Pin Golang version instead of using "latest"
- Pin Air tool version
- Add Air config validation
Apply this diff:
golang := dag.Container().
- From("golang:latest").
+ From(DEFAULT_GO). // Using the constant defined above
WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")).
WithEnvVariable("GOMODCACHE", "/go/pkg/mod").
WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")).
WithEnvVariable("GOCACHE", "/go/build-cache").
WithMountedDirectory(PROJ_MOUNT, source).
WithWorkdir(PROJ_MOUNT).
- WithExec([]string{"go", "install", "github.com/air-verse/air@latest"})
+ WithExec([]string{"go", "install", "github.com/air-verse/[email protected]"}).
+ WithExec([]string{"which", "air"}) // Verify installationCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint
32-32: undefined: dag
(typecheck)
34-34: undefined: dag
(typecheck)
| fmt.Println("🛠️ Building Harbor-Cli with Dagger...") | ||
| // Define the path for the binary output | ||
| os, arch, err := parsePlatform(platform) | ||
| if err != nil { | ||
| log.Fatalf("Error parsing platform: %v", err) | ||
| } |
There was a problem hiding this comment.
Improve error handling and messaging
The error handling could be improved:
- Don't use
log.Fatalfas it terminates the program - Return the error to the caller for proper handling
Apply this diff:
- fmt.Println("🛠️ Building Harbor-Cli with Dagger...")
+ fmt.Printf("🛠️ Building %s with Dagger...\n", component)
// Define the path for the binary output
os, arch, err := parsePlatform(platform)
if err != nil {
- log.Fatalf("Error parsing platform: %v", err)
+ return nil, fmt.Errorf("error parsing platform: %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.
| fmt.Println("🛠️ Building Harbor-Cli with Dagger...") | |
| // Define the path for the binary output | |
| os, arch, err := parsePlatform(platform) | |
| if err != nil { | |
| log.Fatalf("Error parsing platform: %v", err) | |
| } | |
| fmt.Printf("🛠️ Building %s with Dagger...\n", component) | |
| // Define the path for the binary output | |
| os, arch, err := parsePlatform(platform) | |
| if err != nil { | |
| return nil, fmt.Errorf("error parsing platform: %w", err) | |
| } |
| golang = golang. | ||
| WithWorkdir(PROJ_MOUNT+"/ground-control/sql/schema"). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithServiceBinding("pgservice", db). | ||
| WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/goose@latest"}). | ||
| WithExec([]string{"goose", "postgres", "postgres://postgres:password@pgservice:5432/groundcontrol", "up"}). | ||
| WithWorkdir(PROJ_MOUNT + "/ground-control"). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"air", "-c", ".air.toml"}). | ||
| WithExposedPort(8080) |
There was a problem hiding this comment.
Clean up development setup and improve error handling
Several improvements needed:
- Remove debug
lscommands - Extract database connection string to a constant
- Pin goose version
- Add Air config validation
Apply this diff:
golang = golang.
WithWorkdir(PROJ_MOUNT+"/ground-control/sql/schema").
- WithExec([]string{"ls", "-la"}).
WithServiceBinding("pgservice", db).
- WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/goose@latest"}).
+ WithExec([]string{"go", "install", "github.com/pressly/goose/v3/cmd/[email protected]"}).
+ WithExec([]string{"test", "-f", "migrations"}). // Verify migrations exist
WithExec([]string{"goose", "postgres", "postgres://postgres:password@pgservice:5432/groundcontrol", "up"}).
WithWorkdir(PROJ_MOUNT + "/ground-control").
- WithExec([]string{"ls", "-la"}).
+ WithExec([]string{"test", "-f", ".air.toml"}). // Verify config exists
WithExec([]string{"go", "mod", "download"}).
WithExec([]string{"air", "-c", ".air.toml"}).
WithExposedPort(8080)Committable suggestion skipped: line range outside the PR's diff.
| if component == "satellite" || component == "ground-control" { | ||
| var binaryFile *dagger.File | ||
| golang := dag.Container(). | ||
| From("golang:latest"). | ||
| WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")). | ||
| WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). | ||
| WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")). | ||
| WithEnvVariable("GOCACHE", "/go/build-cache"). | ||
| WithMountedDirectory(PROJ_MOUNT, source). | ||
| WithWorkdir(PROJ_MOUNT). | ||
| WithEnvVariable("GOOS", os). | ||
| WithEnvVariable("GOARCH", arch) | ||
|
|
||
| if component == "ground-control" { | ||
| golang = golang. | ||
| WithWorkdir(PROJ_MOUNT + "/ground-control"). | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"go", "build", "."}) | ||
|
|
||
| binaryFile = golang.File(PROJ_MOUNT + "/ground-control/ground-control") | ||
| } else { | ||
| golang = golang. | ||
| WithExec([]string{"ls", "-la"}). | ||
| WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"go", "build", "."}) | ||
|
|
||
| binaryFile = golang.File(PROJ_MOUNT + "/harbor-satellite") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor container setup and remove debug commands
The code has several issues:
- Duplicated container setup logic
- Debug
lscommands - Unpinned golang version
Extract common container setup to a helper method and clean up the code:
+func setupGolangContainer(source *dagger.Directory, os, arch string) *dagger.Container {
+ return dag.Container().
+ From(DEFAULT_GO).
+ WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")).
+ WithEnvVariable("GOMODCACHE", "/go/pkg/mod").
+ WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")).
+ WithEnvVariable("GOCACHE", "/go/build-cache").
+ WithMountedDirectory(PROJ_MOUNT, source).
+ WithWorkdir(PROJ_MOUNT).
+ WithEnvVariable("GOOS", os).
+ WithEnvVariable("GOARCH", arch)
+}
if component == "satellite" || component == "ground-control" {
var binaryFile *dagger.File
- golang := dag.Container().
- From("golang:latest").
- WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod")).
- WithEnvVariable("GOMODCACHE", "/go/pkg/mod").
- WithMountedCache("/go/build-cache", dag.CacheVolume("go-build")).
- WithEnvVariable("GOCACHE", "/go/build-cache").
- WithMountedDirectory(PROJ_MOUNT, source).
- WithWorkdir(PROJ_MOUNT).
- WithEnvVariable("GOOS", os).
- WithEnvVariable("GOARCH", arch)
+ golang := setupGolangContainer(source, os, arch)
if component == "ground-control" {
golang = golang.
WithWorkdir(PROJ_MOUNT + "/ground-control").
- WithExec([]string{"ls", "-la"}).
WithExec([]string{"go", "mod", "download"}).
WithExec([]string{"go", "build", "."})
binaryFile = golang.File(PROJ_MOUNT + "/ground-control/ground-control")
} else {
golang = golang.
- WithExec([]string{"ls", "-la"}).
WithExec([]string{"go", "mod", "download"}).
WithExec([]string{"go", "build", "."})
binaryFile = golang.File(PROJ_MOUNT + "/harbor-satellite")
}Committable suggestion skipped: line range outside the PR's diff.
Fixes: #43
This PR focuses on creating a quickstart guide on how to setup ground-control and satellite in local dev environments.
and simplifies the setup with dagger by creating local dev servers.
Currently, there is a known limitation in dagger with which we can't have live reloading of the dev server.
Dagger Limitation: dagger/dagger#6990
When the above Limitation gets resolved we can have the live reloading in dagger.
Note
We need to rerun
dagger call build-dev --component ground-control upto reflect the changes in the dev server.Summary by CodeRabbit
New Features
Documentation
QUICKSTART.mdwith a comprehensive guide for setting up Harbor Satellite locally.README.mdfor clarity, including a new "QuickStart" section and corrections for typographical errors.Bug Fixes
Chores
go.modfor improved performance and compatibility..envfor the ground-control project.