adds workflow to to create sign and publish the images #67
Conversation
WalkthroughThe changes introduce significant enhancements to the Harbor Satellite project, particularly in the areas of image publishing and signing. New methods for publishing and signing container images have been added to the Changes
Possibly related PRs
Suggested labels
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 (
|
![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 (4)
ci/publish_image.go (1)
118-118: Fix print statement to log the correct signed imageThe print statement logs the entire slice
imageAddrsinstead of the single signed image address.Apply this diff to log the correct image address:
- fmt.Printf("Signed image: %s\n", imageAddrs) + fmt.Printf("Signed image: %s\n", imageAddrs[i])ci/utils.go (1)
86-86: Clean up unnecessary blank lineThere's an unnecessary blank line that can be removed for better code readability.
Apply this diff:
-go.mod (2)
Line range hint
208-211: Verify unusual version numbers inreplacedirectivesThe
replacedirectives specify versions with future timestamps, which is unconventional and may cause dependency resolution issues.Lines in question:
replace go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc => go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc v0.0.0-20240518090000-14441aefdf88 replace go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp => go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp v0.3.0 replace go.opentelemetry.io/otel/log => go.opentelemetry.io/otel/log v0.3.0 replace go.opentelemetry.io/otel/sdk/log => go.opentelemetry.io/otel/sdk/log v0.3.0Ensure that these versions are correct and that the modules exist at these versions. If they are placeholders or mistakenly added, update them to the appropriate versions.
Line range hint
212-212: Update the minimum required Go versionThe
godirective specifiesgo 1.22.5, but your toolchain is set togo1.23.2. For consistency, consider updating thegodirective.Apply this diff:
-go 1.22.5 +go 1.23This ensures that the module requires at least Go 1.23, matching your toolchain.
📜 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 (5)
.github/actionns/publish-and-sign.yml(1 hunks)ci/publish_image.go(1 hunks)ci/utils.go(1 hunks)go.mod(1 hunks)secrets/registry.txt(1 hunks)
🔇 Additional comments (3)
ci/publish_image.go (1)
121-121: Confirm returning only the first image address is intended
The function returns imageAddrs[0], which is the first image address. If multiple images are built and signed, ensure that returning only the first address meets the requirements.
Would you like to return all image addresses instead? For example:
-return imageAddrs[0], nil
+return imageAddrs, nilUpdate the function signature accordingly if you decide to return all addresses.
.github/actionns/publish-and-sign.yml (2)
35-35: Verify the version of sigstore/cosign-installer
The workflow uses sigstore/[email protected]. Ensure that this version meets your requirements and consider updating to the latest stable version if needed.
Run the following script to check for the latest version:
Ensure that v3.7.0 is the desired version.
✅ Verification successful
Version v3.7.0 of sigstore/cosign-installer is up to date
The workflow is using the latest stable version (v3.7.0) of the sigstore/cosign-installer action, as confirmed by checking the latest release from the official repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the latest release of sigstore/cosign-installer
curl -s https://api.github.com/repos/sigstore/cosign-installer/releases/latest | jq '.tag_name'
Length of output: 104
44-44: Verify the version of dagger/dagger-for-github
The workflow uses dagger/dagger-for-github@v7. Confirm that this is the correct version and update if necessary.
Run the following script to check for the latest version:
Ensure that v7 aligns with your project's requirements.
✅ Verification successful
The workflow uses the latest major version of dagger/dagger-for-github
The current version v7 in the workflow matches the latest major version available (v7.0.1). Using the major version tag (v7) is a good practice as it automatically includes bug fixes and minor updates while maintaining compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the latest release of dagger/dagger-for-github
curl -s https://api.github.com/repos/dagger/dagger-for-github/releases/latest | jq '.tag_name'
Length of output: 103
| ) (string, error) { | ||
| registryPasswordPlain, _ := registryPassword.Plaintext(ctx) | ||
|
|
||
| cosing_ctr := dag.Container().From("cgr.dev/chainguard/cosign") |
There was a problem hiding this comment.
Correct variable name from cosing_ctr to cosign_ctr
The variable cosing_ctr appears to be misspelled. It should likely be cosign_ctr to reflect its purpose.
Apply this diff to fix the typo:
-cosing_ctr := dag.Container().From("cgr.dev/chainguard/cosign")
+cosign_ctr := dag.Container().From("cgr.dev/chainguard/cosign")Ensure all subsequent references are updated accordingly:
- cosing_ctr = cosing_ctr.WithSecretVariable("GITHUB_TOKEN", githubToken)...
+ cosign_ctr = cosign_ctr.WithSecretVariable("GITHUB_TOKEN", githubToken)...Also applies to: 146-146
| case component == "ground-control": | ||
| directory = source.Directory("ground-control") | ||
| default: | ||
| panic(fmt.Sprintf("unknown component: %s", component)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace panic calls with proper error handling
Using panic for error handling can cause the entire application to exit unexpectedly. It's better to return errors and handle them gracefully.
For example, modify the code as follows:
// Line 31
- panic(fmt.Sprintf("unknown component: %s", component))
+ return nil, fmt.Errorf("unknown component: %s", component)
// Line 62
- panic(fmt.Sprintf("failed to get plaintext: %v", err))
+ return nil, fmt.Errorf("failed to get plaintext: %w", err)
// Line 76
- panic(fmt.Sprintf("failed to publish image: %v", err))
+ return nil, fmt.Errorf("failed to publish image: %w", err)
// Line 190
- panic(fmt.Sprintf("failed to get stderr: %v", err))
+ return nil, fmt.Errorf("failed to get stderr: %w", err)Ensure that the function signatures are updated to return errors where necessary, and handle these errors appropriately in calling functions.
Also applies to: 62-62, 76-76, 190-190
ci/publish_image.go
Outdated
| if err != nil { | ||
| panic(fmt.Sprintf("failed to get plaintext: %v", err)) | ||
| } | ||
| fmt.Printf("Password: %s", password) |
There was a problem hiding this comment.
Avoid logging sensitive information
Printing the registry password to the console poses a security risk by exposing sensitive credentials.
Apply this diff to remove the sensitive log statement:
-fmt.Printf("Password: %s", password)Additionally, consider reviewing other log statements to ensure that no sensitive information (like registryUsername or registry) is unnecessarily exposed.
ci/publish_image.go
Outdated
| if os != "linux" || "os" != "darwin" { | ||
| continue |
There was a problem hiding this comment.
Fix the logical condition to correctly filter operating systems
The condition if os != "linux" || "os" != "darwin" will always evaluate to true, causing unintended behavior where the loop might continue prematurely.
Apply this diff to correct the condition:
-if os != "linux" || "os" != "darwin" {
+if os != "linux" && os != "darwin" {This change ensures that the loop continues only when os is neither "linux" nor "darwin".
📝 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.
| if os != "linux" || "os" != "darwin" { | |
| continue | |
| if os != "linux" && os != "darwin" { | |
| continue |
ci/publish_image.go
Outdated
| registryPassword *dagger.Secret, | ||
| imageAddr string, | ||
| ) (string, error) { | ||
| registryPasswordPlain, _ := registryPassword.Plaintext(ctx) |
There was a problem hiding this comment.
Handle errors when retrieving plaintext password
The error returned by registryPassword.Plaintext(ctx) is currently ignored, which may lead to unexpected behavior if an error occurs.
Apply this diff to handle the error properly:
-registryPasswordPlain, _ := registryPassword.Plaintext(ctx)
+registryPasswordPlain, err := registryPassword.Plaintext(ctx)
+if err != nil {
+ return "", fmt.Errorf("failed to get plaintext password: %w", err)
+}This ensures that any errors are caught and handled appropriately.
📝 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.
| registryPasswordPlain, _ := registryPassword.Plaintext(ctx) | |
| registryPasswordPlain, err := registryPassword.Plaintext(ctx) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to get plaintext password: %w", err) | |
| } |
secrets/registry.txt
Outdated
| @@ -0,0 +1 @@ | |||
| Harbor12345 | |||
There was a problem hiding this comment.
Avoid storing secrets in plaintext files
Storing sensitive information like passwords in plaintext files can lead to security vulnerabilities if the file is exposed.
Consider using a secure method for managing secrets, such as environment variables, encrypted secret managers, or dedicated secret storage solutions.
| @@ -0,0 +1,63 @@ | |||
| name: Publish and Sign Snapshot Image | |||
There was a problem hiding this comment.
Correct the directory name in the file path
The directory actionns in the file path .github/actionns/publish-and-sign.yml appears to be misspelled.
Rename the directory to actions to match standard GitHub Actions conventions:
-.github/actionns/publish-and-sign.yml
+.github/actions/publish-and-sign.yml📝 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.
| name: Publish and Sign Snapshot Image | |
| name: Publish and Sign Snapshot Image |
| // builds given component from source | ||
| func (m *HarborSatellite) build(source *dagger.Directory, component string) *dagger.Directory { | ||
| fmt.Printf("Building %s\n", component) | ||
| // Fetch supported builds (GOOS and GOARCH combinations) | ||
| supportedBuilds := getSupportedBuilds() |
There was a problem hiding this comment.
Undefined function getSupportedBuilds
The function getSupportedBuilds() is called but not defined or imported in this file, which will result in a compilation error.
Ensure that getSupportedBuilds() is accessible in this context. You can:
- Define the function within
ci/utils.go. - Import it from another package if it exists elsewhere.
If it's defined in ci/publish_image.go, consider moving it to a shared utility package that both files can access.
secrets/registry.txt
Outdated
There was a problem hiding this comment.
@Mehul-Kumar-27 why are we using this.?
There was a problem hiding this comment.
No, fixed this
Mehul-Kumar-27
force-pushed
the
release-image
branch
from
8b99065 to
4872ac4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
test/e2e/satellite_test.go (2)
Line range hint
132-136: Consider enhancing error handling for config file loadingWhile the transition to JSON configuration looks good, consider adding explicit validation for the JSON configuration file content before executing the binary.
Consider adding validation before binary execution:
WithFile("./config.json", configFile). +WithExec([]string{"sh", "-c", "test -f config.json && jq . config.json >/dev/null"}). WithExec([]string{"go", "build", "-o", appBinary, sourceFile}).
Line range hint
132-145: Consider adding timeout for satellite service startupThe test might benefit from adding a brief delay or retry mechanism to ensure the satellite service is fully operational before making the ping request.
Consider adding a retry mechanism for the health check:
WithExec([]string{"./" + appBinary}). AsService() +// Add retry logic for service availability +satellite_health_check := client.Container(). + From("golang:alpine"). + WithServiceBinding("satellite", container). + WithExec([]string{"sh", "-c", "for i in $(seq 1 30); do wget -q -O /dev/null http://satellite:9090/api/v1/satellite/ping && exit 0; sleep 1; done; exit 1"}) + +_, err = satellite_health_check.Sync(ctx) +assert.NoError(t, err, "Satellite service failed to become ready") satellite_container := client.Container()..github/workflows/publish_image.yml (1)
1-14: Fix typo in workflow name and improve input descriptionsThe workflow name has a typo in the PR title ("to to"). Also, the input descriptions could be more descriptive.
Apply these improvements:
-name: Publish Image +name: Publish and Sign Images on: workflow_dispatch: inputs: use-registry-password: - description: 'Use registry password to sign the image' + description: 'Enable image signing using registry password authentication' default: true type: boolean actions-id-token-request-url: - description: 'Actions ID token request URL' + description: 'URL for requesting GitHub Actions OIDC token (leave empty to use registry password)' default: '' type: string.github/workflows/build.yaml (2)
Line range hint
47-78: Standardize job names and reduce duplication in build stepsThe job names use different naming conventions (Satellite vs GroundControl). Also, similar to the publish workflow, there's significant duplication between the Satellite and GroundControl jobs.
Refactor using a matrix strategy and consistent naming:
jobs: - Satellite: - GroundControl: + build-and-release: needs: run-tests runs-on: ubuntu-latest + strategy: + matrix: + component: [satellite, ground-control] permissions: contents: write packages: write steps: - name: Checkout repository uses: actions/checkout@v4 - - name: Build the binary for the satellite + - name: Build the binary for ${{ matrix.component }} uses: dagger/dagger-for-github@v5 with: version: latest verb: call - args: build --component=satellite + args: build --component=${{ matrix.component }} - name: Release on Github uses: dagger/dagger-for-github@v5 with: version: latest verb: call - args: release --component=satellite --github-token=env:GITHUB_TOKEN + args: release --component=${{ matrix.component }} --github-token=env:GITHUB_TOKENAlso applies to: 79-111
10-11: Consider removing redundant GITHUB_TOKEN environment variableThe GITHUB_TOKEN is already automatically available in GitHub Actions and can be accessed directly using
${{ secrets.GITHUB_TOKEN }}. Setting it as an environment variable is redundant.-env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}ground-control/.goreleaser.yaml (1)
102-128: Consider adding conventional commit types to changelog groupsThe changelog groups could be expanded to include more conventional commit types for better categorization.
- title: "New Features" - regexp: '^.*?feat(\(.+\))??!?:.+$' + regexp: '^.*?(feat|feature|add)(\(.+\))??!?:.+$' order: 100 - title: "Bug fixes" - regexp: '^.*?(fix|refactor)(\(.+\))??!?:.+$' + regexp: '^.*?(fix|bugfix|hotfix|refactor)(\(.+\))??!?:.+$' order: 200.goreleaser.yaml (1)
60-69: Consider adding package-specific metadataWhile the NFPMS configuration is good, consider adding package-specific metadata like dependencies, conflicts, or recommendations for better package management.
nfpms: - homepage: https://github.com/container-registry/harbor-satellite maintainer: Harbor Community description: |- Harbor Satellite is a lightweight, secure, and easy-to-use container registry that can be deployed in a few minutes on remote edge locations. formats: - rpm - deb - apk - archlinux + dependencies: + - docker-ce + recommends: + - cosignci/utils.go (1)
111-112: Consider using structured loggingReplace fmt.Println with structured logging for better observability and log aggregation.
-fmt.Println("Failed to get tags: ", err.Error(), ".") -fmt.Println("Get Tags Output: ", getTagsOutput, ".") +slog.Error("Failed to get tags", + "error", err, + "output", getTagsOutput)ci/publish_image.go (1)
202-218: Consider adding build caching optimizationThe build process could benefit from additional caching strategies for better performance.
builder := dag.Container(). From(DEFAULT_GO+"-alpine"). WithMountedCache("/go/pkg/mod", dag.CacheVolume("go-mod-"+GO_VERSION)). WithEnvVariable("GOMODCACHE", "/go/pkg/mod"). WithMountedCache("/go/build-cache", dag.CacheVolume("go-build-"+GO_VERSION)). + WithMountedCache("/root/.cache/go-build", dag.CacheVolume("go-build-root-"+GO_VERSION)). WithEnvVariable("GOCACHE", "/go/build-cache").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/build.yaml(5 hunks).github/workflows/publish_image.yml(1 hunks).goreleaser.yaml(2 hunks)ci/main.go(2 hunks)ci/publish_image.go(1 hunks)ci/utils.go(4 hunks)ground-control/.goreleaser.yaml(2 hunks)test/e2e/config.go(1 hunks)test/e2e/satellite_test.go(1 hunks)test/e2e/testdata/config.json(1 hunks)test/e2e/testdata/config.toml(0 hunks)
💤 Files with no reviewable changes (1)
- test/e2e/testdata/config.toml
✅ Files skipped from review due to trivial changes (1)
- test/e2e/testdata/config.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/main.go
🔇 Additional comments (9)
test/e2e/config.go (1)
9-10: Verify JSON configuration file paths
The transition from TOML to JSON configuration looks good, but let's ensure all dependencies are in place.
✅ Verification successful
Let me check the content of the remaining TOML references to ensure they are not related to the configuration being modified.
Configuration path changes verified successfully
The JSON configuration file exists at the specified path, and the remaining TOML references in the codebase are unrelated to this change:
.air.tomlin ci/main.go is for the Air live reload tool- TOML files in container_runtime are for containerd configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new JSON config files exist
echo "Checking for config files..."
fd "config.json" test/e2e/testdata/
# Check for any remaining TOML references in the codebase
echo "Checking for remaining TOML references..."
rg -l "\.toml" --type go
Length of output: 329
Script:
#!/bin/bash
# Check the context of remaining TOML references
echo "Content from ci/main.go:"
rg -C 2 "\.toml" ci/main.go
echo -e "\nContent from cmd/container_runtime/host.go:"
rg -C 2 "\.toml" cmd/container_runtime/host.go
echo -e "\nContent from cmd/container_runtime/containerd.go:"
rg -C 2 "\.toml" cmd/container_runtime/containerd.go
Length of output: 2694
.goreleaser.yaml (3)
28-36: LGTM: Comprehensive architecture support added
The addition of multiple architectures enhances platform compatibility and aligns with industry standards for broad platform support.
38-55: LGTM: Proper Darwin exclusions configured
The Darwin exclusions are correctly configured for unsupported architectures, preventing build attempts for incompatible platforms.
106-125: LGTM: Well-structured changelog grouping
The changelog groups are well-organized with clear categorization and proper ordering of different types of changes.
ci/utils.go (2)
64-65: LGTM: Improved build configuration management
The integration of getSupportedBuilds function improves maintainability by centralizing build configuration.
177-182: LGTM: Well-structured build configuration
The getSupportedBuilds function provides a clear and maintainable way to manage supported platform configurations.
ci/publish_image.go (3)
151-154: LGTM: Proper timeout handling
Good practice setting a context timeout for the signing process to prevent hanging operations.
32-32:
Replace panic with proper error handling
Using panic for error handling can cause the application to exit unexpectedly. Return errors instead.
Also applies to: 52-52, 56-56, 72-72, 82-82, 224-224
160-160:
Fix variable naming typo
The variable cosing_ctr appears to be misspelled. It should be cosign_ctr.
Also applies to: 180-182
| - name: Publish Image for Ground Control | ||
| uses: dagger/dagger-for-github@v7 | ||
| with: | ||
| verb: call | ||
| args: "publish-image-and-sign \ | ||
| --registry='${{ env.REGISTRY }}' \ | ||
| --registry-username='${{ env.REGISTRY_USERNAME }}' \ | ||
| --registry-password=env:REGISTRY_PASSWORD \ | ||
| --github-token=env:GITHUB_TOKEN \ | ||
| --component='${{ env.GROUND_CONTROL }}' \ | ||
| --actions-id-token-request-url='${{ env.ACTIONS_ID_TOKEN_REQUEST_URL }}' \ | ||
| --actions-id-token-request-token=env:ACTIONS_ID_TOKEN_REQUEST_TOKEN \ | ||
| --use-registry-password=${{ env.USE_REGISTRY_PASSWORD }}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in publish steps
The publish steps for Ground Control and Satellite are identical except for the component name. Consider using a matrix strategy or reusable workflow to reduce duplication and improve maintainability.
Here's how to refactor using a matrix strategy:
jobs:
publish:
runs-on: ubuntu-latest
+ strategy:
+ matrix:
+ component: [ground-control, satellite]
steps:
- name: Checkout repository
uses: actions/checkout@v4
- - name: Publish Image for Ground Control
+ - name: Publish Image for ${{ matrix.component }}
uses: dagger/dagger-for-github@v7
with:
verb: call
args: "publish-image-and-sign \
--registry='${{ env.REGISTRY }}' \
--registry-username='${{ env.REGISTRY_USERNAME }}' \
--registry-password=env:REGISTRY_PASSWORD \
--github-token=env:GITHUB_TOKEN \
- --component='${{ env.GROUND_CONTROL }}' \
+ --component='${{ matrix.component }}' \
--actions-id-token-request-url='${{ env.ACTIONS_ID_TOKEN_REQUEST_URL }}' \
--actions-id-token-request-token=env:ACTIONS_ID_TOKEN_REQUEST_TOKEN \
--use-registry-password=${{ env.USE_REGISTRY_PASSWORD }}"
- - name: Publish Image for Satellite
- # Remove duplicate stepAlso applies to: 48-60
| env: | ||
| # Non-secret environment variables | ||
| REGISTRY: ${{ secrets.REGISTRY }} | ||
| GROUND_CONTROL: ground-control | ||
| SATELLITE: satellite | ||
| REGISTRY_PASSWORD: ${{ secrets.REGISTRY_PASSWORD }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| REGISTRY_USERNAME: ${{ secrets.REGISTRY_USERNAME }} | ||
| ACTIONS_ID_TOKEN_REQUEST_TOKEN: ${{ secrets.ACTIONS_ID_TOKEN_REQUEST_TOKEN }} | ||
| ACTIONS_ID_TOKEN_REQUEST_URL: ${{ secrets.ACTIONS_ID_TOKEN_REQUEST_URL }} | ||
| USE_REGISTRY_PASSWORD: ${{ github.event.inputs.use-registry-password }} | ||
|
|
There was a problem hiding this comment.
Add explicit permissions to the workflow
The workflow is accessing sensitive resources but doesn't declare explicit permissions. This is a security best practice to follow the principle of least privilege.
Add the following permissions block:
jobs:
publish:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
+ packages: write # for publishing images
+ id-token: write # for signing images
steps:Committable suggestion skipped: line range outside the PR's diff.
| nfpms: | ||
| - | ||
| homepage: https://github.com/container-registry/harbor-satellite | ||
| maintainer: Harbor Community | ||
| description: |- | ||
| Ground Control is a tool to manage Harbor Satellite instances. | ||
| formats: | ||
| - rpm | ||
| - deb | ||
| - apk | ||
| - archlinux |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add package signing configuration for nfpms
The nfpms configuration is missing package signing settings, which is important for package authenticity verification.
nfpms:
-
homepage: https://github.com/container-registry/harbor-satellite
maintainer: Harbor Community
description: |-
Ground Control is a tool to manage Harbor Satellite instances.
formats:
- rpm
- deb
- apk
- archlinux
+ sign:
+ cmd: cosign
+ args: ["sign-blob", "--key", "env:COSIGN_KEY", "${artifact}", "--output-signature", "${signature}"]📝 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.
| nfpms: | |
| - | |
| homepage: https://github.com/container-registry/harbor-satellite | |
| maintainer: Harbor Community | |
| description: |- | |
| Ground Control is a tool to manage Harbor Satellite instances. | |
| formats: | |
| - rpm | |
| - deb | |
| - apk | |
| - archlinux | |
| nfpms: | |
| - | |
| homepage: https://github.com/container-registry/harbor-satellite | |
| maintainer: Harbor Community | |
| description: |- | |
| Ground Control is a tool to manage Harbor Satellite instances. | |
| formats: | |
| - rpm | |
| - deb | |
| - apk | |
| - archlinux | |
| sign: | |
| cmd: cosign | |
| args: ["sign-blob", "--key", "env:COSIGN_KEY", "${artifact}", "--output-signature", "${signature}"] |
| - ppc64le | ||
| - s390x | ||
| - "386" | ||
| - loong64 | ||
| - mips64 | ||
| - mips64le | ||
| - mips | ||
| - mipsle | ||
| - riscv64 |
There was a problem hiding this comment.
💡 Codebase verification
Some architectures in .goreleaser.yaml are not supported by the build system
The CI system only supports a subset of the architectures listed in .goreleaser.yaml:
- CI supports: linux (amd64, arm64, ppc64le, s390x, 386, riscv64) and darwin (amd64, arm64)
- .goreleaser.yaml includes additional architectures: loong64, mips64, mips64le, mips, mipsle
The codebase:
- Has no CGO dependencies
- No platform-specific code or build tags
- No direct runtime architecture checks
Remove the unsupported architectures from .goreleaser.yaml to match the CI's supported builds:
- loong64
- mips64
- mips64le
- mips
- mipsle
🔗 Analysis chain
Verify architecture support for added platforms
The configuration adds support for many new architectures, but we need to verify that the codebase is compatible with these architectures and that they are properly tested.
Also applies to: 40-57
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for architecture-specific code or dependencies
echo "Checking for architecture-specific code..."
rg -l 'unsafe|asm|386|amd64|arm|syscall'
echo "Checking for CGO dependencies..."
rg -l '#cgo|C\.'
echo "Checking build tags..."
rg -l '^//\s*\+build'
Length of output: 592
Script:
#!/bin/bash
# Let's examine the actual content of files that might have architecture-specific code
echo "=== Checking utils.go for architecture specifics ==="
cat internal/utils/utils.go
echo -e "\n=== Checking adapter_test.go for architecture specifics ==="
cat ground-control/reg/adapter_test.go
echo -e "\n=== Checking ci/utils.go for architecture specifics ==="
cat ci/utils.go
echo -e "\n=== Checking ci/main.go for architecture specifics ==="
cat ci/main.go
echo -e "\n=== Checking Go module dependencies ==="
cat go.mod
Length of output: 44227
Script:
#!/bin/bash
# Let's check the build configuration and supported platforms in CI
echo "=== Checking CI build configurations ==="
cat ci/utils.go | grep -A 10 "getSupportedBuilds()"
echo -e "\n=== Checking for CGO usage ==="
rg -l 'import\s+"C"'
echo -e "\n=== Checking for platform-specific code ==="
# Looking for build tags and platform-specific imports
rg -g '*.go' '//\s*\+build' || true
rg -g '*.go' 'runtime.GOOS|runtime.GOARCH' || true
Length of output: 866
| return cosing_ctr.WithEnvVariable("REGISTRY_PASSWORD", registryPasswordPlain). | ||
| WithExec([]string{"cosign", "env"}). | ||
| WithExec([]string{"cosign", "sign", "--yes", "--recursive", | ||
| "--registry-username", registryUsername, | ||
| "--registry-password", registryPasswordPlain, | ||
| imageAddr, | ||
| "--timeout", "1m", | ||
| }).Stdout(ctx) |
There was a problem hiding this comment.
Avoid exposing sensitive information in command arguments
The registry password is exposed in the command arguments, which could be logged or visible in process lists.
- "--registry-password", registryPasswordPlain,
+ "--registry-password-file", "/run/secrets/registry-password",Consider using a file-based approach for sensitive credentials.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores