Link helm release #6432
Link helm release #6432
Conversation
…gitops-client-refactoring
There was a problem hiding this comment.
either move file or new code at new path /pkg/deployment/....
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 15200081 | Triggered | PGP Private Key | e082b02 | vendor/github.com/ProtonMail/go-crypto/openpgp/read_write_test_data.go | View secret |
| 15200082 | Triggered | PGP Private Key | e082b02 | vendor/github.com/ProtonMail/go-crypto/openpgp/read_write_test_data.go | View secret |
| 7214865 | Triggered | PGP Private Key | f44a592 | vendor/github.com/ProtonMail/go-crypto/openpgp/read_write_test_data.go | View secret |
| 15200083 | Triggered | PGP Private Key | f44a592 | vendor/github.com/ProtonMail/go-crypto/openpgp/read_write_test_data.go | View secret |
| 15200084 | Triggered | PGP Private Key | e082b02 | vendor/github.com/ProtonMail/go-crypto/openpgp/read_write_test_data.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
|
/review |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #ed35c6
Actionable Suggestions - 6
-
pkg/cluster/environment/bean/bean.go - 1
- Missing certificate field population in adapter · Line 39-39
-
pkg/chart/read/ChartReadService.go - 2
- Improper error handling masks failures · Line 163-163
- Improper error handling masks failures · Line 163-163
-
api/helm-app/service/HelmAppService.go - 2
- Incorrect slice initialization with append · Line 1220-1220
- Incorrect slice initialization with append · Line 1237-1237
-
api/restHandler/app/pipeline/configure/DeploymentPipelineRestHandler.go - 1
- Wrong error variable in success response · Line 2583-2583
Review Details
-
Files reviewed - 35 · Commit Range:
dcfd98b..82503e6- api/helm-app/HelmAppRestHandler.go
- api/helm-app/HelmAppRouter.go
- api/helm-app/bean/bean.go
- api/helm-app/gRPC/applicationClient.go
- api/helm-app/gRPC/applist.pb.go
- api/helm-app/gRPC/applist.proto
- api/helm-app/gRPC/applist_grpc.pb.go
- api/helm-app/service/HelmAppService.go
- api/helm-app/service/adapter/adapter.go
- api/helm-app/service/bean/bean.go
- api/restHandler/app/pipeline/configure/DeploymentPipelineRestHandler.go
- api/router/app/pipeline/configure/PipelineConfigRouter.go
- cmd/external-app/wire_gen.go
- env_gen.json
- env_gen.md
- internal/constants/InternalErrorCode.go
- internal/sql/repository/pipelineConfig/PipelineRepository.go
- pkg/app/appDetails/adapter/adapter.go
- pkg/appStore/installedApp/repository/InstalledAppRepository.go
- pkg/appStore/installedApp/service/bean/bean.go
- pkg/argoApplication/ArgoApplicationServiceExtended.go
- pkg/bean/app.go
- pkg/chart/read/ChartReadService.go
- pkg/cluster/environment/adapter/adapter.go
- pkg/cluster/environment/bean/bean.go
- pkg/deployment/gitOps/config/GitOpsConfigReadService.go
- pkg/deployment/gitOps/git/GitOperationService.go
- pkg/deployment/gitOps/validation/gitOpsValidationService.go
- pkg/deployment/trigger/devtronApps/TriggerService.go
- pkg/deployment/trigger/devtronApps/TriggerService_ent1.go
- pkg/pipeline/DeploymentPipelineConfigService.go
- pkg/pipeline/adapter/adapter.go
- pkg/pipeline/bean/ExternalArgoAppLink.go
- util/GlobalConfig.go
- wire_gen.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at shivam@devtron.ai.
Documentation & Help
AI Code Review powered by
| ClusterToken string `json:"-"` | ||
| InsecureSkipTlsVerify bool `json:"-"` | ||
| ClusterConfig map[string]string `json:"-"` | ||
| ClusterCAData string `json:"-"` |
There was a problem hiding this comment.
The new certificate fields ClusterCAData, ClusterKeyData, and ClusterCertData are added to the struct but the adapter function in pkg/cluster/environment/adapter/adapter.go is not populating these fields from the cluster configuration. This will cause these fields to always be empty even when certificate data is available in the cluster config.
Code suggestion
Check the AI-generated fix before applying
@@ -34,6 +34,9 @@
envBean.InsecureSkipTlsVerify = envModel.Cluster.InsecureSkipTlsVerify
+ envBean.ClusterCAData = envModel.Cluster.Config[commonBean.CertificateAuthorityData]
+ envBean.ClusterKeyData = envModel.Cluster.Config[commonBean.TlsKey]
+ envBean.ClusterCertData = envModel.Cluster.Config[commonBean.CertData]
}
Code Review Run #ed35c6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| chartRef, err := impl.ChartRefReadService.FindById(latestChart.ChartRefId) | ||
| if err != nil { | ||
| impl.logger.Errorw("error in finding latest chart by appId", "appId", appId, "err", err) | ||
| return nil, nil |
There was a problem hiding this comment.
Error handling returns nil, nil instead of propagating the actual error. This will mask failures and make debugging difficult. Change to return nil, err to properly propagate errors to callers.
Code suggestion
Check the AI-generated fix before applying
| return nil, nil | |
| return nil, err |
Code Review Run #ed35c6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| chartRef, err := impl.ChartRefReadService.FindById(latestChart.ChartRefId) | ||
| if err != nil { | ||
| impl.logger.Errorw("error in finding latest chart by appId", "appId", appId, "err", err) | ||
| return nil, nil |
There was a problem hiding this comment.
Error handling returns nil, nil instead of propagating the actual error. This will mask failures and make debugging difficult. Change to return nil, err to properly propagate errors to callers.
Code suggestion
Check the AI-generated fix before applying
| return nil, nil | |
| return nil, err |
Code Review Run #ed35c6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return nil, err | ||
| } | ||
|
|
||
| cdPipeline := make([]*pipelineConfig.PipelineDeploymentConfigObj, len(cdPipelineDbObj)) |
There was a problem hiding this comment.
Slice pre-allocation with append() will create incorrect slice structure. The slice is initialized with length len(cdPipelineDbObj) but then append() adds elements beyond this length, resulting in nil elements at the beginning. Fix: use make([]*pipelineConfig.PipelineDeploymentConfigObj, 0, len(cdPipelineDbObj))
Code suggestion
Check the AI-generated fix before applying
| cdPipeline := make([]*pipelineConfig.PipelineDeploymentConfigObj, len(cdPipelineDbObj)) | |
| cdPipeline := make([]*pipelineConfig.PipelineDeploymentConfigObj, 0, len(cdPipelineDbObj)) |
Code Review Run #ed35c6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return nil, err | ||
| } | ||
|
|
||
| installedApps := make([]bean3.DeployedInstalledAppInfo, len(installedAppDBObj)) |
There was a problem hiding this comment.
Slice pre-allocation with append() will create incorrect slice structure. The slice is initialized with length len(installedAppDBObj) but then append() adds elements beyond this length, resulting in zero-value elements at the beginning. Fix: use make([]bean3.DeployedInstalledAppInfo, 0, len(installedAppDBObj))
Code suggestion
Check the AI-generated fix before applying
| installedApps := make([]bean3.DeployedInstalledAppInfo, len(installedAppDBObj)) | |
| installedApps := make([]bean3.DeployedInstalledAppInfo, 0, len(installedAppDBObj)) |
Code Review Run #ed35c6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } else { | ||
| } else if request.DeploymentAppType == util.PIPELINE_DEPLOYMENT_TYPE_HELM { | ||
| response := handler.pipelineBuilder.ValidateLinkHelmAppRequest(ctx, &request) | ||
| common.WriteJsonResp(w, err, response, http.StatusOK) |
There was a problem hiding this comment.
The WriteJsonResp call uses the wrong error variable err from the earlier user authentication check instead of nil for a successful validation response. This will cause the API to return an error status even when Helm validation succeeds. Replace err with nil.
Code suggestion
Check the AI-generated fix before applying
| common.WriteJsonResp(w, err, response, http.StatusOK) | |
| common.WriteJsonResp(w, nil, response, http.StatusOK) |
Code Review Run #ed35c6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Description
Fixes #
Checklist:
Does this PR introduce a user-facing change?
Summary by Bito
This PR introduces helm release linking functionality by adding new REST endpoints and enhancing gRPC protobuf definitions. It extends data structures to support richer information for helm applications, improves validation logic, and refactors error handling mechanisms. These changes streamline helm release management and strengthen overall system reliability.