add option to rerun kubecheck plan (GitHub Only)

[KyriosGN0]

Fixes #105

[Greyeye]

Hi @KyriosGN0, thanks for the PR. would you mind adding a test case for the ParseHook()?

[KyriosGN0]

hi @Greyeye, is there some example on how to do it, im not sure if i should/can mock the entire HTTP request that a webhook sends, would it be possible to have some help? i did test it with tilt and it works.

[Greyeye]

@KyriosGN0 i've pushed PR to your branch. it contains a few fixes and unit tests. also include changes to GitLab support.

[KyriosGN0]

@Greyeye merged your PR, thx a lot

[KyriosGN0]

@Greyeye can you rerun the CI ? i don't see anything weird in the ci logs

[Greyeye]

CI is trying to push the docker image to the org repo, and it's failing. think we need to filter the run when it is executed from forked repo.

@KyriosGN0 can you change

.github/workflows/on_pull_request.yaml .github/workflows/on_pull_request_closed.yaml

and add github.repository == 'zapier/kubechecks' under runs-on

    runs-on: ubuntu-latest
    if: github.repository == 'zapier/kubechecks'

[KyriosGN0]

@Greyeye added

[KyriosGN0]

@Greyeye looks like it failed still

[KyriosGN0]

@Greyeye any update ?

[djeebus]

Looks great! I think the wording could use some work though; check out my comment?

[KyriosGN0]

@djeebus added it

[Greyeye]

@KyriosGN0 doc lint and ci are failing, would you be able to check these please.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

## 😼 Mergecat review of .github/workflows/on_pull_request.yaml ```diff @@ -20,6 +20,7 @@ env: jobs: build: runs-on: ubuntu-22.04 + if: github.repository == 'zapier/kubechecks' permissions: contents: read ``` Feedback & Suggestions: 1. **Conditional Execution**: The addition of the `if` condition to check the repository name is a good practice to ensure the job runs only for a specific repository. However, consider making this condition more flexible by using a list of allowed repositories if you anticipate needing this for multiple repositories in the future. ```yaml if: contains(['zapier/kubechecks', 'another/repo'], github.repository) ``` 2. **Documentation**: Add a comment explaining why this condition is necessary. This will help future maintainers understand the context. ```yaml # Run this job only for the 'zapier/kubechecks' repository if: github.repository == 'zapier/kubechecks' ``` 3. **Security**: Ensure that the `GITHUB_TOKEN` used in the `build-image` step has the least privileges necessary. Double-check the permissions to avoid any potential security risks. --- ## 😼 Mergecat review of .mockery.yaml ```diff @@ -5,6 +5,10 @@ packages: # place your package-specific config here config: all: true + github.com/zapier/kubechecks/pkg/vcs/gitlab_client: + # place your package-specific config here + config: + all: true github.com/zapier/kubechecks/pkg/generator: # place your package-specific config here config: ``` Feedback & Suggestions: 1. **Trailing Whitespace**: There is a trailing whitespace after `all: true` in the new `gitlab_client` entry. This can be removed to maintain consistency and avoid potential issues with parsers. 2. **Alphabetical Order**: For better readability and maintainability, consider keeping the package entries in alphabetical order. This will make it easier to locate specific entries in the future. Suggested Diff: ```diff @@ -5,6 +5,10 @@ packages: # place your package-specific config here config: all: true + github.com/zapier/kubechecks/pkg/vcs/gitlab_client: + # place your package-specific config here + config: + all: true github.com/zapier/kubechecks/pkg/generator: # place your package-specific config here config: ``` --- ## 😼 Mergecat review of pkg/config/config.go ```diff @@ -70,6 +70,7 @@ type ServerConfig struct { TidyOutdatedCommentsMode string `mapstructure:"tidy-outdated-comments-mode"` MaxQueueSize int64 `mapstructure:"max-queue-size"` MaxConcurrenctChecks int `mapstructure:"max-concurrenct-checks"` + ReplanCommentMessage string `mapstructure:"replan-comment-msg"` } func New() (ServerConfig, error) { ``` Feedback & Suggestions: 1. **Typo in Field Name**: The field `MaxConcurrenctChecks` has a typo. It should be `MaxConcurrentChecks`. This typo was present in the original code, but it would be good to correct it now to avoid confusion. 2. **Security Consideration**: Ensure that the new `ReplanCommentMessage` field does not contain any sensitive information that could be exposed in logs or other outputs. 3. **Validation**: Consider adding validation for the new `ReplanCommentMessage` field to ensure it meets any necessary criteria (e.g., length, allowed characters). --- ## 😼 Mergecat review of .github/workflows/on_pull-request_closed.yaml ```diff @@ -12,6 +12,8 @@ env: jobs: remove-temp-image: runs-on: ubuntu-22.04 + if: github.repository == 'zapier/kubechecks' + continue-on-error: true # should match env.FS_TAG, in both pr-open.yaml and pr-close.yaml concurrency: pr-${{ github.event.pull_request.number }} ``` Feedback & Suggestions: 1. **Conditional Execution**: The addition of `if: github.repository == 'zapier/kubechecks'` is a good practice to ensure the job runs only for the specified repository. However, ensure that this condition is necessary and correctly scoped to avoid unintended skips. 2. **Error Handling**: The `continue-on-error: true` directive allows the workflow to proceed even if this job fails. This can be useful, but it might mask underlying issues. Consider whether you want to be notified of failures instead of silently continuing. If you decide to keep it, ensure you have other mechanisms to monitor and handle potential errors. 3. **Security Considerations**: Ensure that the `secrets.GITHUB_TOKEN` used in the steps has the minimum required permissions. This is crucial for maintaining the security of your workflow. 4. **Documentation**: Add comments explaining why `continue-on-error: true` is used. This will help future maintainers understand the rationale behind this decision. Example: ```yaml jobs: remove-temp-image: runs-on: ubuntu-22.04 if: github.repository == 'zapier/kubechecks' # Allow the workflow to continue even if this job fails continue-on-error: true ``` --- ## 😼 Mergecat review of pkg/vcs/gitlab_client/status.go ```diff @@ -92,3 +92,11 @@ func configureBackOff() *backoff.ExponentialBackOff { return expBackOff } + +type CommitsServices interface { + SetCommitStatus(pid interface{}, sha string, opt *gitlab.SetCommitStatusOptions, options ...gitlab.RequestOptionFunc) (*gitlab.CommitStatus, *gitlab.Response, error) +} + +type CommitsService struct { + CommitsServices +} ``` Feedback & Suggestions: 1. **Interface Naming**: The name `CommitsServices` is a bit confusing and doesn't follow Go naming conventions. Typically, interfaces are named in a way that describes the behavior they encapsulate. Consider renaming it to `CommitStatusSetter` or something more descriptive. 2. **Redundant Struct**: The `CommitsService` struct embedding `CommitsServices` interface seems redundant unless you plan to add more fields or methods to `CommitsService`. If not, you can directly use the interface without wrapping it in a struct. 3. **Documentation**: Adding comments to the new interface and struct would improve code readability and maintainability. Explain what the interface and struct are intended for. 4. **Interface Method Parameters**: The `pid` parameter in the `SetCommitStatus` method is of type `interface{}`. If possible, use a more specific type to avoid type assertions and potential runtime errors. Here is a revised version of the diff: ```diff @@ -92,3 +92,11 @@ func configureBackOff() *backoff.ExponentialBackOff { return expBackOff } + +// CommitStatusSetter defines the behavior for setting commit statuses. +type CommitStatusSetter interface { + SetCommitStatus(pid string, sha string, opt *gitlab.SetCommitStatusOptions, options ...gitlab.RequestOptionFunc) (*gitlab.CommitStatus, *gitlab.Response, error) +} + +// GitlabCommitService implements CommitStatusSetter for GitLab. +type GitlabCommitService struct { + Client *gitlab.Client +} ``` --- ## 😼 Mergecat review of pkg/vcs/gitlab_client/pipeline.go ```diff @@ -57,3 +57,11 @@ func (c *Client) GetLastPipelinesForCommit(projectName string, commitSHA string) return nil } + +type PipelinesServices interface { + ListProjectPipelines(pid interface{}, opt *gitlab.ListProjectPipelinesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.PipelineInfo, *gitlab.Response, error) +} + +type PipelinesService struct { + PipelinesServices +} ``` Feedback & Suggestions: 1. **Naming Consistency**: The interface name `PipelinesServices` is a bit confusing and not consistent with Go naming conventions. Typically, interfaces are named in singular form and describe the behavior they encapsulate. Consider renaming it to `PipelineService` or `PipelineLister`. 2. **Redundant Struct**: The `PipelinesService` struct embedding `PipelinesServices` interface seems redundant unless you plan to add more fields or methods to it. If not, you can directly use the interface without wrapping it in a struct. 3. **Documentation**: Adding comments to the new interface and struct would improve code readability and maintainability. It helps other developers understand the purpose and usage of these types. 4. **Interface Methods**: Ensure that the interface methods are well-defined and cover all necessary operations you might need for interacting with pipelines. Currently, it only includes `ListProjectPipelines`. Here is a revised version of the diff with these suggestions: ```diff @@ -57,3 +57,11 @@ func (c *Client) GetLastPipelinesForCommit(projectName string, commitSHA string) return nil } + +// PipelineService defines the methods for interacting with GitLab pipelines. +type PipelineService interface { + ListProjectPipelines(pid interface{}, opt *gitlab.ListProjectPipelinesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.PipelineInfo, *gitlab.Response, error) +} + +// GitLabPipelineService implements the PipelineService interface. +type GitLabPipelineService struct { + PipelineService +} ``` --- ## 😼 Mergecat review of cmd/root.go ```diff @@ -108,6 +108,9 @@ func init() { stringFlag(flags, "worst-hooks-state", "The worst state that can be returned from the hooks renderer.", newStringOpts(). withDefault("panic")) + stringFlag(flags, "replan-comment-msg", "comment message which re-triggers kubechecks on PR.", + newStringOpts(). + withDefault("kubechecks again")) panicIfError(viper.BindPFlags(flags)) setupLogOutput() ``` Feedback & Suggestions: 1. **Security**: Ensure that the `replan-comment-msg` value is sanitized before use to prevent potential injection attacks or misuse. This is especially important if the value is used in any command execution or logging. ```go import "html" ... sanitizedCommentMsg := html.EscapeString(viper.GetString("replan-comment-msg")) ``` 2. **Documentation**: It would be helpful to add a comment explaining the purpose of the new flag `replan-comment-msg` and how it is intended to be used. This will aid future developers in understanding its context. 3. **Consistency**: Ensure that the new flag is documented in any user-facing documentation or help messages if applicable. This will help users understand the new functionality. 4. **Validation**: Consider adding validation to ensure that the `replan-comment-msg` is not empty or does not contain only whitespace. This can prevent potential issues during runtime. ```go if strings.TrimSpace(viper.GetString("replan-comment-msg")) == "" { log.Fatal().Msg("replan-comment-msg cannot be empty") } ``` --- ## 😼 Mergecat review of pkg/server/hook_handler.go ```diff @@ -47,11 +47,11 @@ func (h *VCSHookHandler) groupHandler(c echo.Context) error { return c.String(http.StatusUnauthorized, "Unauthorized") } - pr, err := h.ctr.VcsClient.ParseHook(c.Request(), payload) + pr, err := h.ctr.VcsClient.ParseHook(ctx, c.Request(), payload) if err != nil { switch err { case vcs.ErrInvalidType: - log.Debug().Msg("Ignoring event, not a merge request") + log.Debug().Msg("Ignoring event, not a supported request") return c.String(http.StatusOK, "Skipped") default: // TODO: do something ELSE with the error ``` Feedback & Suggestions: 1. **Context Propagation**: Adding the `ctx` parameter to `ParseHook` is a good improvement for context propagation. Ensure that the `ParseHook` method in the `VcsClient` interface and its implementations are updated accordingly to handle the context. 2. **Log Message Clarity**: Changing the log message from "Ignoring event, not a merge request" to "Ignoring event, not a supported request" is more inclusive of different unsupported event types. This is a positive change for clarity. 3. **Error Handling**: The TODO comment "do something ELSE with the error" suggests that error handling could be improved. Consider implementing a more specific error handling strategy here. For example, you could log different types of errors differently or take specific actions based on the error type. 4. **Security Consideration**: Ensure that the `ctx` being passed to `ParseHook` does not contain sensitive information that could be inadvertently logged or exposed. Overall, the changes are beneficial for context management and log clarity. Just ensure that all related code is updated to handle the new `ctx` parameter and consider improving the error handling strategy. --- ## 😼 Mergecat review of pkg/vcs/types.go ```diff @@ -22,8 +22,8 @@ type Client interface { UpdateMessage(context.Context, *msg.Message, string) error // VerifyHook validates a webhook secret and return the body; must be called even if no secret VerifyHook(*http.Request, string) ([]byte, error) - // ParseHook parses webook payload for valid events - ParseHook(*http.Request, []byte) (PullRequest, error) + // ParseHook parses webook payload for valid events, with context for request-scoped values + ParseHook(context.Context, *http.Request, []byte) (PullRequest, error) // CommitStatus sets a status for a specific commit on the remote VCS CommitStatus(context.Context, PullRequest, pkg.CommitState) error // GetHookByUrl gets a webhook by url ``` ### Feedback & Suggestions: 1. **Context Propagation**: Adding `context.Context` to `ParseHook` is a good idea for propagating request-scoped values like deadlines, cancellation signals, and other request-specific data. Ensure that all implementations of this interface method are updated accordingly to handle the new context parameter. 2. **Documentation Update**: The comment for `ParseHook` has been updated to reflect the addition of the context parameter. This is good practice. Ensure that all related documentation and usage examples are also updated to avoid confusion. 3. **Backward Compatibility**: If this interface is implemented by multiple types, ensure that all implementations are updated to match the new method signature. This change is not backward-compatible, so any existing code that implements this interface will break until updated. 4. **Error Handling**: With the addition of `context.Context`, consider how context cancellation and timeouts are handled within the `ParseHook` method. Ensure that the method respects the context's deadline and cancellation signals to avoid potential resource leaks or hanging operations. Overall, this is a positive change that enhances the flexibility and robustness of the `ParseHook` method. Just make sure to update all implementations and handle the context appropriately. 🚀 --- ## 😼 Mergecat review of docs/usage.md ```diff @@ -60,6 +60,7 @@ The full list of supported environment variables is described below: |`KUBECHECKS_OTEL_ENABLED`|Enable OpenTelemetry.|`false`| |`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_REPLAN_COMMENT_MSG`|comment message which re-triggers kubechecks on PR.|`kubechecks again`| |`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.|`[]`| |`KUBECHECKS_SHOW_DEBUG_INFO`|Set to true to print debug info to the footer of MR comments.|`false`| ``` Feedback & Suggestions: 1. **Consistency in Description**: The description for `KUBECHECKS_REPLAN_COMMENT_MSG` could be more consistent with the style of other descriptions. Consider rephrasing it to: "The comment message that re-triggers kubechecks on a PR." 2. **Clarification**: It might be helpful to clarify what "PR" stands for, especially for users who might not be familiar with the abbreviation. Consider using "Pull Request (PR)" for clarity. 3. **Security Consideration**: Ensure that the value for `KUBECHECKS_REPLAN_COMMENT_MSG` is not something that could be easily guessed or misused by unauthorized users. If this is a security-sensitive feature, consider adding a note about securing this value. Example: ```diff +|`KUBECHECKS_REPLAN_COMMENT_MSG`|The comment message that re-triggers kubechecks on a Pull Request (PR). Ensure this value is secure and not easily guessable.|`kubechecks again`| ``` --- ## 😼 Mergecat review of pkg/vcs/gitlab_client/merge.go ```diff @@ -83,3 +83,15 @@ func (chg *Changes) validChange(path, fileType string) bool { return false } + +type MergeRequestsServices interface { + GetMergeRequestDiffVersions(pid interface{}, mergeRequest int, opt *gitlab.GetMergeRequestDiffVersionsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.MergeRequestDiffVersion, *gitlab.Response, error) + GetMergeRequestChanges(pid interface{}, mergeRequest int, opt *gitlab.GetMergeRequestChangesOptions, options ...gitlab.RequestOptionFunc) (*gitlab.MergeRequest, *gitlab.Response, error) + ListMergeRequestDiffs(pid interface{}, mergeRequest int, opt *gitlab.ListMergeRequestDiffsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.MergeRequestDiff, *gitlab.Response, error) + UpdateMergeRequest(pid interface{}, mergeRequest int, opt *gitlab.UpdateMergeRequestOptions, options ...gitlab.RequestOptionFunc) (*gitlab.MergeRequest, *gitlab.Response, error) + GetMergeRequest(pid interface{}, mergeRequest int, opt *gitlab.GetMergeRequestsOptions, options ...gitlab.RequestOptionFunc) (*gitlab.MergeRequest, *gitlab.Response, error) +} + +type MergeRequestsService struct { + MergeRequestsServices +} ``` Feedback & Suggestions: 1. **Interface Naming**: The name `MergeRequestsServices` is a bit confusing and redundant. Consider renaming it to `MergeRequestService` to better reflect its purpose and to align with Go naming conventions. ```go type MergeRequestService interface { // methods } ``` 2. **Embedding Interface**: The struct `MergeRequestsService` embeds the interface `MergeRequestsServices`. This is a good practice, but ensure that this embedding is necessary for your use case. If you plan to add more methods to the struct later, this is fine. Otherwise, you might just use the interface directly. 3. **Documentation**: Adding comments to the interface methods would be beneficial for future maintainers. This will help understand the purpose of each method without diving into the implementation. 4. **Error Handling**: Ensure that the methods in the interface handle errors appropriately. Since this is an interface, the actual implementation will need to handle errors, but it's good to document expected error handling practices. 5. **Context Parameter**: Consider adding a `context.Context` parameter to each method in the interface. This is a common practice in Go to handle timeouts and cancellations. ```go type MergeRequestService interface { GetMergeRequestDiffVersions(ctx context.Context, pid interface{}, mergeRequest int, opt *gitlab.GetMergeRequestDiffVersionsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.MergeRequestDiffVersion, *gitlab.Response, error) // other methods } ``` By addressing these points, the code will be more maintainable, readable, and aligned with Go best practices. 🛠️ --- ## 😼 Mergecat review of go.mod ```diff @@ -41,7 +41,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/spf13/viper v1.18.2 github.com/stretchr/testify v1.9.0 - github.com/xanzy/go-gitlab v0.105.0 + github.com/xanzy/go-gitlab v0.107.0 github.com/yannh/kubeconform v0.6.4 github.com/ziflex/lecho/v3 v3.5.0 go.opentelemetry.io/contrib/instrumentation/runtime v0.53.0 @@ -171,7 +171,7 @@ require ( github.com/grpc-ecosystem/grpc-gateway/v2 v2.20.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-getter v1.7.5 // indirect - github.com/hashicorp/go-retryablehttp v0.7.5 // indirect + github.com/hashicorp/go-retryablehttp v0.7.7 // indirect github.com/hashicorp/go-safetemp v1.0.0 // indirect github.com/hashicorp/go-version v1.6.0 // indirect github.com/hashicorp/hcl v1.0.0 // indirect ``` Feedback & Suggestions: 1. **Dependency Updates**: The updates to `github.com/xanzy/go-gitlab` and `github.com/hashicorp/go-retryablehttp` are generally good for keeping dependencies current. However, ensure that these updates are compatible with your codebase and do not introduce breaking changes. Run your test suite thoroughly to verify this. 2. **Changelog Review**: Check the changelogs or release notes for `v0.107.0` of `github.com/xanzy/go-gitlab` and `v0.7.7` of `github.com/hashicorp/go-retryablehttp` to understand the changes and new features or bug fixes they bring. This can help in identifying any necessary code adjustments or new opportunities for improvement. 3. **Security Considerations**: Ensure that the new versions do not introduce any security vulnerabilities. Tools like `dependabot` or `snyk` can help in identifying such issues. 4. **Indirect Dependencies**: The `indirect` dependencies are also updated. Make sure these updates do not affect the direct dependencies or the overall functionality of your project. 5. **Version Pinning**: Consider pinning the versions of your dependencies to avoid unexpected updates in the future. This can be done by specifying exact versions rather than ranges. --- ## 😼 Mergecat review of pkg/vcs/gitlab_client/message.go ```diff @@ -161,3 +161,14 @@ func (c *Client) TidyOutdatedComments(ctx context.Context, pr vcs.PullRequest) e return c.hideOutdatedMessages(ctx, pr.FullName, pr.CheckID, allNotes) } + +type NotesServices interface { + CreateMergeRequestNote(pid interface{}, mergeRequest int, opt *gitlab.CreateMergeRequestNoteOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Note, *gitlab.Response, error) + UpdateMergeRequestNote(pid interface{}, mergeRequest, note int, opt *gitlab.UpdateMergeRequestNoteOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Note, *gitlab.Response, error) + DeleteMergeRequestNote(pid interface{}, mergeRequest, note int, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) + ListMergeRequestNotes(pid interface{}, mergeRequest int, opt *gitlab.ListMergeRequestNotesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Note, *gitlab.Response, error) +} + +type NotesService struct { + NotesServices +} ``` ### Feedback & Suggestions: 1. **Interface Naming Consistency**: The interface name `NotesServices` should be singular to follow Go naming conventions for interfaces, which typically use singular nouns. Consider renaming it to `NotesService`. 2. **Redundant Struct**: The `NotesService` struct embedding the `NotesServices` interface seems redundant unless you plan to add more fields or methods to it. If not, you can directly use the interface. 3. **Documentation**: Adding comments to the new interface and struct would improve code readability and maintainability. This is especially useful for other developers who might work on this code in the future. 4. **Security Consideration**: Ensure that the `pid` parameter is properly validated and sanitized to prevent any potential security vulnerabilities, such as injection attacks. ### Suggested Diff: ```diff @@ -161,3 +161,14 @@ func (c *Client) TidyOutdatedComments(ctx context.Context, pr vcs.PullRequest) e return c.hideOutdatedMessages(ctx, pr.FullName, pr.CheckID, allNotes) } + +// NotesService defines the methods required to interact with GitLab notes. +type NotesService interface { + CreateMergeRequestNote(pid interface{}, mergeRequest int, opt *gitlab.CreateMergeRequestNoteOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Note, *gitlab.Response, error) + UpdateMergeRequestNote(pid interface{}, mergeRequest, note int, opt *gitlab.UpdateMergeRequestNoteOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Note, *gitlab.Response, error) + DeleteMergeRequestNote(pid interface{}, mergeRequest, note int, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) + ListMergeRequestNotes(pid interface{}, mergeRequest int, opt *gitlab.ListMergeRequestNotesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Note, *gitlab.Response, error) +} + +// NotesServiceImpl is an implementation of the NotesService interface. +type NotesServiceImpl struct { + NotesService +} ``` --- ## 😼 Mergecat review of pkg/vcs/gitlab_client/project.go ```diff @@ -42,3 +42,22 @@ func (c *Client) GetRepoConfigFile(ctx context.Context, projectId int, mergeReqI return nil, errors.New(".kubecheck.yaml file not found") } + +type ProjectsServices interface { + AddProjectHook(pid interface{}, opt *gitlab.AddProjectHookOptions, options ...gitlab.RequestOptionFunc) (*gitlab.ProjectHook, *gitlab.Response, error) + ListProjectHooks(pid interface{}, opt *gitlab.ListProjectHooksOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.ProjectHook, *gitlab.Response, error) + EditProjectHook(pid interface{}, hook int, opt *gitlab.EditProjectHookOptions, options ...gitlab.RequestOptionFunc) (*gitlab.ProjectHook, *gitlab.Response, error) + GetProject(pid interface{}, opt *gitlab.GetProjectOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Project, *gitlab.Response, error) +} + +type ProjectsService struct { + ProjectsServices +} + +type RepositoryFilesServices interface { + GetRawFile(pid interface{}, fileName string, opt *gitlab.GetRawFileOptions, options ...gitlab.RequestOptionFunc) ([]byte, *gitlab.Response, error) +} + +type RepositoryFilesService struct { + RepositoryFilesServices +} ``` Feedback & Suggestions: 1. **Interface Naming Consistency**: The interface names `ProjectsServices` and `RepositoryFilesServices` are a bit unconventional. Typically, Go interfaces are named in singular form and describe the behavior they encapsulate. Consider renaming them to `ProjectService` and `RepositoryFileService` respectively. 2. **Struct Embedding**: The struct names `ProjectsService` and `RepositoryFilesService` are fine, but embedding the interfaces directly might be confusing. Instead, consider embedding the interfaces with a more descriptive name or using a field name that clarifies their purpose. 3. **Error Handling**: The new code does not introduce any error handling improvements. Ensure that the methods in these interfaces handle errors appropriately when implemented. 4. **Documentation**: Adding comments to describe the purpose of these interfaces and structs would be beneficial for future maintainers. 5. **Security Considerations**: Ensure that the methods defined in these interfaces are implemented with proper security checks, especially when dealing with project hooks and raw file access. Suggested Diff: ```diff @@ -42,3 +42,22 @@ func (c *Client) GetRepoConfigFile(ctx context.Context, projectId int, mergeReqI return nil, errors.New(".kubecheck.yaml file not found") } + +// ProjectService defines the methods related to project operations +type ProjectService interface { + AddProjectHook(pid interface{}, opt *gitlab.AddProjectHookOptions, options ...gitlab.RequestOptionFunc) (*gitlab.ProjectHook, *gitlab.Response, error) + ListProjectHooks(pid interface{}, opt *gitlab.ListProjectHooksOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.ProjectHook, *gitlab.Response, error) + EditProjectHook(pid interface{}, hook int, opt *gitlab.EditProjectHookOptions, options ...gitlab.RequestOptionFunc) (*gitlab.ProjectHook, *gitlab.Response, error) + GetProject(pid interface{}, opt *gitlab.GetProjectOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Project, *gitlab.Response, error) +} + +// ProjectsService embeds ProjectService interface +type ProjectsService struct { + ProjectService +} + +// RepositoryFileService defines the methods related to repository file operations +type RepositoryFileService interface { + GetRawFile(pid interface{}, fileName string, opt *gitlab.GetRawFileOptions, options ...gitlab.RequestOptionFunc) ([]byte, *gitlab.Response, error) +} + +// RepositoryFilesService embeds RepositoryFileService interface +type RepositoryFilesService struct { + RepositoryFileService +} ``` --- ## 😼 Mergecat review of pkg/vcs/github_client/client.go ```diff @@ -5,11 +5,12 @@ import ( "fmt" "io" "net/http" + "net/url" "regexp" "strconv" "strings" - "github.com/chainguard-dev/git-urls" + giturls "github.com/chainguard-dev/git-urls" "github.com/google/go-github/v62/github" "github.com/pkg/errors" "github.com/rs/zerolog/log" @@ -127,7 +128,7 @@ func (c *Client) VerifyHook(r *http.Request, secret string) ([]byte, error) { var nilPr vcs.PullRequest -func (c *Client) ParseHook(r *http.Request, request []byte) (vcs.PullRequest, error) { +func (c *Client) ParseHook(ctx context.Context, r *http.Request, request []byte) (vcs.PullRequest, error) { payload, err := github.ParseWebHook(github.WebHookType(r), request) if err != nil { return nilPr, err @@ -143,6 +144,20 @@ func (c *Client) ParseHook(r *http.Request, request []byte) (vcs.PullRequest, er log.Info().Str("action", p.GetAction()).Msg("ignoring Github pull request event due to non commit based action") return nilPr, vcs.ErrInvalidType } + case *github.IssueCommentEvent: + switch p.GetAction() { + case "created": + if strings.ToLower(p.Comment.GetBody()) == c.cfg.ReplanCommentMessage { + log.Info().Msgf("Got %s comment, Running again", c.cfg.ReplanCommentMessage) + return c.buildRepoFromComment(ctx, p), nil + } else { + log.Info().Str("action", p.GetAction()).Msg("ignoring Github issue comment event due to non matching string") + return nilPr, vcs.ErrInvalidType + } + default: + log.Info().Str("action", p.GetAction()).Msg("ignoring Github issue comment event due to non matching string") + return nilPr, vcs.ErrInvalidType + } default: log.Error().Msg("invalid event provided to Github client") return nilPr, vcs.ErrInvalidType @@ -156,15 +171,62 @@ func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) vcs.PullRequ } return vcs.PullRequest{ - BaseRef: *event.PullRequest.Base.Ref, - HeadRef: *event.PullRequest.Head.Ref, - DefaultBranch: *event.Repo.DefaultBranch, - CloneURL: *event.Repo.CloneURL, + BaseRef: event.PullRequest.Base.GetRef(), + HeadRef: event.PullRequest.Head.GetRef(), + DefaultBranch: event.Repo.GetDefaultBranch(), + CloneURL: event.Repo.GetCloneURL(), FullName: event.Repo.GetFullName(), - Owner: *event.Repo.Owner.Login, + Owner: event.Repo.Owner.GetLogin(), Name: event.Repo.GetName(), - CheckID: *event.PullRequest.Number, - SHA: *event.PullRequest.Head.SHA, + CheckID: event.PullRequest.GetNumber(), + SHA: event.PullRequest.Head.GetSHA(), + Username: c.username, + Email: c.email, + Labels: labels, + + Config: c.cfg, + } +} + +// buildRepoFromComment builds a vcs.PullRequest from a github.IssueCommentEvent +func (c *Client) buildRepoFromComment(context context.Context, comment *github.IssueCommentEvent) vcs.PullRequest { + prURL, err := url.Parse(comment.Issue.GetURL()) + if (err != nil) { + log.Error().Msgf("failed to parse pr url: %+v", prURL) + return nilPr + } + pathSegments := strings.Split(prURL.Path, "/") + if len(pathSegments) < 5 { + // invalid path, return nilPr + log.Error().Msgf("invalid pr url path: %s", prURL.Path) + return nilPr + } + owner := pathSegments[2] + repo := pathSegments[3] + prNumber, err := strconv.Atoi(pathSegments[4]) + if err != nil { + log.Error().Msgf("failed to convert prNumber: %s", err) + return nilPr + } + pr, ghStatus, err := c.googleClient.PullRequests.Get(context, owner, repo, prNumber) + if err != nil || ghStatus.StatusCode < 200 || ghStatus.StatusCode >= 300 { + log.Error().Msgf("failed to get pull request: %s", err) + return nilPr + } + var labels []string + for _, label := range pr.Labels { + labels = append(labels, label.GetName()) + } + return vcs.PullRequest{ + BaseRef: pr.Base.GetRef(), + HeadRef: pr.Head.GetRef(), + DefaultBranch: comment.Repo.GetDefaultBranch(), + CloneURL: pr.Base.Repo.GetCloneURL(), + FullName: comment.Repo.GetFullName(), + Owner: pr.Base.Repo.Owner.GetLogin(), + Name: comment.Repo.GetName(), + CheckID: pr.GetNumber(), + SHA: pr.Head.GetSHA(), Username: c.username, Email: c.email, Labels: labels, @@ -260,7 +322,7 @@ func (c *Client) CreateHook(ctx context.Context, ownerAndRepoName, webhookUrl, w Secret: pkg.Pointer(webhookSecret), }, Events: []string{ - "pull_request", + "pull_request", "issue_comment", }, Name: pkg.Pointer("web"), }) ``` ### Feedback & Suggestions: 1. **Security**: - In `buildRepoFromComment`, the `strconv.Atoi` conversion should handle potential errors more gracefully. Instead of just logging the error, consider returning a more descriptive error message. - Ensure that the `webhookSecret` is handled securely and not logged or exposed in any way. 2. **Error Handling**: - In `buildRepoFromComment`, the error handling for `url.Parse` and `strconv.Atoi` could be improved by returning a more descriptive error message rather than just logging it and returning `nilPr`. - For the `github.PullRequests.Get` call, consider adding more context to the error message to make debugging easier. 3. **Performance**: - The `buildRepoFromComment` function makes multiple calls to `strings.Split`. This could be optimized by storing the result in a variable and reusing it. 4. **Code Readability**: - The `buildRepoFromComment` function could benefit from additional comments explaining the purpose of each step, especially the parsing of the URL and the extraction of path segments. - Consider renaming the `context` parameter in `buildRepoFromComment` to `ctx` to maintain consistency with the rest of the codebase. 5. **Consistency**: - Ensure that the naming conventions are consistent throughout the code. For example, `buildRepoFromComment` uses `context` as a parameter name, while other functions use `ctx`. 6. **Function Signature**: - The `ParseHook` function now takes an additional `context.Context` parameter. Ensure that all calls to this function are updated accordingly throughout the codebase. 7. **Logging**: - In `buildRepoFromComment`, the log messages could be more descriptive. For example, instead of just logging "failed to parse pr url", include the actual URL that failed to parse. --- By addressing these points, the code will be more secure, maintainable, and performant. Keep up the good work! 🚀 ## 😼 Mergecat review of pkg/vcs/gitlab_client/client.go ```diff @@ -9,7 +9,7 @@ import ( "strconv" "strings" - "github.com/chainguard-dev/git-urls" + giturls "github.com/chainguard-dev/git-urls" "github.com/pkg/errors" "github.com/rs/zerolog/log" "github.com/xanzy/go-gitlab" @@ -22,12 +22,21 @@ import ( const GitlabTokenHeader = "X-Gitlab-Token" type Client struct { - c *gitlab.Client + c *GLClient cfg config.ServerConfig username, email string } +type GLClient struct { + MergeRequests MergeRequestsServices + RepositoryFiles RepositoryFilesServices + Notes NotesServices + Pipelines PipelinesServices + Projects ProjectsServices + Commits CommitsServices +} + func CreateGitlabClient(cfg config.ServerConfig) (*Client, error) { // Initialize the GitLab client with access token gitlabToken := cfg.VcsToken @@ -54,7 +63,14 @@ func CreateGitlabClient(cfg config.ServerConfig) (*Client, error) { } client := &Client{ - c: c, + c: &GLClient{ + MergeRequests: &MergeRequestsService{c.MergeRequests}, + RepositoryFiles: &RepositoryFilesService{c.RepositoryFiles}, + Notes: &NotesService{c.Notes}, + Projects: &ProjectsService{c.Projects}, + Commits: &CommitsService{c.Commits}, + Pipelines: &PipelinesService{c.Pipelines}, + }, cfg: cfg, username: user.Username, email: user.Email, @@ -89,7 +105,7 @@ func (c *Client) VerifyHook(r *http.Request, secret string) ([]byte, error) { var nilPr vcs.PullRequest // ParseHook parses and validates a webhook event; return an err if this isn't valid -func (c *Client) ParseHook(r *http.Request, request []byte) (vcs.PullRequest, error) { +func (c *Client) ParseHook(ctx context.Context, r *http.Request, request []byte) (vcs.PullRequest, error) { eventRequest, err := gitlab.ParseHook(gitlab.HookEventType(r), request) if (err != nil) { return nilPr, err @@ -109,6 +125,20 @@ func (c *Client) ParseHook(r *http.Request, request []byte) (vcs.PullRequest, er log.Trace().Msgf("Unhandled Action %s", event.ObjectAttributes.Action) return nilPr, vcs.ErrInvalidType } + case *gitlab.MergeCommentEvent: + switch event.ObjectAttributes.Action { + case gitlab.CommentEventActionCreate: + if strings.ToLower(event.ObjectAttributes.Note) == c.cfg.ReplanCommentMessage { + log.Info().Msgf("Got %s comment, Running again", c.cfg.ReplanCommentMessage) + return c.buildRepoFromComment(event), nil + } else { + log.Info().Msg("ignoring Gitlab merge comment event due to non matching string") + return nilPr, vcs.ErrInvalidType + } + default: + log.Info().Msg("ignoring Gitlab issue comment event due to non matching string") + return nilPr, vcs.ErrInvalidType + } default: log.Trace().Msgf("Unhandled Event: %T", event) return nilPr, vcs.ErrInvalidType @@ -145,6 +175,10 @@ func (c *Client) GetHookByUrl(ctx context.Context, repoName, webhookUrl string) if hook.MergeRequestsEvents { events = append(events, string(gitlab.MergeRequestEventTargetType)) } + if hook.NoteEvents { + events = append(events, string(gitlab.NoteEventTargetType)) + } + return &vcs.WebHookConfig{ Url: hook.URL, Events: events, @@ -161,13 +195,14 @@ func (c *Client) CreateHook(ctx context.Context, repoName, webhookUrl, webhookSe return errors.Wrap(err, "failed to parse repo name") } - _, _, err = c.c.Projects.AddProjectHook(pid, &gitlab.AddProjectHookOptions{ + _, glStatus, err := c.c.Projects.AddProjectHook(pid, &gitlab.AddProjectHookOptions{ URL: pkg.Pointer(webhookUrl), MergeRequestsEvents: pkg.Pointer(true), + NoteEvents: pkg.Pointer(true), Token: pkg.Pointer(webhookSecret), }) - if err != nil { + if err != nil && glStatus.StatusCode < http.StatusOK || glStatus.StatusCode >= http.StatusMultipleChoices { return errors.Wrap(err, "failed to create project webhook") } @@ -240,3 +275,26 @@ func (c *Client) buildRepoFromEvent(event *gitlab.MergeEvent) vcs.PullRequest { Config: c.cfg, } } + +func (c *Client) buildRepoFromComment(event *gitlab.MergeCommentEvent) vcs.PullRequest { + // Convert all labels from this MR to a string array of label names + var labels []string + for _, label := range event.MergeRequest.Labels { + labels = append(labels, label.Title) + } + return vcs.PullRequest{ + BaseRef: event.MergeRequest.TargetBranch, + HeadRef: event.MergeRequest.SourceBranch, + DefaultBranch: event.Project.DefaultBranch, + FullName: event.Project.PathWithNamespace, + CloneURL: event.Project.GitHTTPURL, + Name: event.Project.Name, + CheckID: event.MergeRequest.IID, + SHA: event.MergeRequest.LastCommit.ID, + Username: c.username, + Email: c.email, + Labels: labels, + + Config: c.cfg, + } +} ``` Feedback & Suggestions: 1. **Security Improvement**: - In `CreateHook`, the condition `if err != nil && glStatus.StatusCode < http.StatusOK || glStatus.StatusCode >= http.StatusMultipleChoices` should be corrected to `if err != nil || glStatus.StatusCode < http.StatusOK || glStatus.StatusCode >= http.StatusMultipleChoices` to ensure proper error handling. - **Suggested Change**: ```go if err != nil || glStatus.StatusCode < http.StatusOK || glStatus.StatusCode >= http.StatusMultipleChoices { return errors.Wrap(err, "failed to create project webhook") } ``` 2. **Performance Improvement**: - In `buildRepoFromComment`, the labels are being appended in a loop. Consider pre-allocating the slice with the known length to avoid multiple allocations. - **Suggested Change**: ```go labels := make([]string, len(event.MergeRequest.Labels)) for i, label := range event.MergeRequest.Labels { labels[i] = label.Title } ``` 3. **Code Consistency**: - The `ParseHook` function signature was changed to include `context.Context`, but the `VerifyHook` function still does not include it. For consistency and future-proofing, consider adding `context.Context` to `VerifyHook` as well. - **Suggested Change**: ```go func (c *Client) VerifyHook(ctx context.Context, r *http.Request, secret string) ([]byte, error) { ``` 4. **Error Handling**: - In `CreateGitlabClient`, the error handling for `gitlab.NewClient` uses `log.Fatal()`, which will terminate the program. Consider returning the error instead to allow the caller to handle it. - **Suggested Change**: ```go c, err := gitlab.NewClient(gitlabToken, gitlabOptions...) if err != nil { return nil, errors.Wrap(err, "could not create Gitlab client") } ``` 5. **Logging**: - In `ParseHook`, the log messages for ignored events could be more descriptive to aid debugging. - **Suggested Change**: ```go log.Info().Msgf("Ignoring Gitlab merge comment event due to non-matching string: %s", event.ObjectAttributes.Note) log.Info().Msgf("Ignoring Gitlab issue comment event due to non-matching string: %s", event.ObjectAttributes.Note) ``` --- ## 😼 Mergecat review of pkg/vcs/gitlab_client/client_test.go ```diff @@ -1,10 +1,18 @@ package gitlab_client import ( + "context" + "fmt" + "net/http" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/xanzy/go-gitlab" + gitlabMocks "github.com/zapier/kubechecks/mocks/gitlab_client/mocks" + "github.com/zapier/kubechecks/pkg/config" + "github.com/zapier/kubechecks/pkg/vcs" ) func TestCustomGitURLParsing(t *testing.T) { @@ -40,3 +48,184 @@ func TestCustomGitURLParsing(t *testing.T) { }) } } + +func TestClient_GetHookByUrl(t *testing.T) { + type fields struct { + c *GLClient + cfg config.ServerConfig + username string + email string + } + type args struct { + ctx context.Context + repoName string + webhookUrl string + } + tests := []struct { + name string + fields fields + args args + want *vcs.WebHookConfig + wantErr assert.ErrorAssertionFunc + }{ + { + name: "normal ok", + fields: fields{ + c: MockGitLabProjects("ListProjectHooks", + []interface{}{ + []*gitlab.ProjectHook{ + { + URL: "https://dummywebhooks.local", + MergeRequestsEvents: true, + NoteEvents: true, + }, + }, + &gitlab.Response{ + Response: &http.Response{StatusCode: http.StatusOK}, + }, + nil, + }), + cfg: config.ServerConfig{}, + username: "", + email: "", + }, + args: args{ + ctx: context.TODO(), + repoName: "test", + webhookUrl: "https://dummywebhooks.local", + }, + want: &vcs.WebHookConfig{ + Url: "https://dummywebhooks.local", + Events: []string{"merge_request", "note"}, + }, + wantErr: assert.NoError, + }, + { + name: "gl client err", + fields: fields{ + c: MockGitLabProjects("ListProjectHooks", + []interface{}{ + nil, + &gitlab.Response{ + Response: &http.Response{StatusCode: http.StatusInternalServerError}, + }, + fmt.Errorf("dummy error"), + }), + cfg: config.ServerConfig{}, + username: "", + email: "", + }, + args: args{ + ctx: context.TODO(), + repoName: "test", + webhookUrl: "https://dummywebhooks.local", + }, + want: nil, + wantErr: assert.Error, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Client{ + c: tt.fields.c, + cfg: tt.fields.cfg, + username: tt.fields.username, + email: tt.fields.email, + } + got, err := c.GetHookByUrl(tt.args.ctx, tt.args.repoName, tt.args.webhookUrl) + if !tt.wantErr(t, err, fmt.Sprintf("GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.repoName, tt.args.webhookUrl)) { + return + } + assert.Equalf(t, tt.want, got, "GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.repoName, tt.args.webhookUrl) + }) + } +} + +// MockGitLabProjects is a generic function to mock Gitlab MergeRequest client methods +func MockGitLabProjects(methodName string, returns []interface{}) *GLClient { + mockClient := new(gitlabMocks.MockProjectsServices) + mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...) + + return &GLClient{ + Projects: mockClient, + } +} + +func TestClient_CreateHook(t *testing.T) { + type fields struct { + c *GLClient + cfg config.ServerConfig + username string + email string + } + type args struct { + ctx context.Context + repoName string + webhookUrl string + webhookSecret string + } + tests := []struct { + name string + fields fields + args args + wantErr assert.ErrorAssertionFunc + }{ + { + name: "normal ok", + fields: fields{ + c: MockGitLabProjects("AddProjectHook", + []interface{}{ + &gitlab.ProjectHook{ + URL: "https://dummywebhooks.local", + MergeRequestsEvents: true, + NoteEvents: true, + }, + &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil, + }), + cfg: config.ServerConfig{}, + username: "", + email: "", + }, + args: args{ + ctx: context.TODO(), + repoName: "main", + webhookUrl: "https://dummywebhooks.local", + webhookSecret: "", + }, + wantErr: assert.NoError, + }, + { + name: "gitlab error", + fields: fields{ + c: MockGitLabProjects("AddProjectHook", + []interface{}{ + nil, + &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, + fmt.Errorf("dummy error"), + }), + cfg: config.ServerConfig{}, + username: "", + email: "", + }, + args: args{ + ctx: context.TODO(), + repoName: "main", + webhookUrl: "https://dummywebhooks.local", + webhookSecret: "", + }, + wantErr: assert.Error, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Client{ + c: tt.fields.c, + cfg: tt.fields.cfg, + username: tt.fields.username, + email: tt.fields.email, + } + tt.wantErr(t, c.CreateHook(tt.args.ctx, tt.args.repoName, tt.args.webhookUrl, tt.args.webhookSecret), fmt.Sprintf("CreateHook(%v, %v, %v, %v)", tt.args.ctx, tt.args.repoName, tt.args.webhookUrl, tt.args.webhookSecret)) + }) + } +} ``` Feedback & Suggestions: 1. **Security**: Ensure that sensitive information such as `webhookSecret` is handled securely. Consider using environment variables or a secure vault for storing secrets. ```go webhookSecret := os.Getenv("WEBHOOK_SECRET") ``` 2. **Error Handling**: The error messages could be more descriptive to aid in debugging. Instead of just returning `fmt.Errorf("dummy error")`, provide more context. ```go fmt.Errorf("failed to list project hooks: %w", err) ``` 3. **Mocking**: The `MockGitLabProjects` function could be more flexible by allowing different methods to be mocked without hardcoding the method name. ```go func MockGitLabProjects(methodName string, returns ...interface{}) *GLClient { mockClient := new(gitlabMocks.MockProjectsServices) mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...) return &GLClient{Projects: mockClient} } ``` 4. **Test Coverage**: Ensure that edge cases are covered in your tests. For example, test scenarios where the `webhookUrl` is invalid or the `repoName` does not exist. 5. **Code Duplication**: The `fields` and `args` structs are repeated in both `TestClient_GetHookByUrl` and `TestClient_CreateHook`. Consider refactoring to reduce duplication. ```go type commonFields struct { c *GLClient cfg config.ServerConfig username string email string } ``` 6. **Context Handling**: Use context with a timeout to avoid potential hanging tests. ```go ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() ``` By addressing these points, the code will be more secure, maintainable, and robust. 🛡️💡 --- ## 😼 Mergecat review of pkg/vcs/github_client/client_test.go ```diff @@ -25,6 +25,16 @@ func MockGitHubMethod(methodName string, returns []interface{}) *GClient { } } +// MockGitHubPullRequestMethod is a generic function to mock GitHub client methods +func MockGitHubPullRequestMethod(methodName string, returns []interface{}) *GClient { + mockClient := new(githubMocks.MockPullRequestsServices) + mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...) + + return &GClient{ + PullRequests: mockClient, + } +} + func TestParseRepo(t *testing.T) { testcases := []struct { name, input string @@ -324,3 +334,235 @@ func TestClient_GetHookByUrl(t *testing.T) { }) } } + +func TestClient_buildRepoFromComment(t *testing.T) { + type fields struct { + shurcoolClient *githubv4.Client + googleClient *GClient + cfg config.ServerConfig + username string + email string + } + type args struct { + context context.Context + comment *github.IssueCommentEvent + } + tests := []struct { + name string + fields fields + args args + want vcs.PullRequest + }{ + { + name: "normal ok", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubPullRequestMethod("Get", + []interface{}{ + &github.PullRequest{ + ID: nil, + Number: github.Int(123), + Labels: []*github.Label{ + { + Name: github.String("test label1"), + }, + { + Name: github.String("test label2"), + }, + }, + Head: &github.PullRequestBranch{ + Ref: github.String("main"), + SHA: github.String("dummySHA"), + }, + Base: &github.PullRequestBranch{ + Ref: github.String("main"), + SHA: github.String("dummySHA"), + Repo: &github.Repository{ + CloneURL: github.String("https://github.com/zapier/kubechecks/"), + Owner: &github.User{Login: github.String("zapier")}, + }, + }, + }, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}, + ), + cfg: config.ServerConfig{}, + username: "unittestuser", + email: "unitestuser@localhost.local", + }, + args: args{ + context: context.TODO(), + comment: &github.IssueCommentEvent{ + Issue: &github.Issue{ + URL: github.String("https://github.com/zapier/kubechecks/pull/250"), + }, + Repo: &github.Repository{ + DefaultBranch: github.String("main"), + Name: github.String("prguy"), + FullName: github.String("pr guy"), + }, + }, + }, + want: vcs.PullRequest{ + BaseRef: "main", + HeadRef: "main", + DefaultBranch: "main", + CloneURL: "https://github.com/zapier/kubechecks/", + Name: "prguy", + Owner: "zapier", + CheckID: 123, + SHA: "dummySHA", + FullName: "pr guy", + Username: "unittestuser", + Email: "unitestuser@localhost.local", + Labels: []string{"test label1", "test label2"}, + Config: config.ServerConfig{}, + }, + }, + { + name: "failed to get pr data", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubPullRequestMethod("Get", + []interface{}{ + nil, + &github.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, + fmt.Errorf("dummy error")}, + ), + cfg: config.ServerConfig{}, + username: "unittestuser", + email: "unitestuser@localhost.local", + }, + args: args{ + context: context.TODO(), + comment: &github.IssueCommentEvent{ + Issue: &github.Issue{ + URL: github.String("https://github.com/zapier/kubechecks/pull/250"), + }, + Repo: &github.Repository{ + DefaultBranch: github.String("main"), + Name: github.String("prguy"), + FullName: github.String("pr guy"), + }, + }, + }, + want: nilPr, + }, + { + name: "missing issue pr URL", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubPullRequestMethod("Get", + []interface{}{ + &github.PullRequest{ + ID: nil, + Number: github.Int(123), + Labels: []*github.Label{ + { + Name: github.String("test label1"), + }, + { + Name: github.String("test label2"), + }, + }, + Head: &github.PullRequestBranch{ + Ref: github.String("main"), + SHA: github.String("dummySHA"), + }, + Base: &github.PullRequestBranch{ + Ref: github.String("main"), + SHA: github.String("dummySHA"), + Repo: &github.Repository{ + CloneURL: github.String("https://github.com/zapier/kubechecks/"), + Owner: &github.User{Login: github.String("zapier")}, + }, + }, + }, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}, + ), + cfg: config.ServerConfig{}, + username: "unittestuser", + email: "unitestuser@localhost.local", + }, + args: args{ + context: context.TODO(), + comment: &github.IssueCommentEvent{ + Issue: &github.Issue{ + URL: nil, + }, + Repo: &github.Repository{ + DefaultBranch: github.String("main"), + Name: github.String("prguy"), + FullName: github.String("pr guy"), + }, + }, + }, + want: nilPr, + }, + { + name: "invalid pr url", + fields: fields{ + shurcoolClient: nil, + googleClient: MockGitHubPullRequestMethod("Get", + []interface{}{ + &github.PullRequest{ + ID: nil, + Number: github.Int(123), + Labels: []*github.Label{ + { + Name: github.String("test label1"), + }, + { + Name: github.String("test label2"), + }, + }, + Head: &github.PullRequestBranch{ + Ref: github.String("main"), + SHA: github.String("dummySHA"), + }, + Base: &github.PullRequestBranch{ + Ref: github.String("main"), + SHA: github.String("dummySHA"), + Repo: &github.Repository{ + CloneURL: github.String("https://github.com/zapier/kubechecks/"), + Owner: &github.User{Login: github.String("zapier")}, + }, + }, + }, + &github.Response{Response: &http.Response{StatusCode: http.StatusOK}}, + nil}, + ), + cfg: config.ServerConfig{}, + username: "unittestuser", + email: "unitestuser@localhost.local", + }, + args: args{ + context: context.TODO(), + comment: &github.IssueCommentEvent{ + Issue: &github.Issue{ + URL: github.String("https://github.com/"), + }, + Repo: &github.Repository{ + DefaultBranch: github.String("main"), + Name: github.String("prguy"), + FullName: github.String("pr guy"), + }, + }, + }, + want: nilPr, + }, + } + 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, + } + assert.Equalf(t, tt.want, c.buildRepoFromComment(tt.args.context, tt.args.comment), "buildRepoFromComment(%v, %v)", tt.args.context, tt.args.comment) + }) + } +} ``` Feedback & Suggestions: 1. **Security**: The `MockGitHubPullRequestMethod` function does not sanitize or validate the inputs. Ensure that the inputs are properly validated to avoid potential security issues. 2. **Error Handling**: In the `TestClient_buildRepoFromComment` function, the test cases for "failed to get pr data" and "missing issue pr URL" return `nilPr`. Ensure that `nilPr` is properly defined and handled to avoid potential runtime errors. 3. **Code Duplication**: The `MockGitHubPullRequestMethod` function is very similar to the `MockGitHubMethod` function. Consider refactoring to reduce code duplication. For example, you could create a generic mock method that accepts the service type as a parameter. 4. **Context Naming**: In the `TestClient_buildRepoFromComment` function, the `context` field in the `args` struct should be renamed to `ctx` to follow Go naming conventions. 5. **Test Coverage**: Ensure that all edge cases are covered in the tests. For example, test cases where the `comment` or `Repo` fields are `nil` should be included. 6. **Documentation**: Add comments to explain the purpose of the `TestClient_buildRepoFromComment` function and its test cases. This will help future developers understand the intent of the tests. ---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[KyriosGN0]

@Greyeye should be fixed