search

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

feature: Restrict Kubechecks to a Single Namespace for App Watcher #235

Closed Greyeye closed 2 weeks ago

Greyeye commented 2 weeks ago

Changes

  1. Namespace Restriction for Kubechecks:

    • Current Behavior: Kubechecks monitors all ArgoCD applications and ApplicationSet CRDs across the entire cluster. This is not ideal when multiple ArgoCD instances are running in a single cluster (e.g., for testing purposes).
    • New Behavior: Introduces the ability to specify a namespace for ArgoCD. Kubechecks will now watch ArgoCD CRDs only within the specified namespace.
      • Added a new flag argocd-api-namespace and a corresponding server config ArgoCDNameSpace.
      • Modified NewApplicationInformer to target the specified argocdnamespace.

  2. Improvement to dumpcrd:

    • dumpcrd can now run without requiring a local kubectl binary.

  3. Modification to GitHub Webhook Check:

    • The webhook check for GitHub has been updated to validate against the payload URL instead of the hook's API URL.

  4. OpenAI ChatGPT Model Update:

    • Updated to use the GPT-4o model.
  5. GitHub lib updated to v62

Localdev Changes

  1. Earthly File Renaming:

    • Renamed the local earthly file to earthly.sh to minimize confusion.

  2. TiltFile Fix:

    • Updated TiltFile to read the .tools-version file directly, preventing failures during the earthly build.
    • Updated to read .secret instead of .env; as secrets env vars are read from the .secret

  3. Values.yaml Update:

    • Renamed KUBECHECKS_ARGOCD_NAMESPACE to KUBECHECKS_ARGOCD_API_NAMESPACE in localdev/kubechecks/values.yaml.

  4. Configuration Change:

    • Set KUBECHECKS_ENABLE_CONFTEST to false as the ./policy path is not valid.
github-actions[bot] commented 2 weeks ago

Temporary image deleted.

zapier-sre-bot commented 2 weeks ago

Mergecat's Review

Click to read mergecats review! ## 😼 Mergecat review of .github/workflows/on_pull-request_docs.yaml ```diff - run: ./earthly +rebuild-docs + run: ./earthly.sh +rebuild-docs ``` Feedback & Suggestions: 1. **File Existence Check**: Ensure that `earthly.sh` exists and is executable. If this is a new script, it should be added to the repository and given the appropriate permissions. 2. **Consistency**: If `earthly.sh` is a wrapper or a renamed script, make sure all references in the repository are updated to avoid confusion. 3. **Documentation**: Update any relevant documentation to reflect the change from `earthly` to `earthly.sh` to help maintainers and contributors understand the change. --- ## 😼 Mergecat review of pkg/vcs/github_client/message.go ```diff @@ -5,7 +5,7 @@ import ( "fmt" "strings" - "github.com/google/go-github/v53/github" + "github.com/google/go-github/v62/github" "github.com/rs/zerolog/log" "github.com/shurcooL/githubv4" ``` Feedback & Suggestions: 1. **Compatibility Check**: Ensure that the new version `v62` of the `go-github` library is compatible with the rest of your code. Check the release notes for any breaking changes or deprecated features that might affect your implementation. 2. **Testing**: After updating the library version, run your test suite to verify that everything works as expected. Pay special attention to any tests related to GitHub API interactions. 3. **Dependency Management**: If you are using a dependency management tool like `go mod`, make sure to update your `go.mod` and `go.sum` files accordingly to reflect the new version of the library. --- ## 😼 Mergecat review of .github/workflows/on_pull_request_go.yaml ```diff - - run: ./earthly +ci-golang + - run: ./earthly.sh +ci-golang ``` Feedback & Suggestions: 1. **File Existence Check**: Ensure that `earthly.sh` exists and is executable. If this script is not present or not executable, the CI pipeline will fail. You might want to add a step to verify its presence or make it executable. ```yaml - name: Ensure earthly.sh is executable run: chmod +x ./earthly.sh ``` 2. **Consistency**: If `earthly.sh` is a new script, ensure that all references to `./earthly` in your repository are updated to `./earthly.sh` to maintain consistency. 3. **Documentation**: Update any documentation or comments within the repository to reflect the change from `./earthly` to `./earthly.sh` to avoid confusion for future maintainers. --- ## 😼 Mergecat review of justfile ```diff @@ -65,7 +65,7 @@ unit_test_race: go test -race ./... rebuild_docs: - ./earthly +rebuild-docs + ./earthly.sh +rebuild-docs ci-golang: - ./earthly +ci-golang + ./earthly.sh +ci-golang ``` Feedback & Suggestions: 1. **File Path Consistency**: Ensure that the script `earthly.sh` exists and is executable. If the script was previously named `earthly`, make sure to update all references to it across the project to avoid confusion. 2. **Permissions Check**: Verify that `earthly.sh` has the correct permissions to be executed. You can add a check in your script to ensure it is executable: ```python import os if not os.access('./earthly.sh', os.X_OK): print("Error: earthly.sh is not executable") exit(1) ``` 3. **Error Handling**: Consider adding error handling to check if the script execution fails. This can help in debugging issues related to script execution. --- ## 😼 Mergecat review of .gitignore ```diff @@ -26,3 +26,5 @@ localdev/terraform/gitlab/project.url *.DS_Store /kubechecks localdev/terraform/github/project.url +.secret +.arg ``` Feedback & Suggestions: 1. **Security**: Adding `.secret` and `.arg` to `.gitignore` is a good practice to prevent sensitive information from being committed. However, ensure that these files are not already committed to the repository. If they are, you should remove them from the history using tools like `git filter-branch` or `BFG Repo-Cleaner`. 2. **Consistency**: Consider adding comments to explain why `.secret` and `.arg` are being ignored. This can help other developers understand the purpose of these entries. ```diff +# Ignore secret and argument files +.secret +.arg ``` 3. **Specificity**: If `.secret` and `.arg` are specific to certain directories, it might be better to specify the paths to avoid unintentionally ignoring files with the same name in other parts of the project. ```diff +localdev/.secret +localdev/.arg ``` --- ## 😼 Mergecat review of .github/workflows/on_push_to_main.yaml ```diff @@ -43,7 +43,7 @@ jobs: - name: Build and push the helm charts run: | - ./earthly \ + ./earthly.sh \ --push \ +release-helm \ --repo_owner ${{ github.repository_owner }} \ ``` Feedback & Suggestions: 1. **File Existence Check**: Ensure that `earthly.sh` exists and is executable. You can add a step to verify this before running the script to avoid runtime errors. ```yaml - name: Verify earthly.sh exists and is executable run: | if [ ! -x ./earthly.sh ]; then echo "earthly.sh not found or not executable" exit 1 fi ``` 2. **Security Consideration**: If `earthly.sh` is a script that can be modified, ensure it is sourced from a trusted location or repository to avoid security risks. 3. **Consistency**: If there are other places in the workflow where `./earthly` is used, ensure they are also updated to `./earthly.sh` for consistency. --- ## 😼 Mergecat review of .github/workflows/on_pull-request_helm.yaml ```diff @@ -17,4 +17,4 @@ jobs: - uses: earthly/actions-setup@v1 with: { version: "v${{ env.EARTHLY_TOOL_VERSION }}" } - - run: ./earthly +ci-helm + - run: ./earthly.sh +ci-helm ``` Feedback & Suggestions: 1. **File Existence Check**: Ensure that `earthly.sh` exists and is executable. If this script is not present or not executable, the CI pipeline will fail. You might want to add a step to verify its presence or make it executable. ```yaml - name: Ensure earthly.sh is executable run: chmod +x ./earthly.sh ``` 2. **Error Handling**: Consider adding error handling to provide more context if the script fails. This can be done by capturing the exit code and printing a custom error message. ```yaml - name: Run earthly.sh run: | ./earthly.sh +ci-helm || { echo 'earthly.sh failed'; exit 1; } ``` 3. **Consistency**: Ensure that the change from `./earthly` to `./earthly.sh` is consistent with the rest of your project and documentation. If other parts of your project still reference `./earthly`, it could lead to confusion or errors. --- ## 😼 Mergecat review of .github/actions/build-image/action.yaml ```diff @@ -41,7 +41,7 @@ runs: - name: Build and push the Docker image shell: bash run: >- - ./earthly + ./earthly.sh --push +docker-multiarch ${{ inputs.tag_latest != 'false' && format('--LATEST_IMAGE_NAME=ghcr.io/{0}:latest', github.repository) || '' }} ``` Feedback & Suggestions: 1. **File Existence Check**: Ensure that `earthly.sh` exists and is executable. If it doesn't exist or isn't executable, the script will fail. Consider adding a check before running the script. ```bash if [ ! -x ./earthly.sh ]; then echo "Error: earthly.sh not found or not executable" exit 1 fi ./earthly.sh --push +docker-multiarch ... ``` 2. **Security**: Ensure that `earthly.sh` is a trusted script. Running shell scripts can be risky if they are not from a trusted source. Make sure the script is reviewed and safe to execute. 3. **Consistency**: If `earthly.sh` is a new script replacing `earthly`, ensure that all references to `earthly` in the repository are updated to `earthly.sh` to avoid confusion. 4. **Documentation**: Update any documentation or comments to reflect the change from `earthly` to `earthly.sh` to maintain clarity for future maintainers. --- ## 😼 Mergecat review of docs/usage.md ```diff @@ -37,6 +37,7 @@ The full list of supported environment variables is described below: |Env Var|Description|Default Value| |-----------|-------------|------| |`KUBECHECKS_ARGOCD_API_INSECURE`|Enable to use insecure connections to the ArgoCD API server.|`false`| +|`KUBECHECKS_ARGOCD_API_NAMESPACE`|ArgoCD namespace where the application watcher will read Custom Resource Definitions (CRD) for Application and ApplicationSet resources.|`argocd`| |`KUBECHECKS_ARGOCD_API_SERVER_ADDR`|ArgoCD API Server Address.|`argocd-server`| |`KUBECHECKS_ARGOCD_API_TOKEN`|ArgoCD API token.|| |`KUBECHECKS_ENABLE_CONFTEST`|Set to true to enable conftest policy checking of manifests.|`false`| ``` ### Feedback & Suggestions: 1. **Typo in Description**: The description for `KUBECHECKS_ARGOCD_API_NAMESPACE` should be clear and concise. Consider rephrasing it to: "Namespace where the application watcher reads CRDs for Application and ApplicationSet resources." 2. **Security Note**: Since this environment variable deals with namespaces, it might be useful to add a note about ensuring the namespace is correctly set to avoid potential security issues. 3. **Consistency**: Ensure that the new entry follows the same style and format as the existing entries. For example, the description should be succinct and to the point. 4. **Documentation Update**: If this new environment variable requires additional setup or has implications on the deployment, ensure that these details are documented elsewhere in the usage guide. --- ## 😼 Mergecat review of pkg/aisummary/diff_summary.go ```diff @@ -17,7 +17,7 @@ func (c *OpenAiClient) SummarizeDiff(ctx context.Context, appName, diff string) ctx, span := tracer.Start(ctx, "SummarizeDiff") defer span.End() - model := openai.GPT4Turbo0125 + model := openai.GPT4o if len(diff) < 3500 { model = openai.GPT3Dot5Turbo } ``` Feedback & Suggestions: 1. **Model Name Consistency**: The change from `openai.GPT4Turbo0125` to `openai.GPT4o` might be a typo or an incorrect model name. Ensure that `GPT4o` is the correct identifier for the model you intend to use. If it is a new model, verify its availability and correct naming in the `go-openai` library. 2. **Error Handling**: If `GPT4o` is not a valid model, it could lead to runtime errors. Consider adding a validation step to ensure the model name is correct before making the request. Example: ```go validModels := map[string]bool{ openai.GPT4Turbo0125: true, openai.GPT4o: true, openai.GPT3Dot5Turbo: true, } if !validModels[model] { err := fmt.Errorf("invalid model name: %s", model) telemetry.SetError(span, err, "Invalid model name") return "", err } ``` --- ## 😼 Mergecat review of cmd/container.go ```diff @@ -5,7 +5,7 @@ import ( "fmt" "github.com/pkg/errors" - + "github.com/rs/zerolog/log" "github.com/zapier/kubechecks/pkg/app_watcher" "github.com/zapier/kubechecks/pkg/appdir" "github.com/zapier/kubechecks/pkg/argo_client" @@ -60,6 +60,8 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool) go ctr.ApplicationWatcher.Run(ctx, 1) } + } else { + log.Info().Msgf("not monitoring applications, MonitorAllApplications: %+v", cfg.MonitorAllApplications) } return ctr, nil ``` ### Feedback & Suggestions: 1. **Logging Library Addition**: The addition of the `zerolog` logging library is a good choice for structured logging. However, ensure that this library is consistently used throughout the project for uniformity. 2. **Log Message Clarity**: The log message `"not monitoring applications, MonitorAllApplications: %+v"` is clear, but consider providing more context to make it even more informative. For example: ```go log.Info().Msgf("Application monitoring is disabled. MonitorAllApplications: %+v", cfg.MonitorAllApplications) ``` 3. **Error Handling**: The current diff does not introduce new error handling, but it is a good practice to ensure that all potential errors are logged. Consider adding a log entry before returning errors to provide more context in the logs. 4. **Performance Consideration**: The added logging statement is inside a conditional block that is not frequently executed, so it should not introduce significant performance overhead. However, always be mindful of the performance impact of logging, especially in high-frequency code paths. 5. **Security Consideration**: Ensure that the configuration data being logged does not contain sensitive information. If `cfg.MonitorAllApplications` or any other part of the configuration might contain sensitive data, consider sanitizing it before logging. --- Overall, the diff improves the code by adding informative logging, but always ensure consistency and security in logging practices. ## 😼 Mergecat review of cmd/root.go ```diff @@ -56,8 +56,13 @@ func init() { withDefault("gitlab")) stringFlag(flags, "vcs-token", "VCS API token.") stringFlag(flags, "argocd-api-token", "ArgoCD API token.") - stringFlag(flags, "argocd-api-server-addr", "ArgoCD API Server Address.", newStringOpts().withDefault("argocd-server")) + stringFlag(flags, "argocd-api-server-addr", "ArgoCD API Server Address.", + newStringOpts(). + withDefault("argocd-server")) boolFlag(flags, "argocd-api-insecure", "Enable to use insecure connections to the ArgoCD API server.") + 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-config", "Path to your kubernetes config file, used to monitor applications.") stringFlag(flags, "otel-collector-port", "The OpenTelemetry collector port.") ``` ### Feedback & Suggestions: 1. **Consistency in Formatting**: The new addition of `argocd-api-namespace` follows the same formatting style as the modified `argocd-api-server-addr`, which is good for consistency. However, ensure that all similar flag definitions follow this style for uniformity. 2. **Security Consideration**: The `argocd-api-namespace` flag might expose sensitive namespace information. Ensure that this information is handled securely and consider adding validation to check for potentially harmful inputs. 3. **Documentation**: Make sure to update any relevant documentation to include the new `argocd-api-namespace` flag. This will help users understand the new configuration option. 4. **Error Handling**: Consider adding error handling for the new flag to ensure that invalid values are caught early. This can prevent potential runtime issues. 5. **Testing**: Ensure that there are tests in place to verify the behavior of the new `argocd-api-namespace` flag. This will help maintain the reliability of the application. Overall, the changes are well-integrated and maintain the existing code style. Just ensure that the new flag is documented, validated, and tested properly. 🚀 --- ## 😼 Mergecat review of pkg/app_watcher/app_watcher_test.go ```diff @@ -8,12 +8,18 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" appclientsetfake "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zapier/kubechecks/pkg/config" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/zapier/kubechecks/pkg/appdir" ) -func initTestObjects() *ApplicationWatcher { +func initTestObjects(t *testing.T) *ApplicationWatcher { + cfg, err := config.New() + // Handle the error appropriately, e.g., log it or fail the test + require.NoError(t, err, "failed to create config") + // set up the fake Application client set and informer. testApp1 := &v1alpha1.Application{ ObjectMeta: metav1.ObjectMeta{Name: "test-app-1", Namespace: "default"}, @@ -34,15 +40,15 @@ func initTestObjects() *ApplicationWatcher { vcsToArgoMap: appdir.NewVcsToArgoMap("vcs-username"), } - appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second * 1) + appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*1, cfg) ctrl.appInformer = appInformer ctrl.appLister = appLister return ctrl } func TestApplicationAdded(t *testing.T) { - appWatcher := initTestObjects() + appWatcher := initTestObjects(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -68,7 +74,7 @@ func TestApplicationAdded(t *testing.T) { } func TestApplicationUpdated(t *testing.T) { - ctrl := initTestObjects() + ctrl := initTestObjects(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -101,7 +107,7 @@ func TestApplicationUpdated(t *testing.T) { } func TestApplicationDeleted(t *testing.T) { - ctrl := initTestObjects() + ctrl := initTestObjects(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() ``` Feedback & Suggestions: 1. **Error Handling**: The addition of `require.NoError(t, err, "failed to create config")` is a good step for error handling. However, ensure that the error message is descriptive enough to help in debugging. 2. **Configuration Dependency**: The `config.New()` call introduces a dependency on the configuration. Ensure that this configuration is mockable or configurable for different test environments to avoid flaky tests. 3. **Formatting**: The spacing around `time.Second*1` is inconsistent. It should be `time.Second * 1` for better readability. 4. **Test Initialization**: Passing `t *testing.T` to `initTestObjects` is a good practice as it allows for better error handling within the initialization function. Overall, the changes improve error handling and make the tests more robust. 🛠️ --- ## 😼 Mergecat review of localdev/kubechecks/values.yaml ```diff @@ -5,7 +5,7 @@ configMap: KUBECHECKS_ENABLE_WEBHOOK_CONTROLLER: "false" KUBECHECKS_ARGOCD_API_INSECURE: "true" KUBECHECKS_ARGOCD_API_PATH_PREFIX : '/argocd' - KUBECHECKS_ARGOCD_NAMESPACE: 'kubechecks' + KUBECHECKS_ARGOCD_API_NAMESPACE: 'kubechecks' KUBECHECKS_WEBHOOK_URL_PREFIX: 'kubechecks' KUBECHECKS_NAMESPACE: 'kubechecks' KUBECHECKS_FALLBACK_K8S_VERSION: "1.25.0" @@ -20,7 +20,7 @@ configMap: # KUBECHECKS_LABEL_FILTER: "test" # On your PR/MR, prefix this with "kubechecks:" # KUBECHECKS_SCHEMAS_LOCATION: https://github.com/zapier/kubecheck-schemas.git KUBECHECKS_TIDY_OUTDATED_COMMENTS_MODE: "delete" - KUBECHECKS_ENABLE_CONFTEST: "true" + KUBECHECKS_ENABLE_CONFTEST: "false" ``` ### Feedback & Suggestions: 1. **Consistency in Naming Conventions**: - The change from `KUBECHECKS_ARGOCD_NAMESPACE` to `KUBECHECKS_ARGOCD_API_NAMESPACE` should be reflected consistently across the entire codebase to avoid confusion and potential bugs. Ensure that all references to this variable are updated accordingly. 2. **Configuration Change Justification**: - Changing `KUBECHECKS_ENABLE_CONFTEST` from `"true"` to `"false"` might have significant implications. Ensure that this change is well-documented and justified. If this is for a temporary purpose (e.g., debugging), consider adding a comment to explain the reason. 3. **Security Considerations**: - While not directly related to the diff, it's important to note that sensitive information such as `KUBECHECKS_ARGOCD_API_TOKEN` should not be hardcoded in the configuration file. Consider using environment variables or secret management tools to handle such sensitive data securely. 4. **Testing**: - Ensure that these changes are thoroughly tested in a staging environment before deploying to production. Specifically, verify that the new namespace variable and the disabled conftest functionality do not introduce any regressions. --- ## 😼 Mergecat review of pkg/app_watcher/app_watcher.go ```diff @@ -44,7 +44,7 @@ func NewApplicationWatcher(vcsToArgoMap appdir.VcsToArgoMap, cfg config.ServerCo vcsToArgoMap: vcsToArgoMap, } - appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second * 30) + appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, cfg) ctrl.appInformer = appInformer ctrl.appLister = appLister @@ -118,12 +118,19 @@ func (ctrl *ApplicationWatcher) onApplicationDeleted(obj interface{}) { } /* -This Go function, named newApplicationInformerAndLister, is part of the ApplicationWatcher struct. It sets up a Kubernetes SharedIndexInformer and a Lister for Argo CD Applications. -A SharedIndexInformer is used to watch changes to a specific type of Kubernetes resource in an efficient manner. It significantly reduces the load on the Kubernetes API server by sharing and caching watches between all controllers that need to observe the object. -Listers use the data from the informer's cache to provide a read-optimized view of the cache which reduces the load on the API Server and hides some complexity. +newApplicationInformerAndLister, is part of the ApplicationWatcher struct. It sets up a Kubernetes SharedIndexInformer +and a Lister for Argo CD Applications. + +A SharedIndexInformer is used to watch changes to a specific type of Kubernetes resource in an efficient manner. +It significantly reduces the load on the Kubernetes API server by sharing and caching watches between all controllers +that need to observe the object. + +newApplicationInformerAndLister use the data from the informer's cache to provide a read-optimized view of the cache which reduces +the load on the API Server and hides some complexity. */ -func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout time.Duration) (cache.SharedIndexInformer, applisters.ApplicationLister) { - informer := informers.NewApplicationInformer(ctrl.applicationClientset, "", refreshTimeout, +func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig) (cache.SharedIndexInformer, applisters.ApplicationLister) { + log.Debug().Msgf("Creating Application informer with namespace: %s", cfg.ArgoCDNamespace) + informer := informers.NewApplicationInformer(ctrl.applicationClientset, cfg.ArgoCDNamespace, refreshTimeout, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, ) ``` Feedback & Suggestions: 1. **Namespace Logging**: The addition of logging the namespace is a good idea for debugging purposes. However, ensure that the logging level is appropriate for the information being logged. In this case, `Debug` is suitable. 2. **Documentation Update**: The changes to the comments make them more readable, but the documentation should be consistent. Ensure that the comment style and formatting are uniform throughout the codebase. 3. **Function Signature Change**: The addition of `cfg config.ServerConfig` to the `newApplicationInformerAndLister` function signature is a good change for passing configuration. However, ensure that this change is reflected in all places where this function is called to avoid breaking changes. 4. **Performance Consideration**: The use of `time.Second*30` is fine, but consider making this a configurable parameter if it isn't already. This would allow for more flexibility and tuning based on different environments. 5. **Error Handling**: Ensure that any potential errors from the `NewApplicationInformer` function are properly handled. Although this is not part of the diff, it's a good practice to check for errors immediately after calling functions that can return them. --- ## 😼 Mergecat review of docs/contributing.md ```diff @@ -67,10 +67,18 @@ It creates: To get started do the following: -* Copy the `.env.example` and set required values. +* Copy the `.secret.example` and set required values. ```console - cp .env.example .env + cp .secret.example .secret + ``` + You will need to fill in either `GITLAB_TOKEN` or `GITLAB_TOKEN` + If you are testing with GITHUB, please set the tile_config.json file to specify the vcs-type as the default is `gitlab`. + The token you specify must have ability to get repositories, add/delete comment and webhooks. + ```json + { + "vcs-type": "github" + } ``` * From the root directory of this repo: @@ -110,7 +118,7 @@ If you're using minikube with Tilt we recommend following this [guide](https://g ### Code Changes -We use Earthly to simplify our CI/CD process with `kubechecks`. There's a thin wrapper around earthly that passes some common arguments in the root of the repo called `./earthly` that should be used instead of calling earthly directly. This also simplifies testing changes locally before pushing them up to ensure your PR will pass all required checks. The best command to run is `./earthly +test` this will pull all the required dependencies (including any new ones that you have added). It will then run [go vet](https://pkg.go.dev/cmd/vet), and if those pass it will run `go test` with race detection enabled. You can also always run these commands directly `go test -race ./...` will run all tests in the repo with race detection enabled. Please ensure that `./earthly +test` is passing before opening a PR. +We use Earthly to simplify our CI/CD process with `kubechecks`. There's a thin wrapper around earthly that passes some common arguments in the root of the repo called `./earthly.sh` that should be used instead of calling earthly directly. This also simplifies testing changes locally before pushing them up to ensure your PR will pass all required checks. The best command to run is `./earthly.sh +test` this will pull all the required dependencies (including any new ones that you have added). It will then run [go vet](https://pkg.go.dev/cmd/vet), and if those pass it will run `go test` with race detection enabled. You can also always run these commands directly `go test -race ./...` will run all tests in the repo with race detection enabled. Please ensure that `./earthly.sh +test` is passing before opening a PR. ### Documentation Changes ``` Feedback & Suggestions: 1. **Typo Correction**: The line "You will need to fill in either `GITLAB_TOKEN` or `GITLAB_TOKEN`" should be corrected to specify the correct tokens. It seems like a copy-paste error. ```diff - You will need to fill in either `GITLAB_TOKEN` or `GITLAB_TOKEN` + You will need to fill in either `GITLAB_TOKEN` or `GITHUB_TOKEN` ``` 2. **Clarification on `tile_config.json`**: The instruction to set the `tile_config.json` file should be more explicit about where this file is located or how to create it if it doesn't exist. ```diff + If you are testing with GITHUB, please set the `tile_config.json` file in the root directory to specify the vcs-type as the default is `gitlab`. ``` 3. **Token Permissions**: The description of the token permissions should be formatted for better readability. ```diff - The token you specify must have ability to get repositories, add/delete comment and webhooks. + The token you specify must have the ability to: + - Get repositories + - Add/delete comments + - Manage webhooks ``` 4. **Consistency in File Naming**: Ensure consistency in file naming conventions. The change from `./earthly` to `./earthly.sh` should be reflected consistently throughout the document. ```diff - The best command to run is `./earthly +test` + The best command to run is `./earthly.sh +test` ``` By addressing these points, the documentation will be clearer and more user-friendly. 📝✨ --- ## 😼 Mergecat review of pkg/config/config.go ```diff @@ -21,6 +21,7 @@ type ServerConfig struct { 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"` // otel @@ -29,9 +30,10 @@ type ServerConfig struct { OtelCollectorPort string `mapstructure:"otel-collector-port"` // vcs - VcsBaseUrl string `mapstructure:"vcs-base-url"` - VcsToken string `mapstructure:"vcs-token"` - VcsType string `mapstructure:"vcs-type"` + VcsBaseUrl string `mapstructure:"vcs-base-url"` + VcsUploadUrl string `mapstructure:"vcs-upload-url"` // github enterprise upload URL + VcsToken string `mapstructure:"vcs-token"` + VcsType string `mapstructure:"vcs-type"` // webhooks EnsureWebhooks bool `mapstructure:"ensure-webhooks"` @@ -101,6 +103,7 @@ func NewWithViper(v *viper.Viper) (ServerConfig, error) { log.Info().Msgf("Webhook URL Base: %s", cfg.WebhookUrlBase) log.Info().Msgf("Webhook URL Prefix: %s", cfg.UrlPrefix) log.Info().Msgf("VCS Type: %s", cfg.VcsType) + log.Info().Msgf("ArgoCD Namespace: %s", cfg.ArgoCDNamespace) return cfg, nil } ``` Feedback & Suggestions: 1. **Security**: Ensure that sensitive information such as tokens and URLs are not logged. Logging `cfg.ArgoCDNamespace` and other sensitive fields can expose critical information. Consider using `log.Debug()` for such details or omitting them entirely from logs. ```go log.Debug().Msgf("ArgoCD Namespace: %s", cfg.ArgoCDNamespace) ``` 2. **Consistency**: The comment for `VcsUploadUrl` is specific to GitHub Enterprise. If this configuration is intended to be generic, consider making the comment more general or adding similar comments for other fields to maintain consistency. ```go VcsUploadUrl string `mapstructure:"vcs-upload-url"` // upload URL for VCS ``` 3. **Validation**: Adding new fields like `ArgoCDNamespace` and `VcsUploadUrl` should be accompanied by validation checks to ensure they are correctly set. Consider adding validation logic in the `NewWithViper` function or a separate validation function. ```go if cfg.ArgoCDNamespace == "" { return cfg, errors.New("ArgoCDNamespace cannot be empty") } if cfg.VcsUploadUrl == "" { return cfg, errors.New("VcsUploadUrl cannot be empty") } ``` 4. **Documentation**: Ensure that the new fields are documented in any relevant configuration documentation to inform users of their purpose and usage. --- ## 😼 Mergecat review of Tiltfile ```diff @@ -7,7 +7,12 @@ load('ext://uibutton', 'cmd_button') load('ext://helm_resource', 'helm_resource') load('./.tilt/terraform/Tiltfile', 'local_terraform_resource') load('./.tilt/utils/Tiltfile', 'check_env_set') -dotenv() + +# Check if the .secret file exists +if not os.path.exists('.secret'): + fail('The .secret file is missing. Please copy .secret file from .secret.example and setup before running Tilt.') + +dotenv(fn='.secret') config.define_bool("enable_repo", True, 'create a new project for testing this app') config.define_string("vcs-type") @@ -126,7 +131,7 @@ if cfg.get('enable_repo', True): test_go( 'go-test', '.', recursive=True, - timeout='30s', + timeout='60s', extra_args=['-v'], labels=["kubechecks"], deps=[ @@ -138,12 +143,55 @@ 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): + if not os.path.exists(fn): + warn("tool versions file not found: '%s'" % fn) + return dict() + + f = read_file(fn) + + lines = str(f).splitlines() + + tools = dict() + + for linenumber in range(len(lines)): + line = lines[linenumber] + parts = line.split("#", 1) + if len(parts) == 2: + line = parts[0] + line = line.strip() + if line == "": + continue + parts = line.split(' ', 1) + tools[parts[0].strip()] = parts[1].strip() + return tools + +tool_versions = parse_tool_versions(".tool-versions") +git_commit = str(get_git_head()).strip() + earthly_build( context='.', target="+docker-debug", ref='kubechecks', image_arg='IMAGE_NAME', ignore='./dist', + extra_args=[ + '--CHART_RELEASER_VERSION='+tool_versions.get('helm-cr'), + '--GOLANG_VERSION='+tool_versions.get('golang'), + '--GOLANGCI_LINT_VERSION='+tool_versions.get('golangci-lint'), + '--HELM_VERSION='+tool_versions.get('helm'), + '--KUBECONFORM_VERSION='+tool_versions.get('kubeconform'), + '--KUSTOMIZE_VERSION='+tool_versions.get('kustomize'), + '--STATICCHECK_VERSION='+tool_versions.get('staticcheck'), + '--GIT_COMMIT='+git_commit, + ], ) cmd_button('loc:go mod tidy', @@ -216,4 +264,4 @@ load("localdev/test_appsets/Tiltfile", "install_test_appsets") install_test_appsets(cfg) -force_argocd_cleanup_on_tilt_down() \ No newline at end of file +force_argocd_cleanup_on_tilt_down() ``` Feedback & Suggestions: 1. **Security Check for `.secret` File**: - The check for the `.secret` file is a good addition. However, consider using `os.path.isfile` instead of `os.path.exists` to ensure it's a file and not a directory. - **Suggestion**: ```python if not os.path.isfile('.secret'): fail('The .secret file is missing. Please copy .secret file from .secret.example and setup before running Tilt.') ``` 2. **Timeout Increase for `test_go`**: - Increasing the timeout from `30s` to `60s` might be necessary, but ensure this change is well-justified and documented, as it could affect the overall test runtime. 3. **Function `get_git_head`**: - The function `get_git_head` uses `local` which is not defined in the provided code. Ensure that `local` is imported or defined. - **Suggestion**: ```python from subprocess import check_output def get_git_head(): result = check_output(['git', 'rev-parse', '--short', 'HEAD']).strip().decode('utf-8') return result ``` 4. **Function `parse_tool_versions`**: - The function `parse_tool_versions` is well-implemented but can be optimized for readability. - **Suggestion**: ```python def parse_tool_versions(fn): if not os.path.isfile(fn): warn(f"tool versions file not found: '{fn}'") return {} with open(fn, 'r') as f: lines = f.readlines() tools = {} for line in lines: line = line.split("#", 1)[0].strip() # Remove comments and strip whitespace if line: tool, version = line.split(' ', 1) tools[tool.strip()] = version.strip() return tools ``` 5. **Extra Arguments in `earthly_build`**: - Ensure that all the versions retrieved from `tool_versions` are not `None` before using them in `extra_args`. - **Suggestion**: ```python extra_args = [ f'--CHART_RELEASER_VERSION={tool_versions.get("helm-cr", "")}', f'--GOLANG_VERSION={tool_versions.get("golang", "")}', f'--GOLANGCI_LINT_VERSION={tool_versions.get("golangci-lint", "")}', f'--HELM_VERSION={tool_versions.get("helm", "")}', f'--KUBECONFORM_VERSION={tool_versions.get("kubeconform", "")}', f'--KUSTOMIZE_VERSION={tool_versions.get("kustomize", "")}', f'--STATICCHECK_VERSION={tool_versions.get("staticcheck", "")}', f'--GIT_COMMIT={git_commit}', ] ``` 6. **Newline at End of File**: - Ensure there is a newline at the end of the file to follow POSIX standards. --- ## 😼 Mergecat review of go.mod ```diff @@ -14,7 +14,7 @@ require ( github.com/ghodss/yaml v1.0.0 github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 github.com/go-logr/zerologr v1.2.3 - github.com/google/go-github/v53 v53.2.0 + github.com/google/go-github/v62 v62.0.0 github.com/heptiolabs/healthcheck v0.0.0-20211123025425-613501dd5deb github.com/labstack/echo-contrib v0.16.0 github.com/labstack/echo/v4 v4.11.4 @@ -27,7 +27,7 @@ require ( github.com/prometheus/client_golang v1.19.0 github.com/rikatz/kubepug v1.4.0 github.com/rs/zerolog v1.32.0 - github.com/sashabaranov/go-openai v1.20.4 + github.com/sashabaranov/go-openai v1.26.2 github.com/shurcooL/githubv4 v0.0.0-20231126234147-1cffa1f02456 github.com/sirupsen/logrus v1.9.3 github.com/spf13/cobra v1.8.0 @@ -131,6 +131,7 @@ require ( github.com/google/btree v1.1.2 // indirect github.com/google/gnostic v0.6.9 // indirect github.com/google/go-cmp v0.6.0 // indirect + github.com/google/go-github/v53 v53.2.0 // indirect github.com/google/go-jsonnet v0.20.0 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/gofuzz v1.2.0 // indirect @@ -215,6 +216,7 @@ require ( github.com/spdx/tools-golang v0.5.3 // indirect github.com/spf13/afero v1.11.0 // indirect github.com/spf13/cast v1.6.0 // indirect + github.com/stretchr/objx v0.5.2 // indirect github.com/subosito/gotenv v1.6.0 // indirect github.com/tchap/go-patricia/v2 v2.3.1 // indirect github.com/tmccombs/hcl2json v0.3.6 // indirect ``` ### Feedback & Suggestions: 1. **Version Compatibility**: - Upgrading `github.com/google/go-github` from `v53` to `v62` and `github.com/sashabaranov/go-openai` from `v1.20.4` to `v1.26.2` is a significant jump. Ensure that these new versions are compatible with your existing codebase and that there are no breaking changes. Check the release notes and migration guides for these libraries. 2. **Indirect Dependencies**: - Adding `github.com/google/go-github/v53` as an indirect dependency while also upgrading it to `v62` directly might cause confusion. Ensure that this is intentional and that both versions are necessary. If not, consider removing the indirect dependency to avoid potential conflicts. - Similarly, adding `github.com/stretchr/objx v0.5.2` as an indirect dependency should be verified to ensure it doesn't introduce any unexpected behavior or conflicts. 3. **Testing**: - After making these changes, thoroughly test your application to ensure that everything works as expected. Pay special attention to areas where these libraries are used. 4. **Documentation**: - Update any relevant documentation to reflect these changes, especially if there are any new features or changes in behavior due to the library upgrades. By addressing these points, you can ensure a smooth transition to the new library versions and maintain the stability of your project. 🚀 --- ## 😼 Mergecat review of pkg/vcs/github_client/client.go ```diff @@ -10,7 +10,7 @@ import ( "strings" "github.com/chainguard-dev/git-urls" - "github.com/google/go-github/v53/github" + "github.com/google/go-github/v62/github" "github.com/pkg/errors" "github.com/rs/zerolog/log" "github.com/shurcooL/githubv4" @@ -26,12 +26,19 @@ var tracer = otel.Tracer("pkg/vcs/github_client") type Client struct { shurcoolClient *githubv4.Client - googleClient *github.Client + googleClient *GClient cfg config.ServerConfig username, email string } +// GClient is a struct that holds the services for the GitHub client +type GClient struct { + PullRequests PullRequestsServices + Repositories RepositoriesServices + Issues IssuesServices +} + // CreateGithubClient creates a new GitHub client using the auth token provided. We // can't validate the token at this point, so if it exists we assume it works func CreateGithubClient(cfg config.ServerConfig) (*Client, error) { @@ -54,13 +61,15 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) { tc := oauth2.NewClient(ctx, ts) githubUrl := cfg.VcsBaseUrl - if githubUrl == "" { + githubUploadUrl := cfg.VcsUploadUrl + // we need both urls to be set for github enterprise + if githubUrl == "" || githubUploadUrl == "" { googleClient = github.NewClient(tc) // If this has failed, we'll catch it on first call // Use the client from shurcooL's githubv4 library for queries. shurcoolClient = githubv4.NewClient(tc) } else { - googleClient, err = github.NewEnterpriseClient(githubUrl, githubUrl, tc) + googleClient, err = github.NewClient(tc).WithEnterpriseURLs(githubUrl, githubUploadUrl) if err != nil { log.Fatal().Err(err).Msg("failed to create github enterprise client") } @@ -73,8 +82,12 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) { } client := &Client{ - cfg: cfg, - googleClient: googleClient, + cfg: cfg, + googleClient: &GClient{ + PullRequests: PullRequestsService{googleClient.PullRequests}, + Repositories: RepositoriesService{googleClient.Repositories}, + Issues: IssuesService{googleClient.Issues}, + }, shurcoolClient: shurcoolClient, } if user != nil { @@ -219,9 +232,15 @@ func (c *Client) GetHookByUrl(ctx context.Context, ownerAndRepoName, webhookUrl } for _, item := range items { - if item.URL != nil && *item.URL == webhookUrl { + itemConfig := item.GetConfig() + // check if the hook's config has a URL + hookPayloadURL := "" + if itemConfig != nil { + hookPayloadURL = itemConfig.GetURL() + } + if hookPayloadURL == webhookUrl { return &vcs.WebHookConfig{ - Url: item.GetURL(), + Url: hookPayloadURL, Events: item.Events, // TODO: translate GH specific event names to VCS agnostic }, nil } @@ -232,23 +251,26 @@ func (c *Client) GetHookByUrl(ctx context.Context, ownerAndRepoName, webhookUrl func (c *Client) CreateHook(ctx context.Context, ownerAndRepoName, webhookUrl, webhookSecret string) error { owner, repoName := parseRepo(ownerAndRepoName) - _, _, err := c.googleClient.Repositories.CreateHook(ctx, owner, repoName, &github.Hook{ + _, resp, err := c.googleClient.Repositories.CreateHook(ctx, owner, repoName, &github.Hook{ Active: pkg.Pointer(true), - Config: map[string]interface{}{ - "content_type": "json", - "insecure_ssl": "0", - "secret": webhookSecret, - "url": webhookUrl, + Config: &github.HookConfig{ + ContentType: pkg.Pointer("json"), + InsecureSSL: pkg.Pointer("0"), + URL: pkg.Pointer(webhookUrl), + Secret: pkg.Pointer(webhookSecret), }, Events: []string{ "pull_request", }, Name: pkg.Pointer("web"), }) - if err != nil { - return errors.Wrap(err, "failed to create hook") + if err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300 { + statusCode := 0 + if resp != nil { + statusCode = resp.StatusCode + } + return errors.Wrap(err, fmt.Sprintf("failed to create hook, statuscode: %d", statusCode)) } - return nil } ``` Feedback & Suggestions: 1. **Version Update**: The update from `github.com/google/go-github/v53` to `github.com/google/go-github/v62` is good for staying current with the latest features and bug fixes. Ensure that all new methods and changes in the new version are compatible with your code. 2. **GClient Struct**: The introduction of the `GClient` struct to encapsulate the services is a good design choice for better organization and potential future extensions. However, ensure that the `PullRequestsService`, `RepositoriesService`, and `IssuesService` are correctly defined and initialized. 3. **Enterprise URLs**: The change to use `WithEnterpriseURLs` is appropriate for handling both GitHub and GitHub Enterprise URLs. However, ensure that `cfg.VcsUploadUrl` is always set correctly in the configuration to avoid runtime issues. 4. **Error Handling in CreateHook**: The addition of checking the response status code in `CreateHook` is a good improvement for more robust error handling. However, consider logging the response body for more detailed debugging information if the status code indicates an error. 5. **GetHookByUrl Method**: The change to extract the URL from the hook's config is a good improvement for robustness. However, ensure that `itemConfig.GetURL()` is always available and correctly parsed. 6. **Code Consistency**: Ensure that the new `GClient` struct and its usage are consistently applied throughout the codebase. This includes updating any other methods or functions that interact with the GitHub client. 7. **Testing**: After these changes, ensure thorough testing, especially for the new `GClient` struct and the updated methods. This includes unit tests and integration tests to verify that the new structure and error handling work as expected. --- By addressing these points, you can ensure that the code is more robust, maintainable, and ready for future extensions. 🛠️ ## 😼 Mergecat review of tools/dump_crds/go.mod ```diff @@ -1,7 +1,70 @@ module github.com/zapier/kubechecks/tools/dump_crds -go 1.21 +go 1.22.0 -toolchain go1.21.6 +toolchain go1.22.2 -require github.com/Masterminds/semver v1.5.0 +require ( + github.com/Masterminds/semver v1.5.0 + github.com/spf13/cobra v1.8.0 + github.com/spf13/viper v1.18.2 + k8s.io/apimachinery v0.30.1 + k8s.io/client-go v0.30.1 +) + +require ( + github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect + github.com/emicklei/go-restful/v3 v3.11.0 // indirect + github.com/fsnotify/fsnotify v1.7.0 // indirect + github.com/go-logr/logr v1.4.1 // indirect + github.com/go-openapi/jsonpointer v0.19.6 // indirect + github.com/go-openapi/jsonreference v0.20.2 // indirect + github.com/go-openapi/swag v0.22.3 // indirect + github.com/gogo/protobuf v1.3.2 // indirect + github.com/golang/protobuf v1.5.4 // indirect + github.com/google/gnostic-models v0.6.8 // indirect + github.com/google/gofuzz v1.2.0 // indirect + github.com/google/uuid v1.4.0 // indirect + github.com/hashicorp/hcl v1.0.0 // indirect + github.com/imdario/mergo v0.3.6 // indirect + github.com/inconshreveable/mousetrap v1.1.0 // indirect + github.com/josharian/intern v1.0.0 // indirect + github.com/json-iterator/go v1.1.12 // indirect + github.com/lmittmann/tint v1.0.4 // indirect + github.com/magiconair/properties v1.8.7 // indirect + github.com/mailru/easyjson v0.7.7 // indirect + github.com/mitchellh/mapstructure v1.5.0 // indirect + github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect + github.com/modern-go/reflect2 v1.0.2 // indirect + github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect + github.com/pelletier/go-toml/v2 v2.1.0 // indirect + github.com/sagikazarmark/locafero v0.4.0 // indirect + github.com/sagikazarmark/slog-shim v0.1.0 // indirect + github.com/sourcegraph/conc v0.3.0 // indirect + github.com/spf13/afero v1.11.0 // indirect + github.com/spf13/cast v1.6.0 // indirect + github.com/spf13/pflag v1.0.5 // indirect + github.com/subosito/gotenv v1.6.0 // indirect + go.uber.org/atomic v1.9.0 // indirect + go.uber.org/multierr v1.9.0 // indirect + golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect + golang.org/x/net v0.23.0 // indirect + golang.org/x/oauth2 v0.15.0 // indirect + golang.org/x/sys v0.18.0 // indirect + golang.org/x/term v0.18.0 // indirect + golang.org/x/text v0.14.0 // indirect + golang.org/x/time v0.5.0 // indirect + google.golang.org/appengine v1.6.7 // indirect + google.golang.org/protobuf v1.33.0 // indirect + gopkg.in/inf.v0 v0.9.1 // indirect + gopkg.in/ini.v1 v1.67.0 // indirect + gopkg.in/yaml.v2 v2.4.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/api v0.30.1 // indirect + k8s.io/klog/v2 v2.120.1 // indirect + k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect + k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect + sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect + sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect + sigs.k8s.io/yaml v1.3.0 // indirect +) ``` Feedback & Suggestions: 1. **Version Consistency**: Ensure that the Go version (`go 1.22.0`) and toolchain version (`go1.22.2`) are compatible with all the dependencies. Some dependencies might not yet support Go 1.22.0. 2. **Indirect Dependencies**: While it's common to have indirect dependencies, it's good practice to periodically review and prune them if they are no longer needed. This helps in reducing the overall dependency tree and potential security vulnerabilities. 3. **Dependency Updates**: Some of the indirect dependencies are quite old (e.g., `github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc`). Consider updating them to their latest versions to benefit from bug fixes and security patches. 4. **Grouping Dependencies**: Consider grouping direct and indirect dependencies separately for better readability. This can help in quickly identifying which dependencies are directly used by your module. --- ## 😼 Mergecat review of pkg/vcs/github_client/client_test.go ```diff package github_client import ( + "context" + "fmt" + "net/http" "testing" + "github.com/google/go-github/v62/github" + "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + githubMocks "github.com/zapier/kubechecks/mocks/github_client/mocks" + "github.com/zapier/kubechecks/pkg/config" + "github.com/zapier/kubechecks/pkg/vcs" ) +// MockGitHubMethod is a generic function to mock GitHub client methods +func MockGitHubMethod(methodName string, returns []interface{}) *GClient { + mockClient := new(githubMocks.MockRepositoriesServices) + mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...) + + return &GClient{ + Repositories: mockClient, + } +} + func TestParseRepo(t *testing.T) { testcases := []struct { name, input string @@ -40,3 +59,268 @@ func TestParseRepo(t *testing.T) { }) } } + +func TestClient_CreateHook(t *testing.T) { + type fields struct { + shurcoolClient *githubv4.Client + googleClient *GClient + cfg config.ServerConfig + username string + email string + } + type args struct { + ctx context.Context + ownerAndRepoName string + webhookUrl string + webhookSecret string + } + tests := []struct { + name string + fields fields + args args + wantErr assert.ErrorAssertionFunc + }{ + { + name: "normal ok", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("CreateHook", + []interface{}{ + &github.Hook{}, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + webhookSecret: "dummy-webhook-secret", + }, + wantErr: assert.NoError, + }, + { + name: "github responds with error", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("CreateHook", + []interface{}{ + nil, + &github.Response{Response: &http.Response{StatusCode: http.StatusBadRequest}}, + fmt.Errorf("mock bad request")}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + webhookSecret: "dummy-webhook-secret", + }, + wantErr: assert.Error, + }, + { + name: "mock network error error", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("CreateHook", + []interface{}{ + nil, + nil, + fmt.Errorf("mock network error")}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + webhookSecret: "dummy-webhook-secret", + }, + wantErr: assert.Error, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Client{ + shurcoolClient: tt.fields.shurcoolClient, + googleClient: tt.fields.googleClient, + cfg: tt.fields.cfg, + username: tt.fields.username, + email: tt.fields.email, + } + tt.wantErr(t, c.CreateHook(tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl, tt.args.webhookSecret), fmt.Sprintf("CreateHook(%v, %v, %v, %v)", tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl, tt.args.webhookSecret)) + }) + } +} + +func TestClient_GetHookByUrl(t *testing.T) { + type fields struct { + shurcoolClient *githubv4.Client + googleClient *GClient + cfg config.ServerConfig + username string + email string + } + type args struct { + ctx context.Context + ownerAndRepoName string + webhookUrl string + } + tests := []struct { + name string + fields fields + args args + want *vcs.WebHookConfig + wantErr assert.ErrorAssertionFunc + }{ + { + name: "normal ok", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("ListHooks", + []interface{}{ + []*github.Hook{ + { + Config: &github.HookConfig{ + ContentType: github.String("json"), + InsecureSSL: github.String("0"), + URL: github.String("https://dummywebhooks.local"), + Secret: github.String("dummy-webhook-secret"), + }, + Events: []string{"pull_request"}, + }, + }, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + }, + want: &vcs.WebHookConfig{ + Url: "https://dummywebhooks.local", + Events: []string{"pull_request"}, + }, + wantErr: assert.NoError, + }, + { + name: "no matching webhook found", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("ListHooks", + []interface{}{ + []*github.Hook{ + { + Config: &github.HookConfig{ + ContentType: github.String("json"), + InsecureSSL: github.String("0"), + URL: github.String("https://differentwebhook.local"), + Secret: github.String("dummy-webhook-secret"), + }, + Events: []string{"pull_request"}, + }, + }, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + }, + want: nil, + wantErr: assert.Error, + }, + { + name: "0 webhook found", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("ListHooks", + []interface{}{ + nil, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + }, + want: nil, + wantErr: assert.Error, + }, + { + name: "github error", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubMethod("ListHooks", + []interface{}{ + nil, + &github.Response{Response: &http.Response{StatusCode: http.StatusBadRequest}}, + fmt.Errorf("mock bad request")}), + cfg: config.ServerConfig{ + VcsToken: "ghp_helloworld", + VcsType: "github", + }, + username: "dummy-bot", + email: "dummy@zapier.com", + }, + args: args{ + ctx: context.Background(), + ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git", + webhookUrl: "https://dummywebhooks.local", + }, + want: nil, + wantErr: assert.Error, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Client{ + shurcoolClient: tt.fields.shurcoolClient, + googleClient: tt.fields.googleClient, + cfg: tt.fields.cfg, + username: tt.fields.username, + email: tt.fields.email, + } + got, err := c.GetHookByUrl(tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl) + if !tt.wantErr(t, err, fmt.Sprintf("GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)) { + return + } + assert.Equalf(t, tt.want, got, "GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl) + }) + } +} ``` ### Feedback & Suggestions: 1. **Security Concerns**: - **Hardcoded Tokens**: The `VcsToken` field in `config.ServerConfig` is hardcoded with a placeholder token (`"ghp_helloworld"`). This should be replaced with a more secure method of injecting secrets, such as environment variables or a secrets management service. - **Sensitive Information in URLs**: The `ownerAndRepoName` field contains a URL with a username and password (`"https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git"`). This is a potential security risk. Consider using OAuth tokens or other secure methods for authentication. 2. **Error Handling**: - **Detailed Error Messages**: When asserting errors, it would be helpful to include more detailed error messages to aid in debugging. For example, `assert.Error(t, err, "expected error but got none")`. 3. **Mocking Improvements**: - **Mock Reusability**: The `MockGitHubMethod` function is a good start, but it could be extended to handle different methods more flexibly. Consider adding parameters for method arguments and return values to make it more reusable. 4. **Code Duplication**: - **Reduce Duplication**: The `fields` and `args` structs are repeated in multiple test cases. Consider creating helper functions to initialize these structs to reduce duplication and improve readability. 5. **Test Coverage**: - **Additional Test Cases**: Consider adding more test cases to cover edge cases and potential failure scenarios, such as invalid URLs or missing required fields. 6. **Documentation**: - **Function Comments**: Adding comments to the new functions (`MockGitHubMethod`, `TestClient_CreateHook`, `TestClient_GetHookByUrl`) would improve code readability and maintainability. --- By addressing these points, the code will be more secure, maintainable, and easier to understand. Keep up the good work! 😸 ## 😼 Mergecat review of tools/dump_crds/go.sum ```diff @@ -1,2 +1,198 @@ github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww= github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= +github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= +github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= +github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/emicklei/go-restful/v3 v3.11.0 h1:rAQeMHw1c7zTmncogyy8VvRZwtkmkZ4FxERmMY4rD+g= +github.com/emicklei/go-restful/v3 v3.11.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc= +github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= +github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= +github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= +github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= +github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ= +github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-openapi/jsonpointer v0.19.6 h1:eCs3fxoIi3Wh6vtgmLTOjdhSpiqphQ+DaPn38N2ZdrE= +github.com/go-openapi/jsonpointer v0.19.6/go.mod h1:osyAmYz/mB/C3I+WsTTSgw1ONzaLJoLCyoi6/zppojs= +github.com/go-openapi/jsonreference v0.20.2 h1:3sVjiK66+uXK/6oQ8xgcRKcFgQ5KXa2KvnJRumpMGbE= +github.com/go-openapi/jsonreference v0.20.2/go.mod h1:Bl1zwGIM8/wsvqjsOQLJ/SH+En5Ap4rVB5KVcIDZG2k= +github.com/go-openapi/swag v0.22.3 h1:yMBqmnQ0gyZvEb/+KzuWZOXgllrXT4SADYbvDaXHv/g= +github.com/go-openapi/swag v0.22.3/go.mod h1:UzaqsxGiab7freDnrUUra0MwWfN/q7tE4j+VcZ0yl14= +github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 h1:tfuBGBXKqDEevZMzYi5KSi8KkcZtzBcTgAUUtapy0OI= +github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572/go.mod h1:9Pwr4B2jHnOSGXyyzV8ROjYa2ojvAY6HCGYYfMoC3Ls= +github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= +github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= +github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= +github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= +github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= +github.com/google/gnostic-models v0.6.8 h1:yo/ABAfM5IMRsS1VnXjTBvUb61tFIHozhlYvRgGre9I= +github.com/google/gnostic-models v0.6.8/go.mod h1:5n7qKqH0f5wFt+aWF8CW6pZLLNOfYuF5OpfBSENuI8U= +github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= +github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 h1:K6RDEckDVWvDI9JAJYCmNdQXq6neHJOYx3V6jnqNEec= +github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= +github.com/google/uuid v1.4.0 h1:MtMxsa51/r9yyhkyLsVeVt0B+BGQZzpQiTQ4eHZ8bc4= +github.com/google/uuid v1.4.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4= +github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= +github.com/imdario/mergo v0.3.6 h1:xTNEAn+kxVO7dTZGu0CegyqKZmoWFI0rF8UxjlB2d28= +github.com/imdario/mergo v0.3.6/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= +github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= +github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY= +github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y= +github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= +github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= +github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8= +github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= +github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= +github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= +github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= +github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= +github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= +github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= +github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/lmittmann/tint v1.0.4 h1:LeYihpJ9hyGvE0w+K2okPTGUdVLfng1+nDNVR4vWISc= +github.com/lmittmann/tint v1.0.4/go.mod h1:HIS3gSy7qNwGCj+5oRjAutErFBl4BzdQP6cJZ0NfMwE= +github.com/magiconair/properties v1.8.7 h1:IeQXZAiQcpL9mgcAe1Nu6cX9LLw6ExEHKjN0VQdvPDY= +github.com/magiconair/properties v1.8.7/go.mod h1:Dhd985XPs7jluiymwWYZ0G4Z61jb3vdS329zhj2hYo0= +github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0= +github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= +github.com/mitchellh/mapstructure v1.5.0 h1:jeMsZIYE/09sWLaz43PL7Gy6RuMjD2eJVyuac5Z2hdY= +github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= +github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= +github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= +github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= +github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9Gz0M= +github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= +github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= +github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= +github.com/onsi/ginkgo/v2 v2.15.0 h1:79HwNRBAZHOEwrczrgSOPy+eFTTlIGELKy5as+ClttY= +github.com/onsi/ginkgo/v2 v2.15.0/go.mod h1:HlxMHtYF57y6Dpf+mc5529KKmSq9h2FpCF+/ZkwUxKM= +github.com/onsi/gomega v1.31.0 h1:54UJxxj6cPInHS3a35wm6BK/F9nHYueZ1NVujHDrnXE= +github.com/onsi/gomega v1.31.0/go.mod h1:DW9aCi7U6Yi40wNVAvT6kzFnEVEI5n3DloYBiKiT6zk= +github.com/pelletier/go-toml/v2 v2.1.0 h1:FnwAJ4oYMvbT/34k9zzHuZNrhlz48GB3/s6at6/MHO4= +github.com/pelletier/go-toml/v2 v2.1.0/go.mod h1:tJU2Z3

Dependency Review

Click to read mergecats review! No suggestions found