chore: daggerize #55
chore: daggerize #55
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
.dagger/main.go (1)
116-155: Refactor duplicated code inPublishAllImagesandPublishImagefunctionsThe functions
PublishAllImages(lines 116-155) andPublishImage(lines 157-197) contain similar logic, leading to code duplication. This can make maintenance more challenging and increase the risk of inconsistent behavior.Consider extracting the common functionality into a shared helper function or method to reduce duplication and improve maintainability.
Also applies to: 157-197
🧰 Tools
🪛 golangci-lint (1.62.2)
140-140: undefined: dag
(typecheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
.dagger/go.sumis excluded by!**/*.sumsrc/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.dagger/.gitattributes(1 hunks).dagger/.gitignore(1 hunks).dagger/go.mod(1 hunks).dagger/main.go(1 hunks).gitignore(1 hunks).golangci.yaml(1 hunks)dagger.json(1 hunks)src/core/main.go(1 hunks)src/go.mod(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- .dagger/.gitattributes
- dagger.json
- .dagger/.gitignore
- src/core/main.go
- .gitignore
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
93-93: undefined: dag
(typecheck)
140-140: undefined: dag
(typecheck)
184-184: undefined: dag
(typecheck)
🔇 Additional comments (4)
src/go.mod (2)
81-84: Review new dependencies for Dagger integration
The addition of golang.org/x/crypto/x509roots/fallback suggests certificate handling requirements. Please ensure:
- This is a required dependency for Dagger integration
- The version pinning (
0241127184453-8c4e668694cc) is stable and secure
24-24: Verify direct usage of go-openapi packages
The removal of // indirect comments for go-openapi packages suggests they are now directly imported. Please ensure these dependencies are actually used directly in the codebase, particularly in relation to the Dagger integration.
Also applies to: 26-26, 29-29
✅ Verification successful
The go-openapi packages are directly used in the codebase
The search results confirm extensive direct usage of go-openapi packages throughout the codebase:
go-openapi/runtime/middlewareis used in numerous handler files underserver/v2.0/handler/go-openapi/strfmtis used in model definitions and handlersgo-openapi/errorsis used in error handlinggo-openapi/swagis used in registry implementation
The removal of // indirect comments is correct as these dependencies are indeed directly imported and used in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify direct usage of go-openapi packages
# Search for direct imports of the packages
echo "Checking for direct imports of go-openapi packages..."
rg --type go "^import.*go-openapi/(loads|spec|validate)"
Length of output: 175
Script:
#!/bin/bash
# Let's try a broader search for go-openapi packages without restricting to import statements
echo "Searching for any usage of go-openapi packages..."
rg --type go "go-openapi/(loads|spec|validate)" -A 2
# Also check for any other go-openapi packages that might be used
echo -e "\nChecking all go-openapi related imports..."
rg --type go "go-openapi/"
# Let's also check if there are any .go files that might contain OpenAPI/Swagger related code
echo -e "\nLooking for potential OpenAPI/Swagger related files..."
fd -e go -x grep -l -i "openapi\|swagger" {}
Length of output: 9372
.golangci.yaml (1)
1-17: LGTM!
The GolangCI-Lint configuration is properly set up with relevant linters and appropriate exclusions.
.dagger/go.mod (1)
10-11:
Verify the versions of OpenTelemetry dependencies and replace directives
The dependencies and replace directives include versions with future timestamps, such as v0.0.0-20240518090000-14441aefdf88. These versions may not be valid or available, leading to issues during dependency resolution.
Please confirm that the specified versions are correct and accessible. If these are placeholders or intended for future releases, update them to use the appropriate existing versions to ensure compatibility.
Also applies to: 16-21, 44-50
| var imageAddresses []string | ||
| for pkg, imgs := range platformVariantsContainer { | ||
| for _, imageTag := range imageTags { | ||
| container := dag.Container().WithRegistryAuth(registry, registryUsername, registryPassword) |
There was a problem hiding this comment.
Fix the undefined dag identifier in multiple locations
The variable dag is also undefined at lines 140 and 184, causing compilation errors. Replace dag with the correct package or variable name.
Apply these diffs to fix the issues:
At line 140:
- container := dag.Container().WithRegistryAuth(registry, registryUsername, registryPassword)
+ container := dagger.Container().WithRegistryAuth(registry, registryUsername, registryPassword)At line 184:
- addr, err := dag.Container().WithRegistryAuth(registry, registryUsername, registryPassword).
+ addr, err := dagger.Container().WithRegistryAuth(registry, registryUsername, registryPassword).Also applies to: 184-184
🧰 Tools
🪛 golangci-lint (1.62.2)
140-140: undefined: dag
(typecheck)
.dagger/main.go
Outdated
| func (m *Harbor) buildBinary(ctx context.Context, platform Platform, pkg Package) *BuildMetadata { | ||
|
|
||
| os, arch, err := parsePlatform(string(platform)) | ||
| if err != nil { | ||
| log.Fatalf("Error parsing platform: %v", err) | ||
| } | ||
|
|
||
| outputPath := fmt.Sprintf("bin/%s/%s", platform, pkg) | ||
| src := fmt.Sprintf("%s/main.go", pkg) | ||
| builder := dag.Container(). | ||
| From("golang:latest"). | ||
| 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)). | ||
| WithEnvVariable("GOCACHE", "/go/build-cache"). | ||
| WithMountedDirectory("/harbor", m.Source). | ||
| WithWorkdir("/harbor/src/"). | ||
| WithEnvVariable("GOOS", os). | ||
| WithEnvVariable("GOARCH", arch). | ||
| WithEnvVariable("CGO_ENABLED", "0"). | ||
| WithExec([]string{"go", "build", "-o", outputPath, "-ldflags", "-extldflags=-static -s -w", src}) | ||
|
|
||
| return &BuildMetadata{ | ||
| Package: pkg, | ||
| BinaryPath: outputPath, | ||
| Container: builder, | ||
| Platform: platform, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set explicit Go version and cache keys in the build container
In the buildBinary function, the Go version used in the build container is set to "latest". For reproducible builds and to avoid unexpected issues due to version changes, it is better to specify an explicit Go version. Additionally, using the Go version in cache keys ensures that caches are invalidated appropriately when the Go version changes.
Apply this diff to specify the Go version and update cache keys:
builder := dag.Container().
- From("golang:latest").
+ From(fmt.Sprintf("golang:%s", GO_VERSION)).
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)).
WithEnvVariable("GOCACHE", "/go/build-cache").Ensure that GO_VERSION is properly defined with a specific version (e.g., "1.20").
📝 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.
| func (m *Harbor) buildBinary(ctx context.Context, platform Platform, pkg Package) *BuildMetadata { | |
| os, arch, err := parsePlatform(string(platform)) | |
| if err != nil { | |
| log.Fatalf("Error parsing platform: %v", err) | |
| } | |
| outputPath := fmt.Sprintf("bin/%s/%s", platform, pkg) | |
| src := fmt.Sprintf("%s/main.go", pkg) | |
| builder := dag.Container(). | |
| From("golang:latest"). | |
| 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)). | |
| WithEnvVariable("GOCACHE", "/go/build-cache"). | |
| WithMountedDirectory("/harbor", m.Source). | |
| WithWorkdir("/harbor/src/"). | |
| WithEnvVariable("GOOS", os). | |
| WithEnvVariable("GOARCH", arch). | |
| WithEnvVariable("CGO_ENABLED", "0"). | |
| WithExec([]string{"go", "build", "-o", outputPath, "-ldflags", "-extldflags=-static -s -w", src}) | |
| return &BuildMetadata{ | |
| Package: pkg, | |
| BinaryPath: outputPath, | |
| Container: builder, | |
| Platform: platform, | |
| } | |
| } | |
| func (m *Harbor) buildBinary(ctx context.Context, platform Platform, pkg Package) *BuildMetadata { | |
| os, arch, err := parsePlatform(string(platform)) | |
| if err != nil { | |
| log.Fatalf("Error parsing platform: %v", err) | |
| } | |
| outputPath := fmt.Sprintf("bin/%s/%s", platform, pkg) | |
| src := fmt.Sprintf("%s/main.go", pkg) | |
| builder := dag.Container(). | |
| From(fmt.Sprintf("golang:%s", GO_VERSION)). | |
| 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)). | |
| WithEnvVariable("GOCACHE", "/go/build-cache"). | |
| WithMountedDirectory("/harbor", m.Source). | |
| WithWorkdir("/harbor/src/"). | |
| WithEnvVariable("GOOS", os). | |
| WithEnvVariable("GOARCH", arch). | |
| WithEnvVariable("CGO_ENABLED", "0"). | |
| WithExec([]string{"go", "build", "-o", outputPath, "-ldflags", "-extldflags=-static -s -w", src}) | |
| return &BuildMetadata{ | |
| Package: pkg, | |
| BinaryPath: outputPath, | |
| Container: builder, | |
| Platform: platform, | |
| } | |
| } |
| @@ -0,0 +1,50 @@ | |||
| module dagger/harbor | |||
|
|
|||
| go 1.23.2 | |||
There was a problem hiding this comment.
💡 Codebase verification
Invalid Go version specified in go.mod
The specified Go version 1.23.2 is incorrect as:
- Go 1.23 is not yet released (currently in development)
- Go version numbers don't use three components (major.minor.patch) in go.mod files, they only use major.minor
Please update the Go version in go.mod to use a currently released and stable version (e.g., go 1.21 or go 1.22).
🔗 Analysis chain
Verify the Go version specified
The Go version is specified as go 1.23.2, which may not be released or widely available yet. Using an unavailable Go version can cause build failures.
Run the following script to check the latest stable Go version:
Ensure that the Go version specified in go.mod is correct and supported in your build environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Retrieve the latest stable Go version.
curl -s https://go.dev/dl/ | grep -oP 'go[0-9]+\.[0-9]+' | head -1
Length of output: 75
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.dagger/main.go (2)
62-64:⚠️ Potential issueFix the undefined “dag” identifier in multiple locations.
These references will cause compilation errors if the package is imported under the name “dagger” instead of “dag.” Use “dagger” consistently.Apply this diff to replace “dag” with “dagger”:
- linter := dag.Container(). + linter := dagger.Container(). - .WithMountedCache("/lint-cache", dag.CacheVolume("/lint-cache")). + .WithMountedCache("/lint-cache", dagger.CacheVolume("/lint-cache")). - cosing_ctr := dag.Container().From("cgr.dev/chainguard/cosign") + cosing_ctr := dagger.Container().From("cgr.dev/chainguard/cosign")Also applies to: 118-118
🧰 Tools
🪛 golangci-lint (1.62.2)
62-62: undefined: dag
(typecheck)
64-64: undefined: dag
(typecheck)
135-135:⚠️ Potential issueAvoid passing sensitive data as plaintext arguments.
Using the plaintext value “registryPasswordPlain” in command arguments can expose secrets. Passing sensitive information via environment variables or sealed secrets mitigates this risk.Here’s a possible fix that uses “--registry-password” from the environment variable you set:
WithSecretVariable("REGISTRY_PASSWORD", registryPassword). WithExec([]string{"cosign", "env"}). WithExec([]string{"cosign", "sign", "--yes", "--recursive", "--registry-username", registryUsername, - "--registry-password", registryPasswordPlain, + "--registry-password", "$REGISTRY_PASSWORD", imageAddr, "--timeout", "1m", })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.dagger/main.go(1 hunks)src/core/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/main.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
49-49: declared and not used: output
(typecheck)
62-62: undefined: dag
(typecheck)
64-64: undefined: dag
(typecheck)
118-118: undefined: dag
(typecheck)
.dagger/main.go
Outdated
| // LintReport Executes the Linter and writes the linting results to a file golangci-linter-report.sarif | ||
| func (m *Harbor) LintReport(ctx context.Context) (string, error) { | ||
| report := "golangci-lint-report.sarif" | ||
| output, _ := m.Source.File(report).Name(ctx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the unused variable "output".
This variable is declared but never used, leading to a warning from the linter. Consider removing it or using it if it’s actually needed.
Apply this diff to remove the unused code:
func (m *Harbor) LintReport(ctx context.Context) (string, error) {
report := "golangci-lint-report.sarif"
- output, _ := m.Source.File(report).Name(ctx)
return m.linter(ctx).WithExec([]string{"golangci-lint", "run",
"--out-format", "sarif:" + report,
"--issues-exit-code", "0"}).File(report).Export(ctx, report)
}📝 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.
| output, _ := m.Source.File(report).Name(ctx) | |
| func (m *Harbor) LintReport(ctx context.Context) (string, error) { | |
| report := "golangci-lint-report.sarif" | |
| return m.linter(ctx).WithExec([]string{"golangci-lint", "run", | |
| "--out-format", "sarif:" + report, | |
| "--issues-exit-code", "0"}).File(report).Export(ctx, report) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
49-49: declared and not used: output
(typecheck)
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.dagger/main.go (2)
66-68:⚠️ Potential issueFix the undefined
dagreferences.All references to
dag.Container(),dag.CacheVolume(), etc., will fail to compile becausedagis never defined or aliased in your import statements. You can fix this by aliasing the import or renaming references todagger.One possible fix is to alias
"dagger/harbor/internal/dagger"asdag:import ( "context" - "dagger/harbor/internal/dagger" "fmt" "log" "strings" ) +import ( + "context" + dag "dagger/harbor/internal/dagger" + "fmt" + "log" + "strings" +)Also applies to: 121-121, 171-171, 274-274, 329-329, 399-399, 418-418
🧰 Tools
🪛 golangci-lint (1.62.2)
66-66: undefined: dag
(typecheck)
68-68: undefined: dag
(typecheck)
139-139:⚠️ Potential issueStop passing the registry password in plaintext arguments.
The code at line 139 uses
registryPasswordPlaindirectly on the command line. This exposes credentials to process lists and logs, creating a security risk. Use the secret variable instead so the password isn't exposed.--registry-password, registryPasswordPlain, +--registry-password, "$REGISTRY_PASSWORD",Ensure you set the secret variable properly:
cosing_ctr = cosing_ctr.WithSecretVariable("REGISTRY_PASSWORD", registryPassword)
🧹 Nitpick comments (2)
.dagger/main.go (2)
51-51: Remove or clarify commented-out code.Line 51 contains commented-out code that either needs to be removed or explained. Leaving outdated code can cause confusion and clutter.
-// output, _ = m.Source.File(report).Name(ctx)
145-145: Address unfinished TODO.You have a TODO comment at line 145 indicating additional work is planned. If you need assistance or a follow-up PR to implement publishing logic or extra functionalities, let me know!
Would you like me to open an issue or provide a draft implementation for this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dagger/main.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
66-66: undefined: dag
(typecheck)
68-68: undefined: dag
(typecheck)
121-121: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: APITEST_LDAP
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.dagger/main.go (1)
330-330: Pin the Go version instead of relying on “latest”.Using
"golang:latest"can lead to non-deterministic builds and unexpected breakages if new versions introduce changes. Pin a specific Go version to ensure reproducible builds.-From("golang:latest"). +From(fmt.Sprintf("golang:%s", GO_VERSION)).Then define and use a stable version in your code (e.g.,
"1.20").
- cleaned up code - updated the build image
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.dagger/main.go (2)
51-51: Remove or clarify the commented-out code.This commented line appears stale and might confuse future maintainers.
-// output, _ = m.Source.File(report).Name(ctx)
145-145: Offer help with the TODO.There is a TODO “// to-do: going to work on this”. If you'd like assistance on detailing or implementing this functionality, let me know, and I can open a new issue or propose a possible approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dagger/main.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
66-66: undefined: dag
(typecheck)
68-68: undefined: dag
(typecheck)
121-121: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UI_UT
- GitHub Check: UTTEST
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB
- GitHub Check: APITEST_LDAP
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
.dagger/main.go (2)
119-120: Avoid passing sensitive data as plaintext arguments.This code uses
registryPasswordPlainfor--registry-password. Command-line arguments can be visible to other processes, risking leakage of secrets.Below is the suggested fix from a previous review (applies here, too). Pass the password via an environment variable instead:
return cosing_ctr.WithSecretVariable("REGISTRY_PASSWORD", registryPassword). WithExec([]string{"cosign", "env"}). WithExec([]string{ "cosign", "sign", "--yes", "--recursive", "--registry-username", registryUsername, - "--registry-password", registryPasswordPlain, + "--registry-password", "$REGISTRY_PASSWORD", imageAddr, "--timeout", "1m", }).Stdout(ctx)Also applies to: 139-139
13-13: Use a pinned Go version instead of "latest".
Relying on"latest"can break reproducibility and lead to unexpected build failures as the upstream image changes.Below is an example fix for one instance; apply the same pattern for all references to
"golang:latest":-const ( - GO_VERSION = "latest" - ... -) +const ( + GO_VERSION = "1.20" + ... +) builder := dag.Container(). - From("golang:latest"). + From("golang:" + GO_VERSION). ...Also applies to: 331-331, 361-361, 385-385
|
|
||
| import ( | ||
| "context" | ||
| "dagger/harbor/internal/dagger" |
There was a problem hiding this comment.
Fix the undefined dag identifier.
All references to dag.Container(), dag.Directory(), and dag.CacheVolume() will lead to compilation errors unless you alias the imported package or rename these references to match the imported identifier.
Below is a proposed diff to alias the package as dag at line 5, ensuring all existing dag.* calls compile correctly:
-import (
- "context"
- "dagger/harbor/internal/dagger"
- "fmt"
- "log"
- "strings"
-)
+import (
+ "context"
+ dag "dagger/harbor/internal/dagger"
+ "fmt"
+ "log"
+ "strings"
+)Also applies to: 66-68, 121-121, 331-331, 361-361, 384-384, 400-400, 416-416
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.dagger/main.go (2)
89-99: Consider signing all published images instead of just the first.
Currently, you sign only imageAddrs[0]. If you intend to sign each image in the returned slice, loop over them.
197-197: Rename the local variable "BuildImage" to follow Go naming conventions.
Functions, methods, and types begin with uppercase letters when exported. For local variables, a lowercase name is more idiomatic in Go.For instance:
- BuildImage := m.BuildImage(ctx, targetPlatforms[1], pkg, version) + buildImage := m.BuildImage(ctx, targetPlatforms[1], pkg, version)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dagger/main.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
66-66: undefined: dag
(typecheck)
68-68: undefined: dag
(typecheck)
121-121: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: APITEST_DB
- GitHub Check: APITEST_LDAP
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
.dagger/main.go (4)
66-66: Fix the undefined "dag" identifier.
This issue repeats at multiple lines where you call dag.Container(), dag.Directory(), or dag.CacheVolume() even though you do not alias the "dagger/harbor/internal/dagger" import as "dag." This mirrors a past review comment.Below is one possible fix, aliasing the Dagger import to "dag" at the top of the file:
package main import ( "context" - "dagger/harbor/internal/dagger" + dag "dagger/harbor/internal/dagger" "fmt" "log" "strings" ) // Then all references to dag.Container() / dag.Directory() / dag.CacheVolume() will compile.Also applies to: 68-68, 121-121, 171-171, 232-232, 279-279, 297-297, 338-338, 393-393, 408-408, 424-424
🧰 Tools
🪛 golangci-lint (1.62.2)
66-66: undefined: dag
(typecheck)
119-119: Avoid passing the password as a plaintext argument.
You pass registryPasswordPlain directly to "--registry-password", which can expose credentials in process listings. This has been flagged before in a past review comment.Proposed fix:
cosing_ctr = cosing_ctr.WithSecretVariable("REGISTRY_PASSWORD", registryPassword). - WithExec([]string{"cosign", "env"}). WithExec([]string{"cosign", "env"}). WithExec([]string{ "cosign", "sign", "--yes", "--recursive", "--registry-username", registryUsername, - "--registry-password", registryPasswordPlain, + "--registry-password", "$REGISTRY_PASSWORD", imageAddr, "--timeout", "1m", }). Stdout(ctx)Also applies to: 139-139
328-329: Verify the side effects of reassigning m.Source.
Overriding m.Source with dirWithSwagger changes the instance’s state, which may surprise other methods if they rely on the original source. Confirm this is intentional.
340-340: Pin the Go version for reproducible builds.
Currently, GO_VERSION is set to "latest", leading to non-deterministic outcomes. This was also suggested in a past review comment about specifying a fixed Go version and updating cache keys accordingly.Example fix:
const ( GOLANGCILINT_VERSION = "v1.61.0" - GO_VERSION = "latest" + GO_VERSION = "1.20" SYFT_VERSION = "v1.9.0" GORELEASER_VERSION = "v2.3.2" )Also applies to: 369-369
| imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) | ||
| _, err := m.Sign( | ||
| ctx, | ||
| githubToken, | ||
| actionsIdTokenRequestUrl, | ||
| actionsIdTokenRequestToken, | ||
| registryUsername, | ||
| registryPassword, | ||
| imageAddrs[0], | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
Check for empty image addresses to avoid out-of-range panic.
If PublishAllImages returns an empty slice, imageAddrs[0] will cause a runtime panic.
One possible fix is to verify that the slice is non-empty before indexing. For example:
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version)
-if err != nil {
- return "", fmt.Errorf("failed to sign image: %w", err)
-}
imageAddr := ""
+if len(imageAddrs) == 0 {
+ return "", fmt.Errorf("no images were published, cannot sign")
+} else {
+ imageAddr = imageAddrs[0]
+}
_, err := m.Sign(
ctx,
githubToken,
actionsIdTokenRequestUrl,
actionsIdTokenRequestToken,
registryUsername,
registryPassword,
imageAddr,
)
if err != nil {
return "", fmt.Errorf("failed to sign image: %w", err)
}Committable suggestion skipped: line range outside the PR's diff.
- Add registry binary build from distribution/distribution - Add registryctl image Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
.dagger/registry/redis.patch (4)
9-11: Remove duplicate wording in the doc comment.Currently, there's a minor typo in the doc comment: “…specifies the the redis sentinel master set name.” Consider removing the duplicate “the” for clarity.
- // SentinelMasterSet specifies the the redis sentinel master set name. + // SentinelMasterSet specifies the redis sentinel master set name.
39-76: Consider adding logging or retry logic for Sentinel connections.When establishing a sentinel-based Redis connection, an error in “DialTimeout” or the inability to find a valid master can silently fail beyond the returned error. Adding optional logs or minimal retry logic could help diagnose connectivity issues faster in production environments.
71-71: Address the TODO comment.The comment “TODO(stevvooe): We can probably do something more interesting here with the health package.” indicates future work. If relevant, consider integrating a health-check approach that more thoroughly validates Redis readiness, especially in production.
117-145: Improve test coverage by verifying Redis health checks.These tests confirm that the application doesn’t panic with a “sane” configuration. For additional reliability, consider verifying whether a ping or sentinel role check actually succeeds. This ensures the test catches unexpected connection or sentinel misconfiguration errors.
.dagger/main.go (2)
125-125: Rename "cosing_ctr" to "cosign_ctr" for clarity and consistency."cosing_ctr" appears to be a misspelling.
-cosing_ctr := dag.Container().From("cgr.dev/chainguard/cosign") +cosign_ctr := dag.Container().From("cgr.dev/chainguard/cosign")🧰 Tools
🪛 golangci-lint (1.62.2)
125-125: undefined: dag
(typecheck)
55-55: Clarify or remove the commented-out reference to “output”.The variable "output" appears commented out; either restore its usage if needed or remove it for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.dagger/main.go(1 hunks).dagger/registry/redis.patch(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
70-70: undefined: dag
(typecheck)
72-72: undefined: dag
(typecheck)
125-125: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: APITEST_LDAP
- GitHub Check: APITEST_DB
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
.dagger/registry/redis.patch (6)
78-98: Verify potential concurrency overhead.Creating a new function “getRedisAddr” and calling it for each Redis connection dial is powerful for re-checking Sentinel status. However, repeated lookups may introduce overhead. If the system handles many connections, caching or reusing resolved addresses might reduce repeated sentinel lookups. Confirm that this overhead won’t negatively impact high-volume usage scenarios.
99-114: Good approach factoring out TestOnBorrow logic.Switching to a shared “testOnBorrow” function eliminates code duplication and centralizes the health-check logic for sentinel vs. non-sentinel connections. This is an excellent step toward maintainable code.
161-185: Ensure environment readiness for Sentinel testing.“TestNewAppWithRedisSentinelCluster” is helpful for ensuring sentinel logic works, but remember that it requires a running set of mock or real sentinel nodes at “192.168.0.11:26379,192.168.0.12:26379.” If these tests run in CI/CD, confirm that the environment or mocking strategy is set up to avoid failures.
199-199: Pin the vendor version carefully.You've added “github.com/FZambia/sentinel” with a specific commit. It's usually wise to confirm no known security vulnerabilities or major bugfixes exist in subsequent commits or versions. Consider verifying the pinned commit’s stability before finalizing.
200-452: License inclusion is appropriate.Including the Apache 2.0 license is correct as part of vendor code. Ensure that your project’s overall licensing strategy allows for bundling this third-party license without conflict.
453-884: External sentinel library vendor review.The newly added sentinel code from “github.com/FZambia/sentinel” appears well-structured and follows typical patterns for Redigo-based sentinel handling. Concurrency is managed via a read-write mutex, and the logic for discovering and rotating sentinel addresses is aligned with standard redis-sentinel best practices. Overall, the library seems stable for high availability setups.
.dagger/main.go (4)
93-101: Check for empty image addresses to avoid runtime panic.If PublishAllImages returns an empty slice, indexing "imageAddrs[0]" will cause an out-of-range panic.
As a safeguard, confirm that the slice is non-empty before accessing the first element:
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) +if len(imageAddrs) == 0 { + return "", fmt.Errorf("no images published, cannot sign") +} _, err := m.Sign( ctx, githubToken, actionsIdTokenRequestUrl, actionsIdTokenRequestToken, registryUsername, registryPassword, imageAddrs[0], )
5-5:⚠️ Potential issueAlias the "dagger/harbor/internal/dagger" import or rename references to avoid undefined "dag" errors.
The code references "dag.Container()" and others, but the import is not aliased. This will cause compilation errors since "dag" is not defined.
Apply this diff to alias the import as "dag" and fix references:
-import ( - "context" - "dagger/harbor/internal/dagger" - "fmt" - "log" - "strings" -) +import ( + "context" + dag "dagger/harbor/internal/dagger" + "fmt" + "log" + "strings" +)Likely invalid or redundant comment.
13-13: 🛠️ Refactor suggestionPin a specific Go version for reproducible builds.
Using "latest" can lead to unexpected changes in dependencies.
-GO_VERSION = "latest" +GO_VERSION = "1.20"Likely invalid or redundant comment.
379-379: 🛠️ Refactor suggestionUse a stable Go container tag instead of "golang:latest".
Similar to GO_VERSION, referencing "latest" can introduce build unpredictability.
- .From("golang:latest"). + .From("golang:1.20").Likely invalid or redundant comment.
- update dagger from v0.15.2 to v0.15.3 Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
.dagger/main.go (2)
122-122: Offer to handle TODO.A “to-do” comment at line 122 indicates more features or logic may be added to PublishAllImages. Let me know if you’d like help formalizing these requirements or creating a new issue to track them.
288-301: Consider reducing code duplication between “buildImage” and “buildBinary.”The logic for building images reuses much of the same steps as building binaries. You could factor out common steps (e.g., environment setup, caching, go build execution) into a shared helper function to align with DRY principles and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.dagger/main.go(1 hunks).dagger/tests.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
98-98: undefined: dag
(typecheck)
148-148: undefined: dag
(typecheck)
209-209: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: APITEST_LDAP
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: CodeQL-Build
- GitHub Check: APITEST_DB
🔇 Additional comments (2)
.dagger/tests.go (1)
34-34: Address the commented cache clean command.The commented-out cache clean command suggests there might be cache-related issues. Either remove the comment if it's no longer needed or document why it's kept for future reference.
Please clarify:
- Are there known cache issues that need to be addressed?
- Should the cache clean command be enabled in certain scenarios?
.dagger/main.go (1)
74-74:⚠️ Potential issueCheck for out-of-range slice access.
At line 74, “imageAddrs[0]” will panic if no images are returned by PublishAllImages. Consider validating that the slice is non-empty before accessing element zero.
Example fix:
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) -if len(imageAddrs) == 0 { - // ... -} +if len(imageAddrs) == 0 { + return "", fmt.Errorf("no images were published; cannot sign") +} _, err := m.Sign( ctx, ...Likely invalid or redundant comment.
| func (m *Harbor) goVulnCheck(ctx context.Context) *dagger.Container { | ||
| m.Source = m.genAPIs(ctx) | ||
| return dag.Container(). | ||
| From("golang:alpine"). | ||
| WithMountedDirectory("/harbor", m.Source). | ||
| WithWorkdir("/harbor/src"). | ||
| WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@latest"}). | ||
| WithEntrypoint([]string{"/go/bin/govulncheck"}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the govulncheck version for reproducible builds.
Using @latest for govulncheck could lead to inconsistent results between CI runs. Consider pinning to a specific version.
Apply this diff to pin the version:
-WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@latest"}).
+WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@v1.0.1"}). // Replace with desired version📝 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.
| func (m *Harbor) goVulnCheck(ctx context.Context) *dagger.Container { | |
| m.Source = m.genAPIs(ctx) | |
| return dag.Container(). | |
| From("golang:alpine"). | |
| WithMountedDirectory("/harbor", m.Source). | |
| WithWorkdir("/harbor/src"). | |
| WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@latest"}). | |
| WithEntrypoint([]string{"/go/bin/govulncheck"}) | |
| } | |
| func (m *Harbor) goVulnCheck(ctx context.Context) *dagger.Container { | |
| m.Source = m.genAPIs(ctx) | |
| return dag.Container(). | |
| From("golang:alpine"). | |
| WithMountedDirectory("/harbor", m.Source). | |
| WithWorkdir("/harbor/src"). | |
| WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@v1.0.1"}). // Replace with desired version | |
| WithEntrypoint([]string{"/go/bin/govulncheck"}) | |
| } |
| // Executes Linter and writes results to a file golangci-lint.report for github-actions | ||
| func (m *Harbor) LintReport(ctx context.Context) *dagger.File { | ||
| report := "golangci-lint.report" | ||
| return m.lint(ctx).WithExec([]string{ | ||
| "golangci-lint", "-v", "run", "--timeout=10m", | ||
| "--out-format", "github-actions:" + report, | ||
| "--issues-exit-code", "0", | ||
| }).File(report) | ||
| } |
There was a problem hiding this comment.
Remove --issues-exit-code to ensure CI fails on linting issues.
Setting --issues-exit-code to 0 means the CI pipeline won't fail even when linting issues are found. This could lead to code quality issues being merged into the codebase.
Apply this diff to ensure CI fails on linting issues:
return m.lint(ctx).WithExec([]string{
"golangci-lint", "-v", "run", "--timeout=10m",
"--out-format", "github-actions:" + report,
- "--issues-exit-code", "0",
}).File(report)📝 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.
| // Executes Linter and writes results to a file golangci-lint.report for github-actions | |
| func (m *Harbor) LintReport(ctx context.Context) *dagger.File { | |
| report := "golangci-lint.report" | |
| return m.lint(ctx).WithExec([]string{ | |
| "golangci-lint", "-v", "run", "--timeout=10m", | |
| "--out-format", "github-actions:" + report, | |
| "--issues-exit-code", "0", | |
| }).File(report) | |
| } | |
| // Executes Linter and writes results to a file golangci-lint.report for github-actions | |
| func (m *Harbor) LintReport(ctx context.Context) *dagger.File { | |
| report := "golangci-lint.report" | |
| return m.lint(ctx).WithExec([]string{ | |
| "golangci-lint", "-v", "run", "--timeout=10m", | |
| "--out-format", "github-actions:" + report, | |
| }).File(report) | |
| } |
| // Generate Vulnerability Report in sarif format for github-actions | ||
| func (m *Harbor) GoVulnCheckReport(ctx context.Context) (string, error) { | ||
| fmt.Println("👀 Generating Vulnerability Report") | ||
| return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-format", "sarif", "./..."}).Stdout(ctx) | ||
| } |
There was a problem hiding this comment.
Fix the govulncheck command arguments.
Similar to the previous method, the command includes both govulncheck and ./... in the args, but the binary name is already set as the entrypoint.
Apply this diff to fix the command:
-return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-format", "sarif", "./..."}).Stdout(ctx)
+return m.goVulnCheck(ctx).WithExec([]string{"-format", "sarif", "./..."}).Stdout(ctx)📝 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.
| // Generate Vulnerability Report in sarif format for github-actions | |
| func (m *Harbor) GoVulnCheckReport(ctx context.Context) (string, error) { | |
| fmt.Println("👀 Generating Vulnerability Report") | |
| return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-format", "sarif", "./..."}).Stdout(ctx) | |
| } | |
| // Generate Vulnerability Report in sarif format for github-actions | |
| func (m *Harbor) GoVulnCheckReport(ctx context.Context) (string, error) { | |
| fmt.Println("👀 Generating Vulnerability Report") | |
| return m.goVulnCheck(ctx).WithExec([]string{"-format", "sarif", "./..."}).Stdout(ctx) | |
| } |
| // Lint Run the linter golangci-lint | ||
| func (m *Harbor) Lint(ctx context.Context) (string, error) { | ||
| return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stderr(ctx) | ||
| } |
There was a problem hiding this comment.
Return stdout instead of stderr for linting output.
The method currently returns stderr, which means we'll only see errors from the linter itself, not the actual linting output. The linting results are written to stdout.
Apply this diff to capture the linting output:
-return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stderr(ctx)
+return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stdout(ctx)📝 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.
| // Lint Run the linter golangci-lint | |
| func (m *Harbor) Lint(ctx context.Context) (string, error) { | |
| return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stderr(ctx) | |
| } | |
| // Lint Run the linter golangci-lint | |
| func (m *Harbor) Lint(ctx context.Context) (string, error) { | |
| return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stdout(ctx) | |
| } |
| // Check vulnerabilities in go-code | ||
| func (m *Harbor) GoVulnCheck(ctx context.Context) (string, error) { | ||
| fmt.Println("👀 Running Go vulnerabilities check") | ||
| return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-show", "verbose", "./..."}).Stdout(ctx) | ||
| } |
There was a problem hiding this comment.
Fix the govulncheck command arguments.
The command includes both govulncheck and ./... in the args, but the binary name is already set as the entrypoint. This will cause the command to fail.
Apply this diff to fix the command:
-return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-show", "verbose", "./..."}).Stdout(ctx)
+return m.goVulnCheck(ctx).WithExec([]string{"-show", "verbose", "./..."}).Stdout(ctx)| linter := dag.Container(). | ||
| From("golangci/golangci-lint:"+GOLANGCILINT_VERSION+"-alpine"). |
There was a problem hiding this comment.
Fix the missing import for dag variable.
The dag variable is used but not imported. This will cause compilation errors.
Add the missing import:
import (
"context"
"dagger/harbor/internal/dagger"
"fmt"
+ "dagger.io/dagger"
)
+var dag = dagger.Connect()Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.dagger/tests.go (4)
21-21: Capture lint output from stdout.Lint warnings and errors typically print to stdout. Capturing stderr only might miss important details.
-return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stderr(ctx) +return m.lint(ctx).WithExec([]string{"golangci-lint", "-v", "run", "--timeout=10m"}).Stdout(ctx)
43-43: Pin the Alpine version for reproducibility.“golang:alpine” does not specify a version, which can cause differences in builds if the base image updates.
- .From("golang:alpine"). + .From("golang:1.20-alpine").
53-53: Remove redundant “govulncheck” argument when the entrypoint is already set.You are calling “govulncheck” as an argument even though the container’s entrypoint is “/go/bin/govulncheck,” causing duplication.
-return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-show", "verbose", "./..."}).Stdout(ctx) +return m.goVulnCheck(ctx).WithExec([]string{"-show", "verbose", "./..."}).Stdout(ctx)
59-59: Remove redundant “govulncheck” argument for SARIF generation.Same reason as above—already set as the entrypoint, so no need to specify it again.
-return m.goVulnCheck(ctx).WithExec([]string{"govulncheck", "-format", "sarif", "./..."}).Stdout(ctx) +return m.goVulnCheck(ctx).WithExec([]string{"-format", "sarif", "./..."}).Stdout(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.dagger/main.go(1 hunks).dagger/tests.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
.dagger/main.go
98-98: undefined: dag
(typecheck)
148-148: undefined: dag
(typecheck)
209-209: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: APITEST_LDAP
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: OFFLINE
- GitHub Check: UI_UT
- GitHub Check: UTTEST
- GitHub Check: APITEST_DB
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
.dagger/main.go (8)
98-98: Undefined “dag” identifier.You must alias "dagger" at import or replace “dag.” calls with “dagger.” to avoid a compilation error.
-import ( - "dagger/harbor/internal/dagger" - ... -) +import ( + dag "dagger/harbor/internal/dagger" + ... +) ... -cosing_ctr := dag.Container().From("cgr.dev/chainguard/cosign") +cosing_ctr := dag.Container().From("cgr.dev/chainguard/cosign")🧰 Tools
🪛 golangci-lint (1.62.2)
98-98: undefined: dag
(typecheck)
148-148: Undefined “dag” identifier.Same issue as line 98—ensure the package is imported or aliased as “dag.”
-container := dag.Container().WithRegistryAuth(registry, registryUsername, registryPassword) +container := dag.Container().WithRegistryAuth(registry, registryUsername, registryPassword)🧰 Tools
🪛 golangci-lint (1.62.2)
148-148: undefined: dag
(typecheck)
209-209: Undefined “dag” identifier.Same issue—import or alias the dagger package as “dag.”
-artifacts := dag.Directory() +artifacts := dag.Directory()🧰 Tools
🪛 golangci-lint (1.62.2)
209-209: undefined: dag
(typecheck)
352-352: 🛠️ Refactor suggestionPin the Go version for reproducible builds.
Using "latest" may introduce unpredictable outcomes if a new Go version has breaking changes.
- .From("golang:latest"). + .From(fmt.Sprintf("golang:%s", GO_VERSION)).Likely invalid or redundant comment.
13-13: 🛠️ Refactor suggestionPin the Go version for reproducible builds.
Using "latest" can lead to inconsistent or unexpected outcomes if future Go versions introduce breaking changes. Consider pinning a specific Go version (for example, "1.20") to ensure consistent and stable builds.
- GO_VERSION = "latest" + GO_VERSION = "1.20"Likely invalid or redundant comment.
66-74:⚠️ Potential issueCheck for empty slices before indexing imageAddrs.
Accessing imageAddrs[0] might cause a runtime panic if PublishAllImages returns an empty slice. Add a simple length check to handle this scenario gracefully.
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) +if len(imageAddrs) == 0 { + return "", fmt.Errorf("no images were published, cannot sign") +} _, err := m.Sign(Likely invalid or redundant comment.
382-382: 🛠️ Refactor suggestionPin the Go version for reproducible builds.
Similarly, replace "golang:latest" with a specific version or reference GO_VERSION.
- .From("golang:latest"). + .From(fmt.Sprintf("golang:%s", GO_VERSION)).Likely invalid or redundant comment.
406-406: 🛠️ Refactor suggestionPin the Go version for reproducible builds.
Same rationale: using “latest” can cause inconsistent results if a future release breaks compatibility.
- temp := dag.Container(). - From("golang:latest"). + temp := dag.Container(). + From(fmt.Sprintf("golang:%s", GO_VERSION)).Likely invalid or redundant comment.
.dagger/tests.go (2)
13-15: 🛠️ Refactor suggestionRemove “--issues-exit-code=0” to block merges if lint issues exist.
Setting --issues-exit-code=0 causes the pipeline to succeed even when lint issues are detected, potentially lowering code quality.
"golangci-lint", "-v", "run", "--timeout=10m", "--out-format", "github-actions:" + report, - "--issues-exit-code", "0",Likely invalid or redundant comment.
46-46: 🛠️ Refactor suggestionPin the govulncheck version for reproducible results.
Using “@latest” can yield inconsistent vulnerability scanning results across runs.
- .WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@latest"}). + .WithExec([]string{"go", "install", "golang.org/x/vuln/cmd/govulncheck@v1.0.1"}). // exampleLikely invalid or redundant comment.
| WithExec([]string{ | ||
| "cosign", "sign", "--yes", "--recursive", | ||
| "--registry-username", registryUsername, | ||
| "--registry-password", registryPasswordPlain, |
There was a problem hiding this comment.
Avoid passing sensitive credentials on the command line.
Shell command arguments can be observed by other processes. Recommend using an environment variable or a secret.
"--registry-password", registryPasswordPlain,
+ "--registry-password", "$REGISTRY_PASSWORD",Then ensure that REGISTRY_PASSWORD is properly set as a secret variable:
cosing_ctr = cosing_ctr.WithSecretVariable("REGISTRY_PASSWORD", registryPassword)- The main.go dagger file is getting bigger and bigger - Its time we breakdown these components into different files - This way it can be better to navigate and fix issues. Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.dagger/main.go (3)
13-13: Pin the Go version instead of using 'latest'.
Relying on "latest" for both GO_VERSION and the Go image can lead to unpredictable builds if future Go releases introduce breaking changes. Pinning a stable version (e.g., "1.20") helps ensure reproducible builds.Example fix for line 13 and line 357:
-const ( - GO_VERSION = "latest" - ... -) +const ( + GO_VERSION = "1.20" + ... +) ... - .From("golang:latest"). + .From(fmt.Sprintf("golang:%s", GO_VERSION)).Also applies to: 357-357
382-382: Pin the Alpine version for reproducible builds.
Using alpine:latest can introduce unexpected changes when new versions of Alpine are released. Pinning a specific version (e.g., “3.18”) ensures consistency and avoids future breakage.Example fix:
- .From("alpine:latest"). + .From("alpine:3.18").Also applies to: 387-387
399-399: Assess the necessity of “--unsafe-perm” when installing npm packages.
Using “--unsafe-perm” can be a security risk, as it allows scripts to run with root privileges. Confirm if it’s strictly needed to avoid potential vulnerabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.dagger/main.go(1 hunks).dagger/tests.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .dagger/tests.go
🧰 Additional context used
📓 Learnings (1)
.dagger/main.go (1)
Learnt from: bupd
PR: container-registry/harbor-next#55
File: .dagger/main.go:98-98
Timestamp: 2025-02-10T02:21:53.194Z
Learning: In Dagger development environment, the "dag" identifier is automatically defined and available for use in .dagger/*.go files, even if it appears undefined to static analysis tools. These files are gitignored and the undefined identifier warning can be safely ignored.
🪛 golangci-lint (1.62.2)
.dagger/main.go
99-99: undefined: dag
(typecheck)
149-149: undefined: dag
(typecheck)
210-210: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_LDAP
- GitHub Check: APITEST_DB
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
.dagger/main.go (3)
99-99: Ignore “undefined: dag” warnings.
Static analysis flags “dag” as undefined, but per the retrieved learnings, “dag” is automatically defined in Dagger’s development environment for .dagger/*.go files. You can safely disregard these warnings.Also applies to: 149-149, 210-210
🧰 Tools
🪛 golangci-lint (1.62.2)
99-99: undefined: dag
(typecheck)
117-117: Avoid passing passwords as plaintext arguments.
This is a repeated issue from past reviews. Exposing the password in --registry-password poses a security risk. Instead, pass "$REGISTRY_PASSWORD" and rely on the secret variable already set.Suggested change:
- "--registry-password", registryPasswordPlain, + "--registry-password", "$REGISTRY_PASSWORD",
53-75:⚠️ Potential issueCheck for an empty slice before referencing imageAddrs[0].
If PublishAllImages returns an empty slice, calling imageAddrs[0] will cause a runtime panic.Example fix:
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) -if len(imageAddrs) == 0 { - _, err := m.Sign(... - ... - imageAddrs[0], -) +if len(imageAddrs) == 0 { + return "", fmt.Errorf("no images were published, cannot sign") +} _, err := m.Sign( ctx, githubToken, actionsIdTokenRequestUrl, actionsIdTokenRequestToken, registryUsername, registryPassword, imageAddrs[0], )Likely invalid or redundant comment.
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.dagger/main.go (4)
67-76:⚠️ Potential issueAdd bounds check before accessing imageAddrs[0].
If PublishAllImages returns an empty slice, accessing imageAddrs[0] will cause a runtime panic.
Apply this diff to add bounds check:
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) +if len(imageAddrs) == 0 { + return "", fmt.Errorf("no images were published, cannot sign") +} _, err := m.Sign( ctx, githubToken, actionsIdTokenRequestUrl, actionsIdTokenRequestToken, registryUsername, registryPassword, imageAddrs[0], )
114-120:⚠️ Potential issueUse environment variable for registry password.
Passing the registry password as a command-line argument exposes sensitive information to other processes.
Apply this diff to use environment variable:
WithExec([]string{ "cosign", "sign", "--yes", "--recursive", "--registry-username", registryUsername, - "--registry-password", registryPasswordPlain, + "--registry-password", "$REGISTRY_PASSWORD", imageAddr, "--timeout", "1m", })
357-357: 🛠️ Refactor suggestionPin the builder image version for reproducible builds.
Using "golang:latest" can lead to unpredictable builds if new Go versions introduce breaking changes.
Apply this diff to use a specific version:
- From("golang:latest"). + From(fmt.Sprintf("golang:%s", GO_VERSION)).
435-435: 🛠️ Refactor suggestionPin the Git container version for reproducible builds.
Using "golang:latest" can lead to unpredictable behavior if new versions introduce changes.
Apply this diff to use a specific version:
- From("golang:latest"). + From(fmt.Sprintf("golang:%s", GO_VERSION)).
🧹 Nitpick comments (2)
.dagger/main.go (2)
133-138: Remove or document commented code block.The commented code block appears to be related to image tag processing. If this functionality is needed in the future, add a TODO comment explaining why it's commented out and when it should be uncommented.
Apply this diff to add explanation:
- // for i, tag := range imageTags { - // imageTags[i] = strings.TrimSpace(tag) - // if strings.HasPrefix(imageTags[i], "v") { - // imageTags[i] = strings.TrimPrefix(imageTags[i], "v") - // } - // } + // TODO: Re-enable tag processing when version prefix handling is finalized + // Currently disabled to maintain compatibility with existing tag formats + // for i, tag := range imageTags { + // imageTags[i] = strings.TrimSpace(tag) + // if strings.HasPrefix(imageTags[i], "v") { + // imageTags[i] = strings.TrimPrefix(imageTags[i], "v") + // } + // }
399-407: Consolidate npm install commands.Multiple npm install commands can be consolidated to reduce the number of layers in the container image.
Apply this diff to consolidate npm commands:
- WithExec([]string{"npm", "install", "--unsafe-perm"}). - WithExec([]string{"npm", "run", "generate-build-timestamp"}). - WithExec([]string{"node", "--max_old_space_size=2048", "node_modules/@angular/cli/bin/ng", "build", "--configuration", "production"}). - WithExec([]string{"npm", "install", "js-yaml@4.1.0"}). + WithExec([]string{"npm", "install", "--unsafe-perm", "js-yaml@4.1.0"}). + WithExec([]string{"npm", "run", "generate-build-timestamp"}). + WithExec([]string{"node", "--max_old_space_size=2048", "node_modules/@angular/cli/bin/ng", "build", "--configuration", "production"}).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dagger/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.dagger/main.go (1)
Learnt from: bupd
PR: container-registry/harbor-next#55
File: .dagger/main.go:98-98
Timestamp: 2025-02-10T02:21:53.194Z
Learning: In Dagger development environment, the "dag" identifier is automatically defined and available for use in .dagger/*.go files, even if it appears undefined to static analysis tools. These files are gitignored and the undefined identifier warning can be safely ignored.
🪛 golangci-lint (1.62.2)
.dagger/main.go
99-99: undefined: dag
(typecheck)
149-149: undefined: dag
(typecheck)
210-210: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: APITEST_DB
- GitHub Check: CodeQL-Build
- GitHub Check: APITEST_LDAP
🔇 Additional comments (1)
.dagger/main.go (1)
13-13: Pin the Go version for reproducible builds.Using "latest" for GO_VERSION can lead to unpredictable builds if new Go versions introduce breaking changes.
What is the latest stable version of Go?
| WithMountedDirectory("/src", m.Source). | ||
| WithWorkdir("/src") | ||
|
|
||
| gitCommit, _ := temp.WithExec([]string{"git", "rev-parse", "--short=8", "HEAD", "--always"}).Stdout(ctx) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle error from git command.
The error from git command is ignored, which could hide issues with the repository state.
Apply this diff to handle the error:
- gitCommit, _ := temp.WithExec([]string{"git", "rev-parse", "--short=8", "HEAD", "--always"}).Stdout(ctx)
+ gitCommit, err := temp.WithExec([]string{"git", "rev-parse", "--short=8", "HEAD", "--always"}).Stdout(ctx)
+ if err != nil {
+ return fmt.Sprintf("unknown-%s", time.Now().Format("20060102"))
+ }📝 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.
| gitCommit, _ := temp.WithExec([]string{"git", "rev-parse", "--short=8", "HEAD", "--always"}).Stdout(ctx) | |
| gitCommit, err := temp.WithExec([]string{"git", "rev-parse", "--short=8", "HEAD", "--always"}).Stdout(ctx) | |
| if err != nil { | |
| return fmt.Sprintf("unknown-%s", time.Now().Format("20060102")) | |
| } |
| File("LICENSE") | ||
|
|
||
| builder := dag.Container(). | ||
| From("node:16.18.0"). |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update Node.js version to a supported release.
Node.js 16.x reached end-of-life and should be updated to a supported version.
🌐 Web query:
What is the latest LTS version of Node.js?
💡 Result:
The latest Long Term Support (LTS) version of Node.js as of February 2025 is Node.js v22 (codename 'Jod'), which entered Active LTS status on October 29, 2024[1][5]. This version will receive critical updates and security patches through its lifecycle:
- Active LTS Support: Until October 2025
- Maintenance LTS Support: From October 2025 to April 2027[3][5]
Key features introduced in this LTS version include:
- Built-in WebSocket client enabled by default[1]
- OpenSSL 3.x integration with support until September 2026[5]
- Enhanced stability and production readiness[1][7]
Previous LTS versions still under support (as of February 2025):
- Node.js v20 (Iron): In Maintenance LTS until April 2026[3][7]
- Node.js v18 (Hydrogen): Already reached End-of-Life in April 2024[6]
For production environments requiring long-term stability, Node.js v22.x is recommended over odd-numbered releases like v21 or v23, which follow a shorter support cycle[3][9]. The official Node.js website currently promotes v22.14.0 as the latest LTS download[8].
Citations:
- 1: http://nodesource.com/blog/Node.js-v22-Long-Term-Support-LTS/
- 2: https://daily.dev/blog/nodejs-v20-lts-is-now-generally-available-and-its-awesome
- 3: https://www.herodevs.com/blog-posts/node-js-end-of-life-dates-you-should-be-aware-of
- 4: https://www.npmjs.com/package/node-lts-versions
- 5: https://nodejs.org/en/blog/release/v22.11.0
- 6: https://endoflife.date/nodejs
- 7: https://nodejs.org/en/about/previous-releases
- 8: https://nodejs.org/en
- 9: https://github.com/nodejs/Release
Update Node.js version to LTS v22 & Consolidate npm install Commands
-
The file
.dagger/main.go(line 394) currently specifies:From("node:16.18.0").
Since Node.js 16.x has reached end-of-life, update this reference to the supported LTS release (Node.js v22).
-
Additionally, the
buildPortalfunction contains multiplenpm installcommands. Please consolidate these commands to simplify the installation process and improve build performance.
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.dagger/main.go (1)
53-83:⚠️ Potential issueCheck for empty slice when signing the first image.
At line 75, the code calls
imageAddrs[0]without verifying thatimageAddrsis non-empty. IfPublishAllImagesreturns an empty slice, this will cause an out-of-range error. Promptly check the length before indexing.Example fix:
imageAddrs := m.PublishAllImages(ctx, registry, registryUsername, imageTags, registryPassword, version) -if err != nil { - ... -} +if len(imageAddrs) == 0 { + return "", fmt.Errorf("no images published, cannot sign") +} addrToSign := imageAddrs[0] _, err := m.Sign( ctx,
🧹 Nitpick comments (2)
.dagger/main.go (2)
414-414: Update the Node.js base image to a supported LTS version.Node.js 16.x reached end-of-life, which may expose your pipeline to unpatched vulnerabilities. Consider upgrading to Node.js 22 (current LTS) or another actively supported release.
Example fix:
- From("node:16.18.0"). + From("node:22.14.0").
420-429: Consolidate consecutive npm install steps.Multiple
npm installcalls increase build time and can cause potential version conflicts. Combine them into as few steps as possible for better performance and maintainability.Example fix:
- .WithExec([]string{"npm", "install", "--unsafe-perm"}). - .WithExec([]string{"npm", "run", "generate-build-timestamp"}). - .WithExec([]string{"node", "--max_old_space_size=2048", "node_modules/@angular/cli/bin/ng", "build", "--configuration", "production"}). - .WithExec([]string{"npm", "install", "js-yaml@4.1.0"}). - .WithExec([]string{"sh", "-c", fmt.Sprintf("node -e \"const yaml = require('js-yaml'); ... \"" )}). - .WithExec([]string{"npm", "install", "--unsafe-perm"}). - .WithExec([]string{"npm", "run", "build"}). + .WithExec([]string{ + "npm", "install", "--unsafe-perm", "js-yaml@4.1.0", + // Include any other dependencies, all in one install command + }). + .WithExec([]string{"npm", "run", "generate-build-timestamp"}). + .WithExec([]string{ + "node", "--max_old_space_size=2048", + "node_modules/@angular/cli/bin/ng", "build", + "--configuration", "production", + }). + // Next steps remain the same + .WithExec([]string{"sh", "-c", fmt.Sprintf("node -e \"const yaml = require('js-yaml'); ... \"" )}). + .WithExec([]string{"npm", "run", "build"}).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dagger/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.dagger/main.go (1)
Learnt from: bupd
PR: container-registry/harbor-next#55
File: .dagger/main.go:98-98
Timestamp: 2025-02-10T02:21:53.194Z
Learning: In Dagger development environment, the "dag" identifier is automatically defined and available for use in .dagger/*.go files, even if it appears undefined to static analysis tools. These files are gitignored and the undefined identifier warning can be safely ignored.
🪛 golangci-lint (1.62.2)
.dagger/main.go
99-99: undefined: dag
(typecheck)
149-149: undefined: dag
(typecheck)
210-210: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: APITEST_LDAP
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: APITEST_DB
- GitHub Check: UTTEST
- GitHub Check: CodeQL-Build
| return cosing_ctr.WithSecretVariable("REGISTRY_PASSWORD", registryPassword). | ||
| 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 passing plaintext credentials via CLI arguments.
At line 117, registryPasswordPlain is passed directly with the --registry-password flag, making it potentially visible in process lists. Use environment variables or Dagger secrets instead.
Example fix:
- "--registry-password", registryPasswordPlain,
+ "--registry-password", "$REGISTRY_PASSWORD",Ensure that REGISTRY_PASSWORD is properly set as a secret variable and accessible to the cosign process:
cosing_ctr = cosing_ctr.WithSecretVariable("REGISTRY_PASSWORD", registryPassword)Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/publish_prs.yml (1)
43-49: Commented-Out Reviewdog Steps
There are commented-out blocks for reviewdog integration. If these steps are not planned for immediate use, consider removing them to keep the workflow file lean; otherwise, add a clarifying comment about when they might be enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/publish_prs.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/publish_prs.yml
28-28: property "dagger_version" is not defined in object type {}
(expression)
105-105: property "dagger_version" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_DB
- GitHub Check: UTTEST
- GitHub Check: APITEST_LDAP
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: lint
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
.github/workflows/publish_prs.yml (7)
1-12: Overall Workflow Triggers and Metadata Look Good
The workflow name, branch triggers (including the dedicated "build/daggerize" branch), tag patterns, and pull request path filters are defined clearly.
13-21: Lint Job Setup is Well-Structured
The lint job’s configuration—including the checkout step and runner specification—is clear and follows best practices.
25-31: Ensure Output Reference Consistency in Linting Step
The "Run Dagger golangci-lint" step references${{ steps.dagger_version.outputs.version }}(line 28). This output will only be available after the Dagger Version step is assigned anidas suggested above. Once that is fixed, this step should work as intended.🧰 Tools
🪛 actionlint (1.7.4)
28-28: property "dagger_version" is not defined in object type {}
(expression)
32-41: Effective Lint Summary Generation
The step generating the lint summary appends results to$GITHUB_STEP_SUMMARYand appropriately exits with an error if lint issues are detected. This is an effective pattern for CI feedback.
77-95: Push-Latest-Images Job: Initial Steps are Solid
The debugging and repository checkout steps in the "push-latest-images" job are clear and follow good practices.
96-116: Reference to Dagger Version Output in Image Publishing
Within the "Publish All Images" step, the job again references${{ steps.dagger_version.outputs.version }}(line 105). This will only work correctly after the Dagger Version step is assigned anid. Ensure that the earlier change is applied so that this reference becomes valid.🧰 Tools
🪛 actionlint (1.7.4)
105-105: property "dagger_version" is not defined in object type {}
(expression)
128-178: Publish-Release Job is Configured Appropriately
The "publish-release" job correctly handles conditions for tag pushes, release creation, image publishing, and includes debugging steps. The configuration appears robust and aligned with the pipeline’s objectives.
| - name: Dagger Version | ||
| uses: sagikazarmark/dagger-version-action@v0.0.1 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing id in the Dagger Version Step
The "Dagger Version" step does not define an id. Since subsequent steps reference its outputs via ${{ steps.dagger_version.outputs.version }}, this omission will cause runtime errors. Consider adding an id (e.g., id: dagger_version) to this step.
Suggested diff:
- - name: Dagger Version
- uses: sagikazarmark/dagger-version-action@v0.0.1
+ - name: Dagger Version
+ id: dagger_version
+ uses: sagikazarmark/dagger-version-action@v0.0.1📝 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: Dagger Version | |
| uses: sagikazarmark/dagger-version-action@v0.0.1 | |
| - name: Dagger Version | |
| id: dagger_version | |
| uses: sagikazarmark/dagger-version-action@v0.0.1 |
Signed-off-by: bupd <bupdprasanth@gmail.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.dagger/main.go (2)
439-447: Reduce multiplenpm installcalls to improve efficiencyInstalling packages multiple times wastes resources. Combine the dependencies installation into one step—this can shorten build times:
.WithExec([]string{"npm", "install", "--unsafe-perm"}). .WithExec([]string{"npm", "run", "generate-build-timestamp"}). .WithExec([]string{"node", "--max_old_space_size=2048", "node_modules/@angular/cli/bin/ng", "build", "--configuration", "production"}). -.WithExec([]string{"npm", "install", "js-yaml@4.1.0"}). -.WithExec([]string{"sh", "-c", "..."}). +.WithExec([]string{ + "npm", + "install", + "js-yaml@4.1.0", + "--unsafe-perm", + "&&", + "node", + "--max_old_space_size=2048", + "node_modules/@angular/cli/bin/ng", + "build", + "--configuration", + "production", + "&&", + "sh", + "-c", + "..." +}).
133-139: Remove or clarify commented-out codeThe lines appear to be intentionally left out, but commented code can cause confusion and clutter. Consider removing them or adding an explanatory comment if they must remain for reference.
Also applies to: 177-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.dagger/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.dagger/main.go (1)
Learnt from: bupd
PR: container-registry/harbor-next#55
File: .dagger/main.go:98-98
Timestamp: 2025-02-10T02:21:53.194Z
Learning: In Dagger development environment, the "dag" identifier is automatically defined and available for use in .dagger/*.go files, even if it appears undefined to static analysis tools. These files are gitignored and the undefined identifier warning can be safely ignored.
🪛 golangci-lint (1.62.2)
.dagger/main.go
99-99: undefined: dag
(typecheck)
149-149: undefined: dag
(typecheck)
210-210: undefined: dag
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: UTTEST
- GitHub Check: UI_UT
- GitHub Check: OFFLINE
- GitHub Check: APITEST_LDAP
- GitHub Check: APITEST_DB_PROXY_CACHE
- GitHub Check: CodeQL-Build
- GitHub Check: lint
- GitHub Check: APITEST_DB
🔇 Additional comments (2)
.dagger/main.go (2)
99-99: Safe to ignore lint errors on undefineddagStatic analysis flags these lines as undefined
dag, but per your retrieved learnings,dagis automatically defined in the Dagger development environment for.dagger/*.gofiles. It's safe to ignore these warnings.Also applies to: 149-149, 210-210
🧰 Tools
🪛 golangci-lint (1.62.2)
99-99: undefined: dag
(typecheck)
117-117: Avoid passing plaintext credentials via CLI argumentsThis is the same concern from previous reviews. Passing
registryPasswordPlainas a command-line argument exposes sensitive info. Instead, pass the secret via an environment variable or a Dagger secret to prevent accidental leaks.
|
|
||
| const ( | ||
| GOLANGCILINT_VERSION = "v1.61.0" | ||
| GO_VERSION = "latest" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify Go version references for reproducible builds
Currently, the code uses:
- A constant
GO_VERSION = "latest"at line 13 - A hard-coded
"golang:1.23.2"at line 261 - Another hard-coded
"golang:latest"at line 377
This inconsistency can cause non-deterministic builds. Pin the Go version consistently using the GO_VERSION constant:
-const (
- GO_VERSION = "latest"
-)
+const (
+ GO_VERSION = "1.20"
+)
...
-registryBinary := dag.Container().From("golang:1.23.2").
+registryBinary := dag.Container().From(fmt.Sprintf("golang:%s", GO_VERSION)).
...
-From("golang:latest").
+From(fmt.Sprintf("golang:%s", GO_VERSION)).Also applies to: 261-261, 377-377
Daggerize the build pipeline
Summary by CodeRabbit
New Features
dagger/harborwith functionalities for managing container image builds and publishing.main.gofile with methods for image management and publishing.Harborstruct with methods for linting and vulnerability checks for Go code.Bug Fixes
.gitignoreto exclude thebin/directory from version control.Documentation
Chores
go.modfor better readability and management..gitattributesto classify specific files as generated code.