zapier / kubechecks

Check your Kubernetes changes before they hit the cluster
https://kubechecks.readthedocs.io/en/latest/
Mozilla Public License 2.0
136 stars 6 forks source link

Update argocd to 2.10.10 #222

Closed ahinh43 closed 1 month ago

ahinh43 commented 1 month ago

Fixes #221

Upgrades the ArgoCD dependency to 2.10.10 and applies a fix for a new argument requirement introduced in the new version

github-actions[bot] commented 1 month ago

Temporary image deleted.

zapier-sre-bot commented 1 month ago

Mergecat's Review

Click to read mergecats review! ## 😼 Mergecat review of go.mod ```diff @@ -5,8 +5,8 @@ go 1.21 toolchain go1.21.6 require ( - github.com/argoproj/argo-cd/v2 v2.10.6 - github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867 + github.com/argoproj/argo-cd/v2 v2.10.10 + github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412 github.com/cenkalti/backoff/v4 v4.3.0 github.com/chainguard-dev/git-urls v1.0.2 github.com/creasty/defaults v1.7.0 @@ -210,7 +210,7 @@ require ( github.com/sergi/go-diff v1.3.1 // indirect github.com/shteou/go-ignore v0.3.1 // indirect github.com/shurcooL/graphql v0.0.0-20230722043721-ed46e5a46466 // indirect - github.com/skeema/knownhosts v1.2.1 // indirect + github.com/skeema/knownhosts v1.2.2 // indirect github.com/sourcegraph/conc v0.3.0 // indirect github.com/spdx/tools-golang v0.5.3 // indirect github.com/spf13/afero v1.11.0 // indirect ``` ### Feedback & Suggestions: 1. **Dependency Updates**: The updates to `github.com/argoproj/argo-cd/v2`, `github.com/argoproj/gitops-engine`, and `github.com/skeema/knownhosts` are generally good practices to keep dependencies up-to-date. However, ensure that these updates are compatible with your codebase and do not introduce breaking changes. Running your test suite after these updates is crucial. 2. **Version Pinning**: The new versions are pinned to specific commits or versions, which is good for reproducibility. However, consider using semantic versioning tags if available, as they provide more context about the nature of the changes (e.g., patch, minor, major). 3. **Security Considerations**: Always review the changelogs or release notes of the updated dependencies for any security patches or vulnerabilities that might have been addressed. This ensures that your application remains secure. 4. **Indirect Dependencies**: The change to `github.com/skeema/knownhosts` is marked as indirect. Ensure that this update does not inadvertently affect other parts of your dependency tree. 5. **Documentation**: Update your documentation or dependency management notes to reflect these changes. This helps other developers understand why these specific versions were chosen. --- By following these suggestions, you can ensure that your dependency management remains robust and your application continues to function correctly and securely. 🛡️🚀 ## 😼 Mergecat review of pkg/checks/diff/diff.go ```diff @@ -6,6 +6,7 @@ import ( "fmt" "io" "strings" + "time" cmdutil "github.com/argoproj/argo-cd/v2/cmd/util" "github.com/argoproj/argo-cd/v2/controller" @@ -14,6 +15,7 @@ import ( argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/util/argo" argodiff "github.com/argoproj/argo-cd/v2/util/argo/diff" + "github.com/argoproj/argo-cd/v2/util/argo/normalizers" "github.com/argoproj/gitops-engine/pkg/diff" "github.com/argoproj/gitops-engine/pkg/sync/hook" "github.com/argoproj/gitops-engine/pkg/sync/ignore" @@ -196,9 +198,12 @@ func generateDiff(ctx context.Context, request checks.Request, argoSettings *set } ignoreAggregatedRoles := false + ignoreNormalizerOpts := normalizers.IgnoreNormalizerOpts{ + JQExecutionTimeout: 1 * time.Second, + } diffConfig, err := argodiff.NewDiffConfigBuilder(). WithLogger(zerologr.New(&log.Logger)). - WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles). + WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles, ignoreNormalizerOpts). WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod). WithNoCache(). Build() ``` ### Feedback & Suggestions: 1. **Timeout Configuration**: The addition of `JQExecutionTimeout` in `ignoreNormalizerOpts` is a good step towards preventing long-running operations. However, it would be beneficial to make this timeout configurable via the request or environment variables to provide flexibility for different use cases. 2. **Error Handling**: Ensure that the new timeout setting is properly handled in the `argodiff.NewDiffConfigBuilder` method. If the timeout is exceeded, it should provide a clear error message indicating the cause. 3. **Documentation**: Update the function documentation to reflect the new parameter `ignoreNormalizerOpts` and its purpose. This will help future developers understand the changes and the reason behind them. 4. **Unit Tests**: Add unit tests to verify the behavior of the `JQExecutionTimeout` setting. Ensure that the timeout is respected and appropriate errors are raised when the timeout is exceeded. 5. **Performance Impact**: Monitor the performance impact of the new timeout setting. While it prevents long-running operations, it might also affect the performance of legitimate operations that require more time. Adjust the timeout value based on performance testing results. ---

Dependency Review

Click to read mergecats review! No suggestions found