feat: application set handler

[Greyeye]

Change

1. Application Set Check:
   • as mentioned in issue here, a new changes how kubechecks handle the ApplicationSet. A change to spec.template.source will show the diff output for the matching Applications.
2. kubechecks Helm Chart Version Up:
   • To accommodate the applicationSet check, kubechecks will need to access Kubernetes applicationSet CRD and security object to list the ArgoCD Clusters information. ClusterRole has been updated to grant these access.
3. new kubechecks-rbac Helm Chart:
   • This helm chart Kubechekcs-rbac can be used to help grant access to argocd CRD and Clusters list from the remotely running Kubechecks. This is no required to be installed when Kubechecks is running on the same cluster with ArgoCD.
4. AppWatcher Initialisation Change:
   • Moved the AppWatcher's kubeclient initialisation to the container. rest.Config is passed as a parameter to AppWatcher.
5. Fork of Generators Code from Argo CD:
   • Forked generator code from Argo CD, This is because the kubechecks' go-gitlab v0.105 is not compatible with argo-cd's v0.91

Local Dev Change

1. Kubernetes Client Package:
   • Implemented a new Kubernetes client specifically for AWS EKS.
   • Automated STS Authentication for EKS Client
   • Added a custom HTTP transport to the EKS client to automate STS token refresh.
2. Extended Cobra Flags and Server Configuration:
   • Added support for kubernetes-type (e.g., eks, local), kubernetes-clusterid in the server configuration and CLI flags.

[github-actions[bot]]

Temporary image deleted.

[Greyeye]

@djeebus @polyrain

would you be kind enough to review the PR?

• You can ignore the pkg/generator directory as it's just a fork from Argo CD.

• New Helm Chart: kubechecks-rbac:

  • I've added a new Helm chart named kubechecks-rbac to be installed where Argo CD is deployed.
  • This chart is intended to allow kubechecks-staging to access Argo CD's CRDs and Cluster list. I haven't had the opportunity to test it yet. If you have any concerns or suggestions for a better approach, I'd greatly appreciate your input.

• Architecture Documentation:

  • I've included an event flow diagram in the architecture documentation. It would be great if you could review this as well.

[djeebus]

I saw your successful test; nice work! I've also confirmed backwards compatibility. :+1:

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

## 😼 Mergecat review of localdev/terraform/modules/vcs_files/base_files/apps/echo-server/in-cluster/values.yaml ```diff @@ -1,2 +1,2 @@ echo-server: - replicaCount: 2 \ No newline at end of file + replicaCount: 1 \ No newline at end of file ``` Feedback & Suggestions: - Ensure there is a newline at the end of the file. This is a good practice for version control systems and can prevent potential issues with file concatenation in some environments. --- ## 😼 Mergecat review of charts/kubechecks/Chart.yaml ```diff @@ -1,7 +1,7 @@ apiVersion: v2 name: kubechecks description: A Helm chart for kubechecks -version: 0.4.4 +version: 0.4.5 type: application maintainers: - name: zapier ``` Feedback & Suggestions: The diff correctly updates the version number from 0.4.4 to 0.4.5. There are no issues with this change. 👍 --- ## 😼 Mergecat review of .mockery.yaml ```diff @@ -5,3 +5,11 @@ packages: # place your package-specific config here config: all: true + github.com/zapier/kubechecks/pkg/generator: + # place your package-specific config here + config: + all: true + github.com/zapier/kubechecks/pkg/affected_apps: + # place your package-specific config here + config: + all: true ``` Feedback & Suggestions: It appears that the new diff is duplicating entries that already exist in the original code. This redundancy can lead to confusion and potential maintenance issues. Please remove the duplicated entries to keep the configuration clean and concise. --- ## 😼 Mergecat review of localdev/.gitignore ```diff @@ -1,4 +1,6 @@ terraform.tfstate* .terraform.lock.hcl .terraform/** -.terraform.tfstate.lock.info \ No newline at end of file +.terraform.tfstate.lock.info +/terraform/modules/vcs_files/base_files/appsets/httpdump/httpdump.yaml +/terraform/modules/vcs_files/base_files/appsets/echo-server/echo-server.yaml ``` Feedback & Suggestions: 1. **Redundant Entries**: The added lines are duplicates of existing entries. This redundancy can lead to confusion and clutter in the `.gitignore` file. Consider removing the duplicate lines. 2. **Newline at End of File**: Ensure there is a newline at the end of the file. This is a good practice for version control and can prevent potential issues with some tools. Suggested Diff: ```diff @@ -1,4 +1,5 @@ terraform.tfstate* .terraform.lock.hcl .terraform/** .terraform.tfstate.lock.info +/terraform/modules/vcs_files/base_files/appsets/httpdump/httpdump.yaml +/terraform/modules/vcs_files/base_files/appsets/echo-server/echo-server.yaml ``` --- ## 😼 Mergecat review of charts/kubechecks/values.yaml ```diff @@ -6,6 +6,7 @@ configMap: env: {} # KUBECHECKS_ARGOCD_API_INSECURE: "false" # KUBECHECKS_ARGOCD_API_PATH_PREFIX: / + # KUBECHECKS_ARGOCD_API_NAMESPACE: argocd # KUBECHECKS_ARGOCD_WEBHOOK_URL: https://argocd./api/webhook # KUBECHECKS_FALLBACK_K8S_VERSION: "1.22.0" # KUBECHECKS_LOG_LEVEL: debug ``` Feedback & Suggestions: 1. **Redundancy**: The added line `# KUBECHECKS_ARGOCD_API_NAMESPACE: argocd` is already present in the original code. This addition is redundant and does not provide any new information. 2. **No Functional Change**: The diff does not introduce any functional changes or improvements. Ensure that any changes made to the configuration file are necessary and add value. --- ## 😼 Mergecat review of pkg/affected_apps/argocd_matcher_test.go ```diff @@ -34,7 +34,7 @@ func TestFindAffectedAppsWithNilAppsDirectory(t *testing.T) { ) matcher := ArgocdMatcher{} - items, err := matcher.AffectedApps(ctx, changeList, "main") + items, err := matcher.AffectedApps(ctx, changeList, "main", nil) // verify results require.NoError(t, err) ``` Feedback & Suggestions: 1. **Contextual Clarity**: The change itself is minor and corrects the function call to include the `nil` parameter. However, it would be beneficial to add a comment explaining why `nil` is being passed to `AffectedApps`. This will help future maintainers understand the context and reasoning behind this test case. 2. **Parameter Validation**: Ensure that the `AffectedApps` method properly handles the `nil` parameter. While this is not directly related to the test, it's a good practice to validate inputs within the method to prevent potential `nil` dereference issues. 3. **Test Coverage**: Consider adding additional test cases to cover different scenarios where the `appsDirectory` might be `nil` or non-`nil`. This will ensure comprehensive test coverage and robustness of the `AffectedApps` method. --- ## 😼 Mergecat review of localdev/argocd/kustomization.yaml ```diff @@ -6,7 +6,7 @@ namespace: kubechecks images: - name: quay.io/argoproj/argocd newName: quay.io/argoproj/argocd - newTag: v2.6.12 + newTag: v2.11.3 resources: - argocd-initial-admin-secret.yaml ``` Feedback & Suggestions: 1. **Version Compatibility**: Ensure that the new version `v2.11.3` is compatible with the rest of your configuration and other dependencies. Sometimes, major version changes can introduce breaking changes. 2. **Testing**: Before deploying this change to production, thoroughly test it in a staging environment to ensure that the new version does not introduce any unexpected behavior or bugs. 3. **Documentation**: Update any relevant documentation to reflect the change in the ArgoCD version. This helps maintainers and users understand the current state of the system. 4. **Changelog Review**: Review the changelog for `v2.11.3` to understand what has changed since `v2.6.12`. This can help you anticipate any potential issues or new features that might be useful. --- ## 😼 Mergecat review of charts/kubechecks/templates/clusterrole.yaml ```diff @@ -4,5 +4,8 @@ metadata: name: {{ include "kubechecks.fullname" . }} rules: - apiGroups: ['argoproj.io'] - resources: ['applications', 'appprojects', 'services'] + resources: ['applications', 'appprojects', 'applicationsets', 'services'] + verbs: ['get', 'list', 'watch'] + - apiGroups: [''] # The core API group, which is indicated by an empty string + resources: ['secrets'] verbs: ['get', 'list', 'watch'] ``` ### Feedback & Suggestions: 1. **Redundant Verbs Declaration**: The `verbs` for the `argoproj.io` API group are already declared in the original code. Re-declaring them in the diff is redundant and can be removed. 2. **Formatting Consistency**: Ensure consistent indentation and spacing for better readability. ### Suggested Diff: ```diff @@ -4,5 +4,6 @@ metadata: name: {{ include "kubechecks.fullname" . }} rules: - apiGroups: ['argoproj.io'] - resources: ['applications', 'appprojects', 'services'] + resources: ['applications', 'appprojects', 'applicationsets', 'services'] - apiGroups: [''] # The core API group, which is indicated by an empty string resources: ['secrets'] verbs: ['get', 'list', 'watch'] ``` --- ## 😼 Mergecat review of pkg/events/worker.go ```diff @@ -5,11 +5,11 @@ import ( "fmt" "sync/atomic" - "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - "github.com/rs/zerolog" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/rs/zerolog" "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/argo_client" "github.com/zapier/kubechecks/pkg/checks" ``` Feedback & Suggestions: - The diff only reorders the import statements for `v1alpha1` and `zerolog`. While this change does not introduce any functional issues, it does not provide any significant improvement either. - If the goal is to organize imports, consider grouping standard library imports, third-party imports, and local imports separately for better readability. For example: ```go import ( "context" "fmt" "sync/atomic" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/rs/zerolog" "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/argo_client" "github.com/zapier/kubechecks/pkg/checks" "github.com/zapier/kubechecks/pkg/container" "github.com/zapier/kubechecks/pkg/git" "github.com/zapier/kubechecks/pkg/msg" "github.com/zapier/kubechecks/telemetry" ) ``` This approach makes it easier to distinguish between different types of imports. 🛠️ --- ## 😼 Mergecat review of localdev/test_apps/Tiltfile ```diff @@ -13,11 +13,12 @@ def install_test_apps(cfg): projectUrl=str(read_file(vcsPath, "")).strip('\n') print("Remote Project URL: " + projectUrl) - for app in ["echo-server", "httpbin"]: + for app in ["echo-server", "httpbin", "app-root"]: print("Creating Test App: " + app) # read the application YAML and patch the repoURL objects = read_yaml_stream("localdev/test_apps/{}.yaml".format(app)) + for o in objects: o['metadata']['namespace'] = "kubechecks" o['spec']['source']['repoURL'] = projectUrl ``` Feedback & Suggestions: 1. **Redundant Addition**: The diff adds `"app-root"` to the list of applications, but this is already present in the original code. This change is redundant and should be removed to avoid confusion. 2. **Whitespace Addition**: The diff introduces an unnecessary blank line after `objects = read_yaml_stream("localdev/test_apps/{}.yaml".format(app))`. While this doesn't affect functionality, it can clutter the code. Consider removing it for cleaner code. Suggested diff: ```diff @@ -13,11 +13,12 @@ def install_test_apps(cfg): projectUrl=str(read_file(vcsPath, "")).strip('\n') print("Remote Project URL: " + projectUrl) - for app in ["echo-server", "httpbin"]: + for app in ["echo-server", "httpbin", "app-root"]: print("Creating Test App: " + app) # read the application YAML and patch the repoURL objects = read_yaml_stream("localdev/test_apps/{}.yaml".format(app)) - + for o in objects: o['metadata']['namespace'] = "kubechecks" o['spec']['source']['repoURL'] = projectUrl ``` --- ## 😼 Mergecat review of pkg/argo_client/applications.go ```diff @@ -132,8 +132,8 @@ func (argo *ArgoClient) GetApplicationSets(ctx context.Context) (*v1alpha1.Appli resp, err := appClient.List(ctx, new(applicationset.ApplicationSetListQuery)) if err != nil { - telemetry.SetError(span, err, "Argo List All Applications error") - return nil, errors.Wrap(err, "failed to applications") + telemetry.SetError(span, err, "Argo List All Application Sets error") + return nil, errors.Wrap(err, "failed to list application sets") } return resp, nil } ``` ### Feedback & Suggestions: 1. **Error Message Consistency**: The changes correctly update the error messages to reflect the context of listing application sets instead of applications. This improves clarity and debugging. 2. **Code Quality**: The changes improve the accuracy of the error messages, which is crucial for maintaining high code quality and ease of maintenance. 3. **Security**: No security issues are introduced by this change. 4. **Performance**: No performance issues are introduced by this change. Overall, the changes are appropriate and improve the clarity of the error messages. No further improvements are necessary for this diff. 👍 --- ## 😼 Mergecat review of pkg/affected_apps/matcher.go ```diff @@ -5,11 +5,12 @@ import ( "path" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/zapier/kubechecks/pkg/git" ) type AffectedItems struct { Applications []v1alpha1.Application - ApplicationSets []ApplicationSet + ApplicationSets []v1alpha1.ApplicationSet } func (ai AffectedItems) Union(other AffectedItems) AffectedItems { @@ -48,7 +49,7 @@ type ApplicationSet struct { } type Matcher interface { - AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) + AffectedApps(ctx context.Context, changeList []string, targetBranch string, repo *git.Repo) (AffectedItems, error) } // modifiedDirs filters a list of changed files down to a list ``` Feedback & Suggestions: 1. **Import Duplication**: The `github.com/zapier/kubechecks/pkg/git` package is imported twice. This is redundant and should be cleaned up. - **Fix**: Remove the duplicate import statement. 2. **Type Consistency**: The change from `[]ApplicationSet` to `[]v1alpha1.ApplicationSet` is correct for consistency, but ensure that the `ApplicationSet` type defined in the original code is no longer needed and can be safely removed. - **Fix**: Remove the `ApplicationSet` type definition if it is no longer used elsewhere in the code. 3. **Contextual Clarity**: The addition of the `repo *git.Repo` parameter to the `AffectedApps` method in the `Matcher` interface is a good change for providing more context. Ensure that all implementations of this interface are updated accordingly to avoid runtime errors. - **Fix**: Verify and update all implementations of the `Matcher` interface to include the new `repo *git.Repo` parameter. --- By addressing these points, the code will be cleaner, more consistent, and less prone to errors. 🛠️ ## 😼 Mergecat review of pkg/config/config.go ```diff @@ -17,12 +17,14 @@ import ( type ServerConfig struct { // argocd - ArgoCDServerAddr string `mapstructure:"argocd-api-server-addr"` - ArgoCDToken string `mapstructure:"argocd-api-token"` - ArgoCDPathPrefix string `mapstructure:"argocd-api-path-prefix"` - ArgoCDInsecure bool `mapstructure:"argocd-api-insecure"` - ArgoCDNamespace string `mapstructure:"argocd-api-namespace"` - KubernetesConfig string `mapstructure:"kubernetes-config"` + ArgoCDServerAddr string `mapstructure:"argocd-api-server-addr"` + ArgoCDToken string `mapstructure:"argocd-api-token"` + ArgoCDPathPrefix string `mapstructure:"argocd-api-path-prefix"` + ArgoCDInsecure bool `mapstructure:"argocd-api-insecure"` + ArgoCDNamespace string `mapstructure:"argocd-api-namespace"` + KubernetesConfig string `mapstructure:"kubernetes-config"` + KubernetesType string `mapstructure:"kubernetes-type"` + KubernetesClusterID string `mapstructure:"kubernetes-clusterid"` // otel EnableOtel bool `mapstructure:"otel-enabled"` ``` Feedback & Suggestions: 1. **Security**: Ensure that sensitive information such as `ArgoCDToken` and `VcsToken` are handled securely. Consider adding comments or documentation to remind developers to avoid logging these values or exposing them in error messages. 2. **Consistency**: The indentation and alignment changes are good for readability. Ensure that this style is consistently applied throughout the codebase. 3. **Validation**: Consider adding validation for the new fields `KubernetesType` and `KubernetesClusterID` to ensure they meet expected formats or values. This can prevent misconfigurations and potential runtime errors. 4. **Documentation**: Update any relevant documentation to reflect the addition of `KubernetesType` and `KubernetesClusterID` fields. This helps maintainers and users understand the new configuration options. --- ## 😼 Mergecat review of localdev/test_appsets/httpdump.yaml ```diff @@ -5,6 +5,7 @@ metadata: namespace: kubechecks spec: generators: + # this is a simple list generator - list: elements: - name: a @@ -16,7 +17,7 @@ spec: finalizers: - resources-finalizer.argocd.argoproj.io name: "in-cluster-{{ name }}-httpdump" - namespace: argocd + namespace: kubechecks labels: argocd.argoproj.io/application-set-name: "httpdump" spec: @@ -25,11 +26,12 @@ spec: server: '{{ url }}' project: default source: - repoURL: ${REPO_URL} + repoURL: REPO_URL targetRevision: HEAD path: 'apps/httpdump/overlays/{{ name }}/' syncPolicy: automated: prune: true syncOptions: - - CreateNamespace=true \ No newline at end of file + - CreateNamespace=true + sources: [] ``` Feedback & Suggestions: 1. **Comment Placement**: The comment `# this is a simple list generator` was added in the diff. While this is helpful, it would be more readable if it were placed above the `generators` key instead of within the list. This makes it clear that the comment applies to the entire `generators` section. ```diff spec: + # this is a simple list generator generators: ``` 2. **Environment Variable Usage**: The change from `${REPO_URL}` to `REPO_URL` might be unintentional. If `REPO_URL` is intended to be an environment variable, it should be kept as `${REPO_URL}`. If it's a placeholder, consider adding a comment to clarify. ```diff - repoURL: ${REPO_URL} + repoURL: REPO_URL # Ensure this is not an environment variable ``` 3. **Newline at End of File**: Ensure there is a newline at the end of the file. This is a common best practice in many coding standards. ```diff + sources: [] + # Ensure there is a newline at the end of the file ``` --- ## 😼 Mergecat review of pkg/appdir/app_directory.go ```diff @@ -58,13 +58,18 @@ func (d *AppDirectory) ProcessApp(app v1alpha1.Application) { } } +// FindAppsBasedOnChangeList receives a list of modified file paths and +// returns the list of applications that are affected by the changes. +// +// changeList: a slice of strings representing the paths of modified files. +// targetBranch: the branch name to compare against the target revision of the applications. +// e.g. changeList = ["path/to/file1", "path/to/file2"] func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBranch string) []v1alpha1.Application { log.Debug().Msgf("checking %d changes", len(changeList)) appsSet := make(map[string]struct{}) for _, changePath := range changeList { log.Debug().Msgf("change: %s", changePath) - for dir, appNames := range d.appDirs { if strings.HasPrefix(changePath, dir) { log.Debug().Msg("dir match!") @@ -146,10 +151,15 @@ func (d *AppDirectory) GetApps(filter func(stub v1alpha1.Application) bool) []v1 } func (d *AppDirectory) AddApp(app v1alpha1.Application) { + if _, exists := d.appsMap[app.Name]; exists { + log.Debug().Msgf("app %s already exists", app.Name) + return + } log.Debug(). Str("appName", app.Name). Str("cluster-name", app.Spec.Destination.Name). Str("cluster-server", app.Spec.Destination.Server). + Str("source", getSourcePath(app)). Msg("add app") d.appsMap[app.Name] = app d.AddDir(app.Name, getSourcePath(app)) ``` Feedback & Suggestions: 1. **Documentation Addition**: The added comments for `FindAppsBasedOnChangeList` are helpful for understanding the function's purpose and parameters. 👍 2. **Redundant Logging**: The added log statement in `AddApp` to check if the app already exists is useful. However, consider using `log.Warn()` instead of `log.Debug()` to make it more noticeable, as this is an important event. 3. **Performance Improvement**: In `FindAppsBasedOnChangeList`, the nested loop checking for directory matches could be optimized. If `d.appDirs` is large, this could become a performance bottleneck. Consider using a more efficient data structure or algorithm to reduce the time complexity. 4. **Security Consideration**: Ensure that the paths in `changeList` are sanitized to prevent any potential path traversal vulnerabilities. This is especially important if the paths are derived from user input. 5. **Consistency**: The spacing in the diff is inconsistent. There is an extra blank line removed after `log.Debug().Msgf("change: %s", changePath)`. Ensure consistent formatting for better readability. --- ## 😼 Mergecat review of pkg/affected_apps/multi_matcher.go ```diff @@ -4,6 +4,7 @@ import ( "context" "github.com/pkg/errors" + "github.com/zapier/kubechecks/pkg/git" ) func NewMultiMatcher(matchers ...Matcher) Matcher { @@ -14,11 +15,11 @@ type MultiMatcher struct { matchers []Matcher } -func (m MultiMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) { +func (m MultiMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string, repo *git.Repo) (AffectedItems, error) { var total AffectedItems for index, matcher := range m.matchers { - items, err := matcher.AffectedApps(ctx, changeList, targetBranch) + items, err := matcher.AffectedApps(ctx, changeList, targetBranch, repo) if err != nil { return total, errors.Wrapf(err, "failed to find items in matcher #%d", index) } ``` Feedback & Suggestions: 1. **Redundant Import**: The import statement for `github.com/zapier/kubechecks/pkg/git` is already present in the original code. The diff reintroduces it, which is unnecessary and can be removed. 2. **Contextual Consistency**: Ensure that all matchers in the `MultiMatcher` struct are updated to handle the new `repo` parameter. This change should be reflected in all places where `AffectedApps` is called to avoid runtime errors. 3. **Documentation**: Consider adding comments to the `AffectedApps` method to explain the purpose of the `repo` parameter for better code readability and maintainability. Here is the improved diff: ```diff @@ -4,6 +4,7 @@ import ( "context" "github.com/pkg/errors" ) func NewMultiMatcher(matchers ...Matcher) Matcher { @@ -14,11 +15,11 @@ type MultiMatcher struct { matchers []Matcher } -func (m MultiMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) { +func (m MultiMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string, repo *git.Repo) (AffectedItems, error) { var total AffectedItems for index, matcher := range m.matchers { - items, err := matcher.AffectedApps(ctx, changeList, targetBranch) + items, err := matcher.AffectedApps(ctx, changeList, targetBranch, repo) if err != nil { return total, errors.Wrapf(err, "failed to find items in matcher #%d", index) } ``` --- ## 😼 Mergecat review of pkg/affected_apps/config_matcher.go ```diff @@ -9,6 +9,8 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/pkg/errors" "github.com/rs/zerolog/log" + "github.com/zapier/kubechecks/pkg/git" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/zapier/kubechecks/pkg/container" "github.com/zapier/kubechecks/pkg/repo_config" @@ -28,9 +30,9 @@ func NewConfigMatcher(cfg *repo_config.Config, ctr container.Container) *ConfigM return &ConfigMatcher{cfg: cfg, argoClient: ctr.ArgoClient} } -func (b *ConfigMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) { +func (b *ConfigMatcher) AffectedApps(ctx context.Context, changeList []string, _ string, _ *git.Repo) (AffectedItems, error) { triggeredAppsMap := make(map[string]string) - var appSetList []ApplicationSet + var appSetList []v1alpha1.ApplicationSet triggeredApps, triggeredAppsets, err := b.triggeredApps(ctx, changeList) if err != nil { @@ -42,7 +44,11 @@ func (b *ConfigMatcher) AffectedApps(ctx context.Context, changeList []string, t } for _, appset := range triggeredAppsets { - appSetList = append(appSetList, ApplicationSet{appset.Name}) + appSetList = append(appSetList, v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: appset.Name, + }, + }) } allArgoApps, err := b.argoClient.GetApplications(ctx) ``` Feedback & Suggestions: 1. **Unused Imports**: The imports for `github.com/zapier/kubechecks/pkg/git` and `k8s.io/apimachinery/pkg/apis/meta/v1` were already present in the original code. Ensure that these imports are necessary and not redundant. 2. **Parameter Naming**: The underscore `_` for the parameters in `AffectedApps` function is a good practice to indicate unused parameters. However, ensure that this change aligns with the overall design and usage of the function. 3. **Type Consistency**: The change from `ApplicationSet` to `v1alpha1.ApplicationSet` is appropriate. Ensure that this change is consistent throughout the codebase to avoid type mismatches. 4. **Error Handling**: Consider adding more context to the error messages for better debugging. For example, in the `triggeredApps` function, the error message could include more details about the context in which the error occurred. Overall, the changes look good and improve the type consistency in the code. 🛠️ --- ## 😼 Mergecat review of docs/architecture.md ```diff @@ -36,3 +36,10 @@ By abstracting the PR/MR in this way, `kubechecks` remains VCS provider agnostic {: style="height:350px;display:block;margin:0 auto;} The final piece of the puzzle is the `CheckEvent`; an internal structure that takes a `Client` and a `Repo` and begins running all configured checks. A `CheckEvent` first determines what applications within the repository have been affected by the PR/MR, and begins concurrently running the check suite against each affected application to generate a report for that app. As each application updates its report, the `CheckEvent` compiles all reports together and instructs the `Client` to update the PR/MR with a comment detailing the current progress; resulting in one comment per run of `kubechecks` with the latest information about that particular run. Whenever a new run of `kubechecks` is initiated, all previous comments are deleted to reduce clutter. + +### Event Flow Diagram + +{: style="height:350px;display:block;margin:0 auto;} + +This diagram illustrates the flow of events from the initial webhook trigger to the final report generation and comment update process. + ``` Feedback & Suggestions: 1. **Consistency in Image Styling**: Ensure that the styling for the new image matches the existing images for consistency. The current diff uses `{: style="height:350px;display:block;margin:0 auto;"}` which is consistent, so no changes needed here. 👍 2. **Content Flow**: The addition of the "Event Flow Diagram" section is well-placed and logically follows the explanation of the `CheckEvent`. This helps in visualizing the process described. Good job! 👏 3. **Grammar and Clarity**: The new text is clear and grammatically correct. No changes needed here. ✅ Overall, the diff is well-constructed and integrates seamlessly with the existing document. No further improvements are necessary. Great work! 🌟 --- ## 😼 Mergecat review of pkg/app_watcher/app_watcher.go ```diff @@ -2,18 +2,18 @@ package app_watcher import ( "context" + "fmt" "reflect" "strings" "time" - appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned" - "github.com/rs/zerolog/log" - "k8s.io/client-go/tools/clientcmd" - appv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned" informers "github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions/application/v1alpha1" applisters "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1" + "github.com/rs/zerolog/log" "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "github.com/zapier/kubechecks/pkg/appdir" @@ -29,18 +29,17 @@ type ApplicationWatcher struct { vcsToArgoMap appdir.VcsToArgoMap } -// NewApplicationWatcher creates new instance of ApplicationWatcher. -func NewApplicationWatcher(vcsToArgoMap appdir.VcsToArgoMap, cfg config.ServerConfig) (*ApplicationWatcher, error) { - // this assumes kubechecks is running inside the cluster - kubeCfg, err := clientcmd.BuildConfigFromFlags("", cfg.KubernetesConfig) - if err != nil { - log.Fatal().Msgf("Error building kubeconfig: %s", err.Error()) +// NewApplicationWatcher creates a new instance of ApplicationWatcher. +// +// - kubeCfg is the Kubernetes configuration. +// - vcsToArgoMap is the mapping between VCS and Argo applications. +// - cfg is the server configuration. +func NewApplicationWatcher(kubeCfg *rest.Config, vcsToArgoMap appdir.VcsToArgoMap, cfg config.ServerConfig) (*ApplicationWatcher, error) { + if kubeCfg == nil { + return nil, fmt.Errorf("kubeCfg cannot be nil") } - - appClient := appclientset.NewForConfigOrDie(kubeCfg) - ctrl := ApplicationWatcher{ - applicationClientset: appClient, + applicationClientset: appclientset.NewForConfigOrDie(kubeCfg), vcsToArgoMap: vcsToArgoMap, } ``` Feedback & Suggestions: 1. **Import Order**: The import order was changed, but it would be better to group standard library imports, third-party imports, and local imports separately for better readability. This is a common Go convention. 2. **Error Handling**: The original code had error handling for building the Kubernetes config (`clientcmd.BuildConfigFromFlags`). The new code assumes `kubeCfg` is always provided and valid. Ensure that the calling code guarantees this, or consider adding a fallback or error handling mechanism. 3. **Documentation**: The updated function documentation is good, but ensure that the rest of the codebase follows similar documentation standards for consistency. --- ## 😼 Mergecat review of Tiltfile ```diff @@ -5,6 +5,7 @@ load('ext://tests/golang', 'test_go') load('ext://namespace', 'namespace_create') load('ext://uibutton', 'cmd_button') load('ext://helm_resource', 'helm_resource') +load('ext://local_output', 'local_output') load('./.tilt/terraform/Tiltfile', 'local_terraform_resource') load('./.tilt/utils/Tiltfile', 'check_env_set') @@ -144,10 +145,6 @@ test_go( ) -# get the git commit ref -def get_git_head(): - result = local('git rev-parse --short HEAD') - return result # read .tool-versions file and return a dictionary of tools and their versions def parse_tool_versions(fn): @@ -174,7 +171,9 @@ def parse_tool_versions(fn): return tools tool_versions = parse_tool_versions(".tool-versions") -git_commit = str(get_git_head()).strip() + +# get the git commit ref +git_commit = local_output('git rev-parse --short HEAD') earthly_build( context='.', @@ -260,8 +259,8 @@ k8s_resource( load("localdev/test_apps/Tiltfile", "install_test_apps") install_test_apps(cfg) -load("localdev/test_appsets/Tiltfile", "install_test_appsets") -install_test_appsets(cfg) +load("localdev/test_appsets/Tiltfile", "copy_test_appsets") +copy_test_appsets(cfg) force_argocd_cleanup_on_tilt_down() ``` Feedback & Suggestions: 1. **Redundant Load Statement**: - The `load('ext://local_output', 'local_output')` statement is redundant since it is already present in the original code. This can be removed to avoid unnecessary duplication. ```diff @@ -5,7 +5,6 @@ load('ext://tests/golang', 'test_go') load('ext://namespace', 'namespace_create') load('ext://uibutton', 'cmd_button') load('ext://helm_resource', 'helm_resource') -load('ext://local_output', 'local_output') load('./.tilt/terraform/Tiltfile', 'local_terraform_resource') load('./.tilt/utils/Tiltfile', 'check_env_set') ``` 2. **Security Consideration**: - The `local_output('git rev-parse --short HEAD')` command execution is fine, but ensure that the `local_output` function is secure and handles any potential command injection vulnerabilities. If `local_output` is a wrapper around `os.system` or similar, it should sanitize inputs. 3. **Function Removal**: - The removal of the `get_git_head` function is appropriate since it is replaced by `local_output`. Ensure that `local_output` is a reliable and tested function. 4. **Consistency in Function Naming**: - The change from `install_test_appsets` to `copy_test_appsets` should be consistent with the function's purpose. Ensure that the new function name accurately reflects its functionality. ```diff @@ -260,8 +259,8 @@ k8s_resource( load("localdev/test_apps/Tiltfile", "install_test_apps") install_test_apps(cfg) -load("localdev/test_appsets/Tiltfile", "install_test_appsets") -install_test_appsets(cfg) +load("localdev/test_appsets/Tiltfile", "copy_test_appsets") +copy_test_appsets(cfg) ``` 5. **Documentation**: - Consider adding comments to explain why certain changes were made, especially for future maintainers who might wonder why `local_output` was chosen over the previous method. --- ## 😼 Mergecat review of localdev/test_appsets/Tiltfile ```diff @@ -3,28 +3,20 @@ # Test ArgoCD Applications # ///////////////////////////////////////////////////////////////////////////// -def install_test_appsets(cfg): +def copy_test_appsets(cfg): # Load the terraform url we output, default to gitlab if cant find a vcs-type variable vcsPath = "./localdev/terraform/{}/project.url".format(cfg.get('vcs-type', 'gitlab')) print("Path to url: " + vcsPath) projectUrl=str(read_file(vcsPath, "")).strip('\n') print("Remote Project URL: " + projectUrl) - k8s_kind('ApplicationSets', api_version="apiextensions.k8s.io/v1") - if projectUrl != "": - for appset in ["httpdump"]: - print("Creating Test ApplicationSet: " + appset) + for appset in ["httpdump","echo-server"]: + source_file = "./localdev/test_appsets/{}.yaml".format(appset) + dest_file = "./localdev/terraform/modules/vcs_files/base_files/appsets/{}/{}.yaml".format(appset,appset) - # read the application YAML and patch the repoURL - objects = read_yaml_stream("localdev/test_appsets/{}.yaml".format(appset)) - for o in objects: - o['spec']['template']['spec']['source']['repoURL'] = projectUrl - k8s_yaml(encode_yaml_stream(objects)) + # Copy the file to the specific terraform directory + local("mkdir -p ./localdev/terraform/modules/vcs_files/base_files/appsets/{} && cp {} {}".format(appset, source_file, dest_file)) - k8s_resource( - new_name=appset, - objects=['{}:applicationset'.format(appset)], - labels=["test_appsets"], - resource_deps=["argocd-crds","argocd"], - ) + # Modify the copied file to replace ${REPO_URL} with projectUrl + local("sed -i '' 's#REPO_URL#{}#g' {}".format(projectUrl, dest_file)) ``` Feedback & Suggestions: 1. **Security**: Using `local` to execute shell commands can be risky, especially if `projectUrl` or other variables are not properly sanitized. Consider using Python's `subprocess` module with proper escaping to avoid shell injection vulnerabilities. ```python import subprocess subprocess.run(["mkdir", "-p", f"./localdev/terraform/modules/vcs_files/base_files/appsets/{appset}"]) subprocess.run(["cp", source_file, dest_file]) subprocess.run(["sed", "-i", "", f"s#REPO_URL#{projectUrl}#g", dest_file]) ``` 2. **Error Handling**: The current code does not handle potential errors that might occur during file operations or command executions. Adding error handling can make the script more robust. ```python import os try: os.makedirs(f"./localdev/terraform/modules/vcs_files/base_files/appsets/{appset}", exist_ok=True) shutil.copy(source_file, dest_file) with open(dest_file, 'r') as file: filedata = file.read() filedata = filedata.replace('REPO_URL', projectUrl) with open(dest_file, 'w') as file: file.write(filedata) except Exception as e: print(f"Error: {e}") ``` 3. **Performance**: The `local` command is called multiple times in a loop, which can be inefficient. Using Python's built-in file handling and directory management functions can improve performance. 4. **Logging**: Instead of using `print` statements, consider using the `logging` module for better control over logging levels and outputs. ```python import logging logging.basicConfig(level=logging.INFO) logging.info(f"Path to url: {vcsPath}") logging.info(f"Remote Project URL: {projectUrl}") ``` By addressing these points, the code will be more secure, robust, and maintainable. 🛡️🚀 --- ## 😼 Mergecat review of pkg/affected_apps/multi_matcher_test.go ```diff @@ -6,14 +6,15 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/stretchr/testify/require" + "github.com/zapier/kubechecks/pkg/git" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) type fakeMatcher struct { items AffectedItems } -func (f fakeMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) { +func (f fakeMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string, repo *git.Repo) (AffectedItems, error) { return f.items, nil } @@ -29,7 +30,7 @@ func TestMultiMatcher(t *testing.T) { ctx := context.Background() matcher := NewMultiMatcher(matcher1, matcher2) - total, err := matcher.AffectedApps(ctx, nil, "") + total, err := matcher.AffectedApps(ctx, nil, "", nil) require.NoError(t, err) require.Len(t, total.Applications, 1) @@ -47,7 +48,7 @@ func TestMultiMatcher(t *testing.T) { ctx := context.Background() matcher := NewMultiMatcher(matcher1, matcher2) - total, err := matcher.AffectedApps(ctx, nil, "") + total, err := matcher.AffectedApps(ctx, nil, "", nil) require.NoError(t, err) require.Len(t, total.Applications, 1) @@ -69,7 +70,7 @@ func TestMultiMatcher(t *testing.T) { ctx := context.Background() matcher := NewMultiMatcher(matcher1, matcher2) - total, err := matcher.AffectedApps(ctx, nil, "") + total, err := matcher.AffectedApps(ctx, nil, "", nil) require.NoError(t, err) require.Len(t, total.Applications, 1) @@ -92,7 +93,7 @@ func TestMultiMatcher(t *testing.T) { ctx := context.Background() matcher := NewMultiMatcher(matcher1, matcher2) - total, err := matcher.AffectedApps(ctx, nil, "") + total, err := matcher.AffectedApps(ctx, nil, "", nil) require.NoError(t, err) require.Len(t, total.Applications, 2) ``` Feedback & Suggestions: 1. **Unused Import**: The import statement for `github.com/zapier/kubechecks/pkg/git` was added, but it is not used in the code. If it is not necessary, it should be removed to keep the code clean. ```diff - "github.com/zapier/kubechecks/pkg/git" ``` 2. **Function Signature Change**: The function signature for `AffectedApps` in `fakeMatcher` was changed to include an additional `repo *git.Repo` parameter. Ensure that this change is intentional and that the `repo` parameter is used appropriately in the actual implementation of `AffectedApps`. If `repo` is not used, it should be removed to avoid confusion. 3. **Consistency in Function Calls**: The changes in the test cases to include `nil` for the `repo` parameter are correct. Ensure that all calls to `AffectedApps` in the rest of the codebase are updated accordingly to avoid potential runtime errors. 4. **Documentation**: If the `repo` parameter is a new addition, consider updating the function documentation to reflect this change. This will help other developers understand the purpose of the new parameter. --- ## 😼 Mergecat review of docs/usage.md ```diff @@ -46,7 +46,9 @@ The full list of supported environment variables is described below: |`KUBECHECKS_ENABLE_PREUPGRADE`|Enable preupgrade checks.|`true`| |`KUBECHECKS_ENSURE_WEBHOOKS`|Ensure that webhooks are created in repositories referenced by argo.|`false`| |`KUBECHECKS_FALLBACK_K8S_VERSION`|Fallback target Kubernetes version for schema / upgrade checks.|`1.23.0`| +|`KUBECHECKS_KUBERNETES_CLUSTERID`|Kubernetes Cluster ID, must be specified if kubernetes-type is eks.|| |`KUBECHECKS_KUBERNETES_CONFIG`|Path to your kubernetes config file, used to monitor applications.|| +|`KUBECHECKS_KUBERNETES_TYPE`|Kubernetes Type One of eks, or local.|`local`| |`KUBECHECKS_LABEL_FILTER`|(Optional) If set, The label that must be set on an MR (as "kubechecks:") for kubechecks to process the merge request webhook.|| |`KUBECHECKS_LOG_LEVEL`|Set the log output level. One of error, warn, info, debug, trace.|`info`| |`KUBECHECKS_MAX_CONCURRENCT_CHECKS`|Number of concurrent checks to run.|`32`| @@ -59,7 +61,7 @@ The full list of supported environment variables is described below: |`KUBECHECKS_PERSIST_LOG_LEVEL`|Persists the set log level down to other module loggers.|`false`| |`KUBECHECKS_POLICIES_LOCATION`|Sets rego policy locations to be used for every check request. Can be common path inside the repos being checked or git urls in either git or http(s) format.|`[./policies]`| |`KUBECHECKS_REPO_REFRESH_INTERVAL`|Interval between static repo refreshes (for schemas and policies).|`5m`| -|`KUBECHECKS_SCHEMAS_LOCATION`|Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.|`[./schemas]`| +|`KUBECHECKS_SCHEMAS_LOCATION`|Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.|`[]`| |`KUBECHECKS_SHOW_DEBUG_INFO`|Set to true to print debug info to the footer of MR comments.|`false`| |`KUBECHECKS_TIDY_OUTDATED_COMMENTS_MODE`|Sets the mode to use when tidying outdated comments. One of hide, delete.|`hide`| |`KUBECHECKS_VCS_BASE_URL`|VCS base url, useful if self hosting gitlab, enterprise github, etc.|| ``` Feedback & Suggestions: 1. **Consistency in Descriptions**: Ensure that the descriptions for the new environment variables (`KUBECHECKS_KUBERNETES_CLUSTERID` and `KUBECHECKS_KUBERNETES_TYPE`) are consistent with the existing ones. For example, `KUBECHECKS_KUBERNETES_TYPE` should have a period at the end of the description. 2. **Default Values**: For `KUBECHECKS_KUBERNETES_CLUSTERID`, it might be helpful to specify a default value or explicitly state "No default value" to maintain consistency with other entries. 3. **Schema Location Default Value**: Changing the default value of `KUBECHECKS_SCHEMAS_LOCATION` from `[./schemas]` to `[]` might have implications. Ensure this change is intentional and documented, as it could affect users relying on the previous default. --- ## 😼 Mergecat review of cmd/root.go ```diff @@ -63,6 +63,11 @@ func init() { stringFlag(flags, "argocd-api-namespace", "ArgoCD namespace where the application watcher will read Custom Resource Definitions (CRD) for Application and ApplicationSet resources.", newStringOpts(). withDefault("argocd")) + stringFlag(flags, "kubernetes-type", "Kubernetes Type One of eks, or local. Defaults to local.", + newStringOpts(). + withChoices("eks", "local"). + withDefault("local")) + stringFlag(flags, "kubernetes-clusterid", "Kubernetes Cluster ID, must be specified if kubernetes-type is eks.") stringFlag(flags, "kubernetes-config", "Path to your kubernetes config file, used to monitor applications.") stringFlag(flags, "otel-collector-port", "The OpenTelemetry collector port.") @@ -73,10 +78,7 @@ func init() { newStringOpts(). withChoices("hide", "delete"). withDefault("hide")) - stringSliceFlag(flags, "schemas-location", "Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.", - newStringSliceOpts(). - withDefault([]string{"./schemas"})) - + stringSliceFlag(flags, "schemas-location", "Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.") boolFlag(flags, "enable-conftest", "Set to true to enable conftest policy checking of manifests.") stringSliceFlag(flags, "policies-location", "Sets rego policy locations to be used for every check request. Can be common path inside the repos being checked or git urls in either git or http(s) format.", newStringSliceOpts(). ``` ### Feedback & Suggestions: 1. **Consistency in Default Values**: - The `schemas-location` flag previously had a default value of `./schemas`. Removing this default value might lead to unexpected behavior if the user does not provide a value. Consider retaining the default value for consistency and to avoid potential issues. ```diff - stringSliceFlag(flags, "schemas-location", "Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.", - newStringSliceOpts(). - withDefault([]string{"./schemas"})) + stringSliceFlag(flags, "schemas-location", "Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.", + newStringSliceOpts(). + withDefault([]string{"./schemas"})) ``` 2. **Error Handling for Kubernetes Cluster ID**: - The `kubernetes-clusterid` flag is required if `kubernetes-type` is `eks`. However, there is no validation to ensure this dependency is met. Consider adding validation logic to check that `kubernetes-clusterid` is provided when `kubernetes-type` is `eks`. ```go if viper.GetString("kubernetes-type") == "eks" && viper.GetString("kubernetes-clusterid") == "" { log.Fatal().Msg("kubernetes-clusterid must be specified if kubernetes-type is eks") } ``` 3. **Security Considerations**: - Ensure that sensitive information such as tokens (`vcs-token`, `argocd-api-token`) are not logged or exposed in any way. Consider adding a check to mask these values in logs. ```go func maskSensitiveInfo(value string) string { if len(value) > 4 { return value[:2] + "****" + value[len(value)-2:] } return "****" } log.Info().Str("vcs-token", maskSensitiveInfo(viper.GetString("vcs-token"))).Msg("Using VCS token") ``` 4. **Performance Considerations**: - The `init` function is becoming quite large and complex. Consider breaking it down into smaller, more manageable functions to improve readability and maintainability. ```go func init() { setupViper() setupFlags() setupLogOutput() } func setupViper() { viper.SetEnvKeyReplacer(envKeyReplacer) viper.SetEnvPrefix(envPrefix) viper.AutomaticEnv() } func setupFlags() { flags := RootCmd.PersistentFlags() // Add all flag definitions here } ``` --- ## 😼 Mergecat review of pkg/container/main.go ```diff @@ -5,6 +5,7 @@ import ( "io/fs" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + client "github.com/zapier/kubechecks/pkg/kubernetes" "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/app_watcher" @@ -16,23 +17,30 @@ import ( ) type Container struct { - ApplicationWatcher *app_watcher.ApplicationWatcher - ArgoClient *argo_client.ArgoClient + ApplicationWatcher *app_watcher.ApplicationWatcher + ApplicationSetWatcher *app_watcher.ApplicationSetWatcher + ArgoClient *argo_client.ArgoClient Config config.ServerConfig RepoManager *git.RepoManager VcsClient vcs.Client VcsToArgoMap VcsToArgoMap + + KubeClientSet client.Interface } type VcsToArgoMap interface { AddApp(*v1alpha1.Application) + AddAppSet(*v1alpha1.ApplicationSet) UpdateApp(old, new *v1alpha1.Application) + UpdateAppSet(old *v1alpha1.ApplicationSet, new *v1alpha1.ApplicationSet) DeleteApp(*v1alpha1.Application) + DeleteAppSet(app *v1alpha1.ApplicationSet) GetVcsRepos() []string GetAppsInRepo(string) *appdir.AppDirectory + GetAppSetsInRepo(string) *appdir.AppSetDirectory GetMap() map[pkg.RepoURL]*appdir.AppDirectory WalkKustomizeApps(cloneURL string, fs fs.FS) *appdir.AppDirectory } ``` Feedback & Suggestions: 1. **Redundant Import**: The import statement for `client "github.com/zapier/kubechecks/pkg/kubernetes"` is redundant as it is already present in the original code. Remove the duplicate import to avoid confusion and maintain clean code. 2. **Alignment Consistency**: Ensure that the alignment of the struct fields is consistent. The original code had a consistent alignment which was altered in the diff. This helps in maintaining readability. Suggested changes: ```diff @@ -5,6 +5,7 @@ import ( "io/fs" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - client "github.com/zapier/kubechecks/pkg/kubernetes" "github.com/zapier/kubechecks/pkg" "github.com/zapier/kubechecks/pkg/app_watcher" @@ -16,23 +17,30 @@ import ( ) type Container struct { - ApplicationWatcher *app_watcher.ApplicationWatcher - ArgoClient *argo_client.ArgoClient + ApplicationWatcher *app_watcher.ApplicationWatcher + ApplicationSetWatcher *app_watcher.ApplicationSetWatcher + ArgoClient *argo_client.ArgoClient Config config.ServerConfig RepoManager *git.RepoManager VcsClient vcs.Client VcsToArgoMap VcsToArgoMap + + KubeClientSet client.Interface } type VcsToArgoMap interface { AddApp(*v1alpha1.Application) + AddAppSet(*v1alpha1.ApplicationSet) UpdateApp(old, new *v1alpha1.Application) + UpdateAppSet(old *v1alpha1.ApplicationSet, new *v1alpha1.ApplicationSet) DeleteApp(*v1alpha1.Application) + DeleteAppSet(app *v1alpha1.ApplicationSet) GetVcsRepos() []string GetAppsInRepo(string) *appdir.AppDirectory + GetAppSetsInRepo(string) *appdir.AppSetDirectory GetMap() map[pkg.RepoURL]*appdir.AppDirectory WalkKustomizeApps(cloneURL string, fs fs.FS) *appdir.AppDirectory } ``` --- ## 😼 Mergecat review of pkg/appdir/vcstoargomap.go ```diff @@ -5,26 +5,31 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/rs/zerolog/log" - "github.com/zapier/kubechecks/pkg" ) type VcsToArgoMap struct { - username string - appDirByRepo map[pkg.RepoURL]*AppDirectory + username string + appDirByRepo map[pkg.RepoURL]*AppDirectory + appSetDirByRepo map[pkg.RepoURL]*AppSetDirectory } func NewVcsToArgoMap(vcsUsername string) VcsToArgoMap { return VcsToArgoMap{ - username: vcsUsername, - appDirByRepo: make(map[pkg.RepoURL]*AppDirectory), + username: vcsUsername, + appDirByRepo: make(map[pkg.RepoURL]*AppDirectory), + appSetDirByRepo: make(map[pkg.RepoURL]*AppSetDirectory), } } func (v2a VcsToArgoMap) GetMap() map[pkg.RepoURL]*AppDirectory { return v2a.appDirByRepo } +func (v2a VcsToArgoMap) GetAppSetMap() map[pkg.RepoURL]*AppSetDirectory { + return v2a.appSetDirByRepo +} + func (v2a VcsToArgoMap) GetAppsInRepo(repoCloneUrl string) *AppDirectory { repoUrl, _, err := pkg.NormalizeRepoUrl(repoCloneUrl) if err != nil { @@ -40,6 +45,21 @@ func (v2a VcsToArgoMap) GetAppsInRepo(repoCloneUrl string) *AppDirectory { return appdir } +// GetAppSetsInRepo returns AppSetDirectory for the specified repository URL. +func (v2a VcsToArgoMap) GetAppSetsInRepo(repoCloneUrl string) *AppSetDirectory { + repoUrl, _, err := pkg.NormalizeRepoUrl(repoCloneUrl) + if err != nil { + log.Warn().Err(err).Msgf("failed to parse %s", repoCloneUrl) + } + appSetDir := v2a.appSetDirByRepo[repoUrl] + if appSetDir == nil { + appSetDir = NewAppSetDirectory() + v2a.appSetDirByRepo[repoUrl] = appSetDir + } + + return appSetDir +} + func (v2a VcsToArgoMap) WalkKustomizeApps(cloneURL string, fs fs.FS) *AppDirectory { var ( err error @@ -98,6 +118,41 @@ func (v2a VcsToArgoMap) GetVcsRepos() []string { for key := range v2a.appDirByRepo { repos = append(repos, key.CloneURL(v2a.username)) } - + for key := range v2a.appSetDirByRepo { + repos = append(repos, key.CloneURL(v2a.username)) + } return repos } + +func (v2a VcsToArgoMap) AddAppSet(app *v1alpha1.ApplicationSet) { + if app.Spec.Template.Spec.GetSource().RepoURL == "" { + log.Warn().Msgf("%s/%s: no source, skipping", app.Namespace, app.Name) + return + } + + appDirectory := v2a.GetAppSetsInRepo(app.Spec.Template.Spec.GetSource().RepoURL) + appDirectory.ProcessApp(*app) +} + +func (v2a VcsToArgoMap) UpdateAppSet(old *v1alpha1.ApplicationSet, new *v1alpha1.ApplicationSet) { + if new.Spec.Template.Spec.GetSource().RepoURL == "" { + log.Warn().Msgf("%s/%s: no source, skipping", new.Namespace, new.Name) + return + } + + oldAppDirectory := v2a.GetAppSetsInRepo(old.Spec.Template.Spec.GetSource().RepoURL) + oldAppDirectory.RemoveApp(*old) + + newAppDirectory := v2a.GetAppSetsInRepo(new.Spec.Template.Spec.GetSource().RepoURL) + newAppDirectory.ProcessApp(*new) +} + +func (v2a VcsToArgoMap) DeleteAppSet(app *v1alpha1.ApplicationSet) { + if app.Spec.Template.Spec.GetSource().RepoURL == "" { + log.Warn().Msgf("%s/%s: no source, skipping", app.Namespace, app.Name) + return + } + + oldAppDirectory := v2a.GetAppSetsInRepo(app.Spec.Template.Spec.GetSource().RepoURL) + oldAppDirectory.RemoveApp(*app) +} ``` Feedback & Suggestions: 1. **Error Handling**: In the `GetAppSetsInRepo` function, after logging the error, it would be better to return `nil` to avoid potential `nil` pointer dereference issues. ```go if err != nil { log.Warn().Err(err).Msgf("failed to parse %s", repoCloneUrl) return nil } ``` 2. **Consistency in Naming**: Ensure consistency in naming variables. For example, `repoCloneUrl` should be `repoCloneURL` to match Go naming conventions and the rest of the codebase. 3. **Code Duplication**: The code for handling `ApplicationSet` is very similar to the code for handling `Application`. Consider refactoring to reduce duplication. For example, you could create a generic function to handle both types. 4. **Documentation**: Add comments to the new functions (`GetAppSetMap`, `AddAppSet`, `UpdateAppSet`, `DeleteAppSet`) to maintain consistency and improve code readability. 5. **Performance**: When appending to the `repos` slice in `GetVcsRepos`, consider preallocating the slice with an estimated capacity to avoid multiple allocations. ```go repos := make([]string, 0, len(v2a.appDirByRepo) + len(v2a.appSetDirByRepo)) ``` 6. **Logging**: Ensure that all log messages provide enough context to be useful. For example, in `AddAppSet`, `UpdateAppSet`, and `DeleteAppSet`, include the `app.Name` and `app.Namespace` in the log messages for better traceability. --- ## 😼 Mergecat review of pkg/affected_apps/argocd_matcher.go ```diff @@ -5,14 +5,14 @@ import ( "os" "github.com/rs/zerolog/log" - "github.com/zapier/kubechecks/pkg/appdir" "github.com/zapier/kubechecks/pkg/container" "github.com/zapier/kubechecks/pkg/git" ) type ArgocdMatcher struct { - appsDirectory *appdir.AppDirectory + appsDirectory *appdir.AppDirectory + appSetsDirectory *appdir.AppSetDirectory } func NewArgocdMatcher(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) (*ArgocdMatcher, error) { @@ -23,8 +23,13 @@ func NewArgocdMatcher(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) (*Arg Union(repoApps). Union(kustomizeAppFiles) + repoAppSets := getArgocdAppSets(vcsToArgoMap, repo) + appSetDirectory := appdir.NewAppSetDirectory(). + Union(repoAppSets) + return &ArgocdMatcher{ - appsDirectory: appDirectory, + appsDirectory: appDirectory, + appSetsDirectory: appSetDirectory, }, nil } @@ -54,13 +59,31 @@ func getArgocdApps(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) *appdir. return repoApps } -func (a *ArgocdMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string) (AffectedItems, error) { +func getArgocdAppSets(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) *appdir.AppSetDirectory { + log.Debug().Msgf("looking for %s repos", repo.CloneURL) + repoApps := vcsToArgoMap.GetAppSetsInRepo(repo.CloneURL) + + if repoApps == nil { + log.Debug().Msg("found no appSets") + } else { + log.Debug().Msgf("found %d appSets", repoApps.Count()) + } + return repoApps +} + +func (a *ArgocdMatcher) AffectedApps(_ context.Context, changeList []string, targetBranch string, repo *git.Repo) (AffectedItems, error) { if a.appsDirectory == nil { return AffectedItems{}, nil } appsSlice := a.appsDirectory.FindAppsBasedOnChangeList(changeList, targetBranch) - return AffectedItems{Applications: appsSlice}, nil + appSetsSlice := a.appSetsDirectory.FindAppsBasedOnChangeList(changeList, targetBranch, repo) + + // and return both apps and appSets + return AffectedItems{ + Applications: appsSlice, + ApplicationSets: appSetsSlice, + }, nil } var _ Matcher = new(ArgocdMatcher) ``` Feedback & Suggestions: 1. **Redundant Code**: The `getArgocdAppSets` function is duplicated in the diff. This is unnecessary and should be removed to avoid redundancy. ```diff -func getArgocdAppSets(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) *appdir.AppSetDirectory { - log.Debug().Msgf("looking for %s repos", repo.CloneURL) - repoApps := vcsToArgoMap.GetAppSetsInRepo(repo.CloneURL) - - if repoApps == nil { - log.Debug().Msg("found no appSets") - } else { - log.Debug().Msgf("found %d appSets", repoApps.Count()) - } - return repoApps -} ``` 2. **Error Handling**: The `NewArgocdMatcher` function does not handle potential errors from `getArgocdApps`, `getKustomizeApps`, and `getArgocdAppSets`. It would be better to handle these errors to avoid potential runtime issues. ```go repoApps, err := getArgocdApps(vcsToArgoMap, repo) if err != nil { return nil, err } kustomizeAppFiles, err := getKustomizeApps(vcsToArgoMap, repo, repo.Directory) if err != nil { return nil, err } repoAppSets, err := getArgocdAppSets(vcsToArgoMap, repo) if err != nil { return nil, err } ``` 3. **Context Usage**: The `AffectedApps` function should use the `ctx` parameter instead of `_` to pass the context to `FindAppsBasedOnChangeList`. This ensures that any context-related cancellations or deadlines are respected. ```go appsSlice := a.appsDirectory.FindAppsBasedOnChangeList(ctx, changeList, targetBranch) appSetsSlice := a.appSetsDirectory.FindAppsBasedOnChangeList(ctx, changeList, targetBranch, repo) ``` 4. **Logging**: The logging messages could be more descriptive to provide better insights during debugging. ```go log.Debug().Msgf("looking for app sets in repo %s", repo.CloneURL) ``` By addressing these points, the code will be more robust, maintainable, and easier to debug. 🛠️✨ --- ## 😼 Mergecat review of pkg/events/check.go ```diff @@ -13,6 +13,8 @@ import ( "github.com/pkg/errors" "github.com/rs/zerolog" "github.com/rs/zerolog/log" + "github.com/zapier/kubechecks/pkg/generator" + "github.com/zapier/kubechecks/pkg/repo_config" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -22,7 +24,6 @@ import ( "github.com/zapier/kubechecks/pkg/container" "github.com/zapier/kubechecks/pkg/git" "github.com/zapier/kubechecks/pkg/msg" - "github.com/zapier/kubechecks/pkg/repo_config" "github.com/zapier/kubechecks/pkg/vcs" "github.com/zapier/kubechecks/telemetry" ) @@ -49,13 +50,34 @@ type CheckEvent struct { appsSent int32 appChannel chan *v1alpha1.Application wg sync.WaitGroup + generator generator.AppsGenerator + matcher affected_apps.Matcher } type repoManager interface { Clone(ctx context.Context, cloneURL, branchName string) (*git.Repo, error) } +func GenerateMatcher(ce *CheckEvent, repo *git.Repo) error { + log.Debug().Msg("using the argocd matcher") + m, err := affected_apps.NewArgocdMatcher(ce.ctr.VcsToArgoMap, repo) + if err != nil { + return errors.Wrap(err, "failed to create argocd matcher") + } + ce.matcher = m + cfg, err := repo_config.LoadRepoConfig(repo.Directory) + if err != nil { + return errors.Wrap(err, "failed to load repo config") + } else if cfg != nil { + log.Debug().Msg("using the config matcher") + configMatcher := affected_apps.NewConfigMatcher(cfg, ce.ctr) + ce.matcher = affected_apps.NewMultiMatcher(ce.matcher, configMatcher) + } + return nil +} + func NewCheckEvent(pullRequest vcs.PullRequest, ctr container.Container, repoManager repoManager, processors []checks.ProcessorEntry) *CheckEvent { + ce := &CheckEvent{ addedAppsSet: make(map[string]v1alpha1.Application), appChannel: make(chan *v1alpha1.Application, ctr.Config.MaxQueueSize), @@ -64,6 +86,7 @@ func NewCheckEvent(pullRequest vcs.PullRequest, ctr container.Container, repoMan processors: processors, pullRequest: pullRequest, repoManager: repoManager, + generator: generator.New(), logger: log.Logger.With(). Str("repo", pullRequest.Name). Int("event_id", pullRequest.CheckID). @@ -87,34 +110,35 @@ func (ce *CheckEvent) UpdateListOfChangedFiles(ctx context.Context, repo *git.Re return nil } +type MatcherFn func(ce *CheckEvent, repo *git.Repo) error + // GenerateListOfAffectedApps walks the repo to find any apps or appsets impacted by the changes in the MR/PR. -func (ce *CheckEvent) GenerateListOfAffectedApps(ctx context.Context, repo *git.Repo, targetBranch string) error { +func (ce *CheckEvent) GenerateListOfAffectedApps(ctx context.Context, repo *git.Repo, targetBranch string, initMatcherFn MatcherFn) error { _, span := tracer.Start(ctx, "GenerateListOfAffectedApps") defer span.End() var err error - var matcher affected_apps.Matcher - - log.Debug().Msg("using an argocd matcher") - matcher, err = affected_apps.NewArgocdMatcher(ce.ctr.VcsToArgoMap, repo) + err = initMatcherFn(ce, repo) if err != nil { return errors.Wrap(err, "failed to create argocd matcher") } - cfg, err := repo_config.LoadRepoConfig(repo.Directory) - if err != nil { - return errors.Wrap(err, "failed to load repo config") - } else if cfg != nil { - log.Debug().Msg("using the config matcher") - configMatcher := affected_apps.NewConfigMatcher(cfg, ce.ctr) - matcher = affected_apps.NewMultiMatcher(matcher, configMatcher) - } - - ce.affectedItems, err = matcher.AffectedApps(ctx, ce.fileList, targetBranch) + // use the changed file path to get the list of affected apps + // fileList is a list of changed files in the PR/MR, e.g. ["path/to/file1", "path/to/file2"] + ce.affectedItems, err = ce.matcher.AffectedApps(ctx, ce.fileList, targetBranch, repo) if err != nil { telemetry.SetError(span, err, "Get Affected Apps") ce.logger.Error().Err(err).Msg("could not get list of affected apps and appsets") } + for _, appSet := range ce.affectedItems.ApplicationSets { + apps, err := ce.generator.GenerateApplicationSetApps(ctx, appSet, &ce.ctr) + if err != nil { + ce.logger.Error().Err(err).Msg("could not generate apps from appSet") + continue + } + ce.affectedItems.Applications = append(ce.affectedItems.Applications, apps...) + } + span.SetAttributes( attribute.Int("numAffectedApps", len(ce.affectedItems.Applications)), attribute.Int("numAffectedAppSets", len(ce.affectedItems.ApplicationSets)), @@ -149,7 +173,7 @@ func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneU err error repo *git.Repo ) - + ce.logger.Info().Stack().Str("branchName", branchName).Msg("cloning repo") ce.repoLock.Lock() defer ce.repoLock.Unlock() @@ -209,7 +233,7 @@ func (ce *CheckEvent) Process(ctx context.Context) error { _, span := tracer.Start(ctx, "GenerateListOfAffectedApps") defer span.End() - // Clone the repo's BaseRef (main etc) locally into the temp dir we just made + // Clone the repo's BaseRef (main, etc.) locally into the temp dir we just made repo, err := ce.getRepo(ctx, ce.ctr.VcsClient, ce.pullRequest.CloneURL, ce.pullRequest.BaseRef) if err != nil { return errors.Wrap(err, "failed to clone repo") @@ -226,7 +250,7 @@ func (ce *CheckEvent) Process(ctx context.Context) error { } // Generate a list of affected apps, storing them within the CheckEvent (also returns but discarded here) - if err = ce.GenerateListOfAffectedApps(ctx, repo, ce.pullRequest.BaseRef); err != nil { + if err = ce.GenerateListOfAffectedApps(ctx, repo, ce.pullRequest.BaseRef, GenerateMatcher); err != nil { return errors.Wrap(err, "failed to generate a list of affected apps") } @@ -259,7 +283,7 @@ func (ce *CheckEvent) Process(ctx context.Context) error { } go w.run(ctx) } - + ce.logger.Info().Msgf("adding %d apps to the queue", len(ce.affectedItems.Applications)) // Produce apps onto channel for _, app := range ce.affectedItems.Applications { ce.queueApp(app) @@ -269,7 +293,7 @@ func (ce *CheckEvent) Process(ctx context.Context) error { close(ce.appChannel) - ce.logger.Debug().Msg("finished an app") + ce.logger.Debug().Msg("finished an app/appsets") ce.logger.Debug(). Int("all apps", len(ce.addedAppsSet)). ``` Feedback & Suggestions: 1. **Import Management**: The import reordering and addition of `generator` and `repo_config` are fine. However, ensure that the imports are grouped logically (standard library, third-party, and local imports) for better readability. 2. **Function Extraction**: The extraction of `GenerateMatcher` function is a good refactor for readability and separation of concerns. However, consider adding more detailed logging within this function to aid in debugging. 3. **Concurrency and Synchronization**: The addition of `generator` and `matcher` fields to `CheckEvent` struct is appropriate. Ensure that these fields are thread-safe if they are accessed concurrently. 4. **Error Handling**: The error handling in `GenerateMatcher` and `GenerateListOfAffectedApps` is good. However, consider adding more context to the error messages to make debugging easier. 5. **Logging**: The added logging statements are helpful. Ensure that sensitive information is not logged, especially in production environments. 6. **Code Comments**: The comments added to explain the purpose of `GenerateListOfAffectedApps` and other functions are helpful. Ensure that all public functions and methods have comments explaining their purpose and usage. 7. **Performance**: The changes do not introduce any obvious performance issues. However, ensure that the `GenerateMatcher` function is efficient, especially if it is called frequently. 8. **Security**: No new security vulnerabilities are introduced in the diff. Ensure that any external inputs (e.g., from `repo_config.LoadRepoConfig`) are properly sanitized and validated. Overall, the changes improve the readability and maintainability of the code. Good job! 🛠️ --- ## 😼 Mergecat review of go.mod ```diff @@ -7,6 +7,11 @@ toolchain go1.21.6 require ( github.com/argoproj/argo-cd/v2 v2.11.6 github.com/argoproj/gitops-engine v0.7.1-0.20240715141605-18ba62e1f1fb + github.com/aws/aws-sdk-go-v2 v1.30.1 + github.com/aws/aws-sdk-go-v2/config v1.27.24 + github.com/aws/aws-sdk-go-v2/service/eks v1.46.0 + github.com/aws/aws-sdk-go-v2/service/sts v1.30.1 + github.com/aws/smithy-go v1.20.3 github.com/cenkalti/backoff/v4 v4.3.0 github.com/chainguard-dev/git-urls v1.0.2 github.com/creasty/defaults v1.7.0 @@ -16,6 +21,8 @@ require ( github.com/go-logr/zerologr v1.2.3 github.com/google/go-github/v62 v62.0.0 github.com/heptiolabs/healthcheck v0.0.0-20211123025425-613501dd5deb + github.com/imdario/mergo v0.3.16 + github.com/jeremywohl/flatten v1.0.1 github.com/labstack/echo-contrib v0.17.1 github.com/labstack/echo/v4 v4.12.0 github.com/masterminds/semver v1.5.0 @@ -50,8 +57,12 @@ require ( google.golang.org/grpc v1.64.0 gopkg.in/dealancer/validate.v2 v2.1.0 gopkg.in/yaml.v3 v3.0.1 + k8s.io/api v0.26.15 + k8s.io/apiextensions-apiserver v0.26.10 k8s.io/apimachinery v0.26.15 k8s.io/client-go v0.26.15 + sigs.k8s.io/controller-runtime v0.14.7 + sigs.k8s.io/yaml v1.4.0 ) require ( @@ -66,8 +77,10 @@ require ( github.com/CycloneDX/cyclonedx-go v0.8.0 // indirect github.com/KeisukeYamashita/go-vcl v0.4.0 // indirect github.com/MakeNowJust/heredoc v1.0.0 // indirect + github.com/Masterminds/goutils v1.1.1 // indirect github.com/Masterminds/semver v1.5.0 // indirect github.com/Masterminds/semver/v3 v3.2.1 // indirect + github.com/Masterminds/sprig/v3 v3.2.3 // indirect github.com/Microsoft/go-winio v0.6.1 // indirect github.com/OneOfOne/xxhash v1.2.8 // indirect github.com/ProtonMail/go-crypto v0.0.0-20230828082145-3c4c8a2d2371 // indirect @@ -78,6 +91,15 @@ require ( github.com/apparentlymart/go-textseg/v13 v13.0.0 // indirect github.com/argoproj/pkg v0.13.7-0.20230627120311-a4dd357b057e // indirect github.com/aws/aws-sdk-go v1.50.8 // indirect + github.com/aws/aws-sdk-go-v2/credentials v1.17.24 // indirect + github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.9 // indirect + github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.13 // indirect + github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.13 // indirect + github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.15 // indirect + github.com/aws/aws-sdk-go-v2/service/sso v1.22.1 // indirect + github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.2 // indirect github.com/basgys/goxml2json v1.1.0 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/bgentry/go-netrc v0.0.0-20140422174119-9fd32a8b3d3d // indirect @@ -103,6 +125,7 @@ require ( github.com/emicklei/go-restful/v3 v3.10.2 // indirect github.com/emirpasic/gods v1.18.1 // indirect github.com/evanphx/json-patch v5.9.0+incompatible // indirect + github.com/evanphx/json-patch/v5 v5.6.0 // indirect github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d // indirect github.com/fatih/camelcase v1.0.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect @@ -140,6 +163,8 @@ require ( github.com/googleapis/enterprise-certificate-proxy v0.3.2 // indirect github.com/googleapis/gax-go/v2 v2.12.2 // indirect github.com/gorilla/mux v1.8.1 // indirect + github.com/gosimple/slug v1.13.1 // indirect + github.com/gosimple/unidecode v1.0.1 // indirect github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 // indirect github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect @@ -151,7 +176,7 @@ require ( github.com/hashicorp/go-version v1.6.0 // indirect github.com/hashicorp/hcl v1.0.0 // indirect github.com/hashicorp/hcl/v2 v2.17.0 // indirect - github.com/imdario/mergo v0.3.16 // indirect + github.com/huandu/xstrings v1.3.3 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/itchyny/gojq v0.12.13 // indirect github.com/itchyny/timefmt-go v0.1.5 // indirect @@ -175,9 +200,11 @@ require ( github.com/mattn/go-colorable v0.1.13 // indirect github.com/mattn/go-isatty v0.0.20 // indirect github.com/mattn/go-runewidth v0.0.15 // indirect + github.com/mitchellh/copystructure v1.0.0 // indirect github.com/mitchellh/go-homedir v1.1.0 // indirect github.com/mitchellh/go-testing-interface v1.14.1 // indirect github.com/mitchellh/go-wordwrap v1.0.1 // indirect + github.com/mitchellh/reflectwalk v1.0.0 // indirect github.com/moby/buildkit v0.12.5 // indirect github.com/moby/docker-image-spec v1.3.1 // indirect github.com/moby/spdystream v0.2.0 // indirect @@ -208,6 +235,7 @@ require ( github.com/sagikazarmark/slog-shim v0.1.0 // indirect github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 // indirect github.com/sergi/go-diff v1.3.1 // indirect + github.com/shopspring/decimal v1.2.0 // 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.2 // indirect @@ -260,8 +288,6 @@ require ( gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect - k8s.io/api v0.26.15 // indirect - k8s.io/apiextensions-apiserver v0.26.10 // indirect k8s.io/apiserver v0.26.15 // indirect k8s.io/cli-runtime v0.26.15 // indirect k8s.io/component-base v0.26.15 // indirect @@ -276,12 +302,10 @@ require ( muzzammil.xyz/jsonc v1.0.0 // indirect olympos.io/encoding/edn v0.0.0-20201019073823-d3554ca0b0a3 // indirect oras.land/oras-go/v2 v2.3.1 // indirect - sigs.k8s.io/controller-runtime v0.14.7 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/kustomize/api v0.12.1 // indirect sigs.k8s.io/kustomize/kyaml v0.13.9 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect - sigs.k8s.io/yaml v1.4.0 // indirect ) replace ( ``` Feedback & Suggestions: 1. **Duplicate Dependencies**: The diff reintroduces some dependencies that were already present in the original code. For example, `github.com/aws/aws-sdk-go-v2`, `github.com/imdario/mergo`, and `k8s.io/api`. Ensure that dependencies are not duplicated to avoid potential conflicts and maintain a clean `go.mod` file. 2. **Indirect Dependencies**: The diff adds several indirect dependencies directly under the `require` block. While this is not inherently wrong, it is generally better to let the Go module system manage indirect dependencies to avoid unnecessary clutter and potential version conflicts. 3. **Version Consistency**: Ensure that the versions of the dependencies are consistent and up-to-date. For instance, `k8s.io/apiextensions-apiserver` is listed with different versions (`v0.26.10` and `v0.26.15`). Consistency in versions helps in maintaining compatibility and stability. To improve the `go.mod` file, consider removing duplicates and letting the Go module system handle indirect dependencies where possible. --- ## 😼 Mergecat review of cmd/container.go ```diff @@ -12,6 +12,7 @@ import ( "github.com/zapier/kubechecks/pkg/config" "github://github.com/zapier/kubechecks/pkg/container" "github://github.com/zapier/kubechecks/pkg/git" + client "github://github.com/zapier/kubechecks/pkg/kubernetes" "github://github.com/zapier/kubechecks/pkg/vcs/github_client" "github://github.com/zapier/kubechecks/pkg/vcs/gitlab_client" ) @@ -36,7 +37,30 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool) if err != nil { return ctr, errors.Wrap(err, "failed to create vcs client") } + var kubeClient client.Interface + switch cfg.KubernetesType { + // TODO: expand with other cluster types + case client.ClusterTypeLOCAL: + kubeClient, err = client.New(&client.NewClientInput{ + KubernetesConfigPath: cfg.KubernetesConfig, + ClusterType: cfg.KubernetesType, + }) + if err != nil { + return ctr, errors.Wrap(err, "failed to create kube client") + } + case client.ClusterTypeEKS: + kubeClient, err = client.New(&client.NewClientInput{ + KubernetesConfigPath: cfg.KubernetesConfig, + ClusterType: cfg.KubernetesType, + }, + client.EKSClientOption(ctx, cfg.KubernetesClusterID), + ) + if err != nil { + return ctr, errors.Wrap(err, "failed to create kube client") + } + } + ctr.KubeClientSet = kubeClient // create argo client if ctr.ArgoClient, err = argo_client.NewArgoClient(cfg); err != nil { return ctr, errors.Wrap(err, "failed to create argo client") @@ -52,13 +76,22 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool) return ctr, errors.Wrap(err, "failed to build apps map") } + if err = buildAppSetsMap(ctx, ctr.ArgoClient, ctr.VcsToArgoMap); err != nil { + return ctr, errors.Wrap(err, "failed to build appsets map") + } + if watchApps { - ctr.ApplicationWatcher, err = app_watcher.NewApplicationWatcher(vcsToArgoMap, cfg) + ctr.ApplicationWatcher, err = app_watcher.NewApplicationWatcher(kubeClient.Config(), vcsToArgoMap, cfg) if err != nil { return ctr, errors.Wrap(err, "failed to create watch applications") } + ctr.ApplicationSetWatcher, err = app_watcher.NewApplicationSetWatcher(kubeClient.Config(), vcsToArgoMap, cfg) + if err != nil { + return ctr, errors.Wrap(err, "failed to create watch application sets") + } go ctr.ApplicationWatcher.Run(ctx, 1) + go ctr.ApplicationSetWatcher.Run(ctx) } } else { log.Info().Msgf("not monitoring applications, MonitorAllApplications: %+v", cfg.MonitorAllApplications) @@ -75,6 +108,16 @@ func buildAppsMap(ctx context.Context, argoClient *argo_client.ArgoClient, resul for _, app := range apps.Items { result.AddApp(&app) } + return nil +} +func buildAppSetsMap(ctx context.Context, argoClient *argo_client.ArgoClient, result container.VcsToArgoMap) error { + appSets, err := argoClient.GetApplicationSets(ctx) + if err != nil { + return errors.Wrap(err, "failed to list application sets") + } + for _, appSet := range appSets.Items { + result.AddAppSet(&appSet) + } return nil } ``` ### Feedback & Suggestions: 1. **Redundant Import Statement**: The import statement for `client "github.com/zapier/kubechecks/pkg/kubernetes"` is already present in the original code. The diff adds it again, which is unnecessary and can be removed. 2. **Error Handling Improvement**: The error handling for creating the Kubernetes client is repeated for each cluster type. This can be refactored to reduce redundancy and improve readability. 3. **Concurrency Considerations**: When starting the watchers with `go ctr.ApplicationWatcher.Run(ctx, 1)` and `go ctr.ApplicationSetWatcher.Run(ctx)`, consider adding proper error handling and synchronization mechanisms to ensure these goroutines are managed correctly. 4. **Logging Enhancements**: Add more detailed logging around the creation of clients and watchers to aid in debugging and monitoring. 5. **Security Considerations**: Ensure that sensitive information such as Kubernetes configuration paths and cluster IDs are not logged or exposed inadvertently. Here is a revised version of the diff with these suggestions: ```diff @@ -12,6 +12,7 @@ import ( "github.com/zapier/kubechecks/pkg/config" "github://github.com/zapier/kubechecks/pkg/container" "github://github.com/zapier/kubechecks/pkg/git" - client "github://github.com/zapier/kubechecks/pkg/kubernetes" "github://github.com/zapier/kubechecks/pkg/vcs/github_client" "github://github.com/zapier/kubechecks/pkg/vcs/gitlab_client" ) @@ -36,7 +37,30 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool) if err != nil { return ctr, errors.Wrap(err, "failed to create vcs client") } + var kubeClient client.Interface + kubeClient, err = createKubeClient(ctx, cfg) + if err != nil { + return ctr, errors.Wrap(err, "failed to create kube client") + } + ctr.KubeClientSet = kubeClient // create argo client if ctr.ArgoClient, err = argo_client.NewArgoClient(cfg); err != nil { return ctr, errors.Wrap(err, "failed to create argo client") @@ -52,13 +76,22 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool) return ctr, errors.Wrap(err, "failed to build apps map") } + if err = buildAppSetsMap(ctx, ctr.ArgoClient, ctr.VcsToArgoMap); err != nil { + return ctr, errors.Wrap(err, "failed to build appsets map") + } + if watchApps { - ctr.ApplicationWatcher, err = app_watcher.NewApplicationWatcher(vcsToArgoMap, cfg) + ctr.ApplicationWatcher, err = app_watcher.NewApplicationWatcher(kubeClient.Config(), vcsToArgoMap, cfg) if err != nil { return ctr, errors.Wrap(err, "failed to create watch applications") } + ctr.ApplicationSetWatcher, err = app_watcher.NewApplicationSetWatcher(kubeClient.Config(), vcsToArgoMap, cfg) + if err != nil { + return ctr, errors.Wrap(err, "failed to create watch application sets") + } go ctr.ApplicationWatcher.Run(ctx, 1) + go ctr.ApplicationSetWatcher.Run(ctx) } } else { log.Info().Msgf("not monitoring applications, MonitorAllApplications: %+v", cfg.MonitorAllApplications) @@ -75,6 +108,16 @@ func buildAppsMap(ctx context.Context, argoClient *argo_client.ArgoClient, resul for _, app := range apps.Items { result.AddApp(&app) } + return nil +} +func buildAppSetsMap(ctx context.Context, argoClient *argo_client.ArgoClient, result container.VcsToArgoMap) error { + appSets, err := argoClient.GetApplicationSets(ctx) + if err != nil { + return errors.Wrap(err, "failed to list application sets") + } + for _, appSet := range appSets.Items { + result.AddAppSet(&appSet) + } return nil } + +func createKubeClient(ctx context.Context, cfg config.ServerConfig) (client.Interface, error) { + var kubeClient client.Interface + var err error + switch cfg.KubernetesType { + // TODO: expand with other cluster types + case client.ClusterTypeLOCAL: + kubeClient, err = client.New(&client.NewClientInput{ + KubernetesConfigPath: cfg.KubernetesConfig, + ClusterType: cfg.KubernetesType, + }) + case client.ClusterTypeEKS: + kubeClient, err = client.New(&client.NewClientInput{ + KubernetesConfigPath: cfg.KubernetesConfig, + ClusterType: cfg.KubernetesType, + }, + client.EKSClientOption(ctx, cfg.KubernetesClusterID), + ) + } + return kubeClient, err +} ``` --- This revised diff addresses the redundancy, improves error handling, and adds a new function `createKubeClient` to encapsulate the Kubernetes client creation logic. This makes the code cleaner and easier to maintain. ## 😼 Mergecat review of pkg/events/check_test.go ```diff @@ -6,11 +6,22 @@ import ( "fmt" "testing" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/rs/zerolog" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - + affectedappsmocks "github.com/zapier/kubechecks/mocks/affected_apps/mocks" + generatorsmocks "github.com/zapier/kubechecks/mocks/generator/mocks" + "github.com/zapier/kubechecks/pkg/affected_apps" + "github.com/zapier/kubechecks/pkg/checks" "github.com/zapier/kubechecks/pkg/config" + "github.com/zapier/kubechecks/pkg/container" + "github.com/zapier/kubechecks/pkg/generator" "github.com/zapier/kubechecks/pkg/git" + "github.com/zapier/kubechecks/pkg/msg" + "github.com/zapier/kubechecks/pkg/vcs" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // TestCleanupGetManifestsError tests the cleanupGetManifestsError function. @@ -123,3 +134,182 @@ func TestCheckEventGetRepo(t *testing.T) { assert.Contains(t, ce.clonedRepos, generateRepoKey(canonical, "gh-pages")) }) } + +func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) { + type fields struct { + fileList []string + pullRequest vcs.PullRequest + logger zerolog.Logger + vcsNote *msg.Message + affectedItems affected_apps.AffectedItems + ctr container.Container + repoManager repoManager + processors []checks.ProcessorEntry + clonedRepos map[string]*git.Repo + addedAppsSet map[string]v1alpha1.Application + appsSent int32 + appChannel chan *v1alpha1.Application + generator generator.AppsGenerator + matcher affected_apps.Matcher + } + type args struct { + ctx context.Context + repo *git.Repo + targetBranch string + initMatcherFn MatcherFn + } + tests := []struct { + name string + fields fields + args args + expectedAppCount int + wantErr assert.ErrorAssertionFunc + }{ + // TODO: Add test cases. + { + name: "no error", + fields: fields{ + fileList: nil, + pullRequest: vcs.PullRequest{}, + logger: zerolog.Logger{}, + vcsNote: nil, + affectedItems: affected_apps.AffectedItems{}, + ctr: container.Container{}, + repoManager: nil, + processors: nil, + clonedRepos: nil, + addedAppsSet: nil, + appsSent: 0, + appChannel: nil, + generator: MockGenerator("GenerateApplicationSetApps", []interface{}{[]v1alpha1.Application{}, nil}), + matcher: MockMatcher("AffectedApps", []interface{}{ + affected_apps.AffectedItems{ + ApplicationSets: []v1alpha1.ApplicationSet{ + { + TypeMeta: metav1.TypeMeta{Kind: "ApplicationSet", APIVersion: "argoproj.io/v1alpha1"}, + ObjectMeta: metav1.ObjectMeta{Name: "appset1"}, + }, + }, + }, + nil, + }), + }, + args: args{ + ctx: context.Background(), + repo: &git.Repo{Directory: "/tmp"}, + targetBranch: "HEAD", + initMatcherFn: MockInitMatcherFn(), + }, + expectedAppCount: 1, + wantErr: assert.NoError, + }, + { + name: "matcher error", + fields: fields{ + fileList: nil, + pullRequest: vcs.PullRequest{}, + logger: zerolog.Logger{}, + vcsNote: nil, + affectedItems: affected_apps.AffectedItems{}, + ctr: container.Container{}, + repoManager: nil, + processors: nil, + clonedRepos: nil, + addedAppsSet: nil, + appsSent: 0, + appChannel: nil, + generator: MockGenerator("GenerateApplicationSetApps", []interface{}{[]v1alpha1.Application{}, nil}), + matcher: MockMatcher("AffectedApps", []interface{}{ + affected_apps.AffectedItems{}, + fmt.Errorf("mock error"), + }), + }, + args: args{ + ctx: context.Background(), + repo: &git.Repo{Directory: "/tmp"}, + targetBranch: "HEAD", + initMatcherFn: MockInitMatcherFn(), + }, + expectedAppCount: 0, + wantErr: assert.Error, + }, + { + name: "generator error", + fields: fields{ + fileList: nil, + pullRequest: vcs.PullRequest{}, + logger: zerolog.Logger{}, + vcsNote: nil, + affectedItems: affected_apps.AffectedItems{}, + ctr: container.Container{}, + repoManager: nil, + processors: nil, + clonedRepos: nil, + addedAppsSet: nil, + appsSent: 0, + appChannel: nil, + generator: MockGenerator("GenerateApplicationSetApps", []interface{}{[]v1alpha1.Application{}, fmt.Errorf("mock error")}), + matcher: MockMatcher("AffectedApps", []interface{}{ + affected_apps.AffectedItems{ + ApplicationSets: []v1alpha1.ApplicationSet{ + { + TypeMeta: metav1.TypeMeta{Kind: "ApplicationSet", APIVersion: "argoproj.io/v1alpha1"}, + ObjectMeta: metav1.ObjectMeta{Name: "appset1"}, + }, + }, + }, + nil, + }), + }, + args: args{ + ctx: context.Background(), + repo: &git.Repo{Directory: "/tmp"}, + targetBranch: "HEAD", + initMatcherFn: MockInitMatcherFn(), + }, + expectedAppCount: 0, + wantErr: assert.NoError, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ce := &CheckEvent{ + fileList: tt.fields.fileList, + pullRequest: tt.fields.pullRequest, + logger: tt.fields.logger, + vcsNote: tt.fields.vcsNote, + affectedItems: tt.fields.affectedItems, + ctr: tt.fields.ctr, + repoManager: tt.fields.repoManager, + processors: tt.fields.processors, + clonedRepos: tt.fields.clonedRepos, + addedAppsSet: tt.fields.addedAppsSet, + appsSent: tt.fields.appsSent, + appChannel: tt.fields.appChannel, + generator: tt.fields.generator, + matcher: tt.fields.matcher, + } + tt.wantErr(t, ce.GenerateListOfAffectedApps(tt.args.ctx, tt.args.repo, tt.args.targetBranch, tt.args.initMatcherFn), fmt.Sprintf("GenerateListOfAffectedApps(%v, %v, %v, %v)", tt.args.ctx, tt.args.repo, tt.args.targetBranch, tt.args.initMatcherFn)) + + }) + } +} + +func MockMatcher(methodName string, returns []interface{}) affected_apps.Matcher { + mockClient := new(affectedappsmocks.MockMatcher) + mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...) + + return mockClient +} + +func MockGenerator(methodName string, returns []interface{}) generator.AppsGenerator { + mockClient := new(generatorsmocks.MockAppsGenerator) + mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything).Return(returns...) + + return mockClient +} +func MockInitMatcherFn() MatcherFn { + return func(ce *CheckEvent, repo *git.Repo) error { + return nil + } +} ``` ### Feedback & Suggestions: 1. **Redundant Imports**: The diff reintroduces imports that are already present in the original code. This redundancy should be removed to keep the import section clean and maintainable. ```diff - "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - "github.com/rs/zerolog" - "github.com/stretchr/testify/mock" - affectedappsmocks "github.com/zapier/kubechecks/mocks/affected_apps/mocks" - generatorsmocks "github.com/zapier/kubechecks/mocks/generator/mocks" - "github.com/zapier/kubechecks/pkg/affected_apps" - "github.com/zapier/kubechecks/pkg/checks" - "github.com/zapier/kubechecks/pkg/container" - "github.com/zapier/kubechecks/pkg/generator" - "github.com/zapier/kubechecks/pkg/msg" - "github.com/zapier/kubechecks/pkg/vcs" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ``` 2. **Test Case Comments**: The `TODO: Add test cases` comment is still present. Consider adding meaningful test cases or removing the comment if it's not needed. ```diff - // TODO: Add test cases. ``` 3. **Mock Functions**: The mock functions `MockMatcher`, `MockGenerator`, and `MockInitMatcherFn` are duplicated in the diff. These should be defined once and reused to avoid redundancy. ```diff -func MockMatcher(methodName string, returns []interface{}) affected_apps.Matcher { - mockClient := new(affectedappsmocks.MockMatcher) - mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...) - - return mockClient -} - -func MockGenerator(methodName string, returns []interface{}) generator.AppsGenerator { - mockClient := new(generatorsmocks.MockAppsGenerator) - mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything).Return(returns...) - - return mockClient -} -func MockInitMatcherFn() MatcherFn { - return func(ce *CheckEvent, repo *git.Repo) error { - return nil - } -} ``` 4. **Error Handling**: Ensure that all error cases are properly handled and tested. The current test cases cover some scenarios, but additional edge cases might be beneficial. By addressing these points, the code will be cleaner, more maintainable, and less prone to errors. 🛠️ ---

---

Dependency Review

Click to read mergecats review!

No suggestions found