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]

Is this the right phrasing? A lot of similar tools would do something like /kubechecks retry or something like that. Alternately, we could make it configurable. Still need a good default, but that removes some pressure off of getting it perfectly right.

[KyriosGN0]

honestly im coming from atlantis where do atlantis apply/run apply but i agree that making it configureable is a good suggestion, i'll update the PR this week and tag you

[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.

[KyriosGN0]

@Greyeye should be fixed

[KyriosGN0]

@Greyeye it seems like same issue, can this PR be merged or is this blocking ? (because its not even something in the code itself)

[KyriosGN0]

@Greyeye any updates?

[djeebus]

Don't worry about the failing push, it should be (might be?) sorted out if you pull main, which you'll need to do to resolve the merge conflicts anyway. I added one comment about the string parsing, but other wise looks good! Mind sorting that out, and we can get this merged?

[KyriosGN0]

@djeebus fixed conflicts and commited your suggestion

[KyriosGN0]

@djeebus fixed test failures, the change which you proposed caused test to fails as those methods don't error but rather return 0

[djeebus]

Thanks! It looks like those failed tests uncovered a broken test, and a legit bug. I've fixed them here, if you want to grab them: KyriosGN0/kubechecks@replan...djeebus:kubechecks:fix-replan-code-tests

[KyriosGN0]

@djeebus took your suggestion and also add case for repo/owner is empty string

[KyriosGN0]

@djeebus any updates here ?

[KyriosGN0]

hey @djeebus @Greyeye any possible news about merging this? will really love to have this feature

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/server/hook_handler.go

@@ -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: 👍 Good job on adding the ctx parameter to the ParseHook function call. This ensures that the context is properly propagated, which is important for tracing and cancellation.

2. Log Message Clarity: The change from "not a merge request" to "not a supported request" makes the log message more generic. Ensure that this change aligns with the broader context of what types of requests are expected and handled. If "merge request" was the only unsupported type, the original message might have been more informative.

3. Error Handling: The comment // TODO: do something ELSE with the error suggests that error handling could be improved. Consider implementing a more robust error handling strategy, such as logging the error with more context or returning a more specific HTTP status code.

---

😼 Mergecat review of pkg/vcs/gitlab_client/status.go

+
+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. Naming Consistency: The interface is named CommitsServices, which is plural, while the struct is named CommitsService, which is singular. For clarity and consistency, consider using singular or plural for both. For example, CommitService and CommitServiceImpl or CommitServices and CommitServicesImpl.

2. Interface Design: The SetCommitStatus method in the CommitsServices interface uses interface{} for the pid parameter. This can lead to runtime errors if the wrong type is passed. If possible, specify a more concrete type to improve type safety.

3. Redundancy: The CommitsService struct embeds the CommitsServices interface. This pattern is typically used for mocking or testing purposes. If this is the intention, consider adding comments to clarify the purpose of this design choice. If not, evaluate if this embedding is necessary.

4. Documentation: Adding comments to describe the purpose and usage of the CommitsServices interface and CommitsService struct would improve code readability and maintainability.

---

😼 Mergecat review of cmd/root.go

@@ -115,6 +115,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 Consideration: Ensure that the replan-comment-msg value is sanitized before being used in any context that could be vulnerable to injection attacks. This is especially important if the value can be influenced by external input.

2. Documentation: Consider adding a comment or documentation explaining the purpose of the replan-comment-msg flag. This will help future developers understand its use case and importance.

3. Consistency: Verify that the new flag replan-comment-msg is being used correctly in the rest of the codebase. Ensure that any logic that depends on this flag is properly implemented and tested.

4. Testing: Add unit tests to ensure that the default value and any custom values for replan-comment-msg are handled correctly. This will help catch any potential issues early.

5. Naming: The name replan-comment-msg is descriptive, but consider if it aligns with the naming conventions used for other flags. Consistent naming can improve readability and maintainability.

---

😼 Mergecat review of pkg/config/config.go

@@ -78,6 +78,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 seems to have a typo. It should likely be MaxConcurrentChecks. Correcting this will improve code readability and prevent potential confusion. 📝

2. Security Consideration: If ReplanCommentMessage is user-generated or can be influenced by external input, ensure that it is properly sanitized to prevent injection attacks or other security vulnerabilities. 🔒

3. Documentation: Consider adding a comment above the new ReplanCommentMessage field to describe its purpose and usage. This will help future developers understand its role in the configuration. 📚

---

😼 Mergecat review of go.mod

@@ -41,7 +41,7 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/spf13/viper v1.19.0
 	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

Feedback & Suggestions:

1. Version Update Verification: Ensure that the update from v0.105.0 to v0.107.0 for github.com/xanzy/go-gitlab is compatible with your codebase. Check the release notes or changelog for any breaking changes or new features that might affect your application.

2. Testing: After updating the dependency, run your test suite to verify that everything works as expected. This will help catch any issues introduced by the new version.

3. Indirect Dependencies: If github.com/xanzy/go-gitlab is used indirectly through another package, ensure that the update does not conflict with the expected version of that package.

4. Security: Check if the new version addresses any security vulnerabilities that might have been present in the previous version. This is a good practice to ensure the security of your application.

---

😼 Mergecat review of .github/workflows/on_pull-request_closed.yaml

@@ -12,6 +12,7 @@ env:
 jobs:
   remove-temp-image:
     runs-on: ubuntu-22.04
+    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:

• Error Handling: Adding continue-on-error: true can be useful to ensure that the workflow continues even if a step fails. However, be cautious with this setting as it might mask underlying issues. Consider logging errors or adding notifications to ensure any failures are still visible and can be addressed. 📋

• Documentation: It might be helpful to add a comment explaining why continue-on-error: true is necessary in this context. This will aid future maintainers in understanding the rationale behind this decision. 📝

---

😼 Mergecat review of pkg/vcs/gitlab_client/pipeline.go

@@ -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 is named PipelinesServices, which is plural, while the struct is named PipelinesService, which is singular. For clarity and consistency, consider using singular or plural for both, depending on the intended use.

2. Interface Embedding: The PipelinesService struct embeds the PipelinesServices interface. If the intention is to implement the interface, ensure that the struct provides concrete implementations for the methods defined in the interface. Otherwise, this embedding might not be necessary.

3. Interface Design: The pid parameter in the ListProjectPipelines method is of type interface{}. If possible, specify a more concrete type to improve type safety and clarity.

4. Documentation: Consider adding comments to describe the purpose and usage of the PipelinesServices interface and PipelinesService struct. This will help other developers understand the design and intent behind these types.

5. Unused Code: If the newly added types are not yet used in the code, ensure that they are integrated into the existing logic or remove them until they are needed to avoid confusion.

---

😼 Mergecat review of docs/usage.md

@@ -64,6 +64,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 starts with a lowercase letter, while other descriptions start with an uppercase letter. For consistency, consider changing "comment" to "Comment".

2. Clarification: The description "comment message which re-triggers kubechecks on PR" could be more explicit. Consider rephrasing to "The comment message that, when posted on a PR, will re-trigger kubechecks."

3. Security Consideration: If this environment variable is user-configurable, ensure that there are no security implications, such as command injection, when processing the comment message.

---

😼 Mergecat review of pkg/vcs/gitlab_client/merge.go

+
+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 interface MergeRequestsServices could be more intuitively named as MergeRequestService to better reflect its purpose and align with Go naming conventions. 🏷️

2. Interface Methods: Consider documenting the methods in the MergeRequestsServices interface to clarify their purpose and expected behavior. This will improve code readability and maintainability. 📚

3. Type Safety: The use of interface{} for pid in the interface methods can lead to runtime errors if the wrong type is passed. If possible, specify a more concrete type to enhance type safety. 🔒

4. Redundancy: The MergeRequestsService struct embeds the MergeRequestsServices interface. If there is no additional functionality or fields in MergeRequestsService, consider whether this struct is necessary. If it is intended for future expansion, a comment indicating this could be helpful. 🔄

---

😼 Mergecat review of pkg/vcs/gitlab_client/message.go

+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: The interface NotesServices could be renamed to NotesService to follow Go naming conventions, where interfaces are typically named in singular form to represent a single service or capability. This will also align with the struct name NotesService.

2. Interface Segregation: Consider splitting the NotesServices interface into smaller interfaces if not all methods are used together. This follows the Interface Segregation Principle, making it easier to implement and test.

3. Documentation: Adding comments to the interface and struct would be beneficial for understanding their purpose and usage. This is especially helpful for new developers or contributors to the codebase.

4. Redundancy: The NotesService struct embedding NotesServices interface seems redundant unless you plan to add additional fields or methods to NotesService. If not, consider using the interface directly.

5. Type Safety: Consider using more specific types instead of interface{} for pid if possible. This will improve type safety and reduce potential runtime errors.

---

😼 Mergecat review of pkg/vcs/types.go

-	// 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)

Feedback & Suggestions:

1. Context Usage: 👍 Adding context.Context to the ParseHook function is a good improvement for handling request-scoped values, such as deadlines and cancellation signals. Ensure that the context is used effectively within the implementation to handle these aspects.

2. Documentation Update: 📚 The comment was updated to reflect the change, which is great. Make sure that the implementation of ParseHook also utilizes the context appropriately, and consider adding examples or further documentation if necessary to guide future developers on how to use this context.

3. Backward Compatibility: 🔄 If this interface is implemented by multiple types, ensure that all implementations are updated to handle the new parameter. This change could break existing code if not all implementations are updated accordingly.

---

😼 Mergecat review of .mockery.yaml

@@ -5,11 +5,7 @@ packages:
     # place your package-specific config here
     config:
       all: true
-  github.com/zapier/kubechecks/pkg/generator:
-    # place your package-specific config here
-    config:
-      all: true
-  github.com/zapier/kubechecks/pkg/affected_apps:
+  github.com/zapier/kubechecks/pkg/vcs/gitlab_client:
     # place your package-specific config here
     config:
       all: true

Feedback & Suggestions:

1. Documentation Update: 📚 It seems like the github.com/zapier/kubechecks/pkg/generator and github.com/zapier/kubechecks/pkg/affected_apps packages have been removed and replaced with github.com/zapier/kubechecks/pkg/vcs/gitlab_client. Ensure that any documentation or comments related to these changes are updated to reflect the new package structure.

2. Verification: 🔍 Double-check that the removal of the generator and affected_apps packages is intentional and that they are no longer needed. If they are still required elsewhere, consider if they should be included in another configuration.

3. Testing: 🧪 After making these changes, ensure that you run tests to verify that the new configuration works as expected and that no functionality relying on the removed packages is broken.

---

😼 Mergecat review of pkg/vcs/gitlab_client/project.go

+
+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: The interface names ProjectsServices and RepositoryFilesServices could be misleading as they suggest plural services. Consider renaming them to ProjectService and RepositoryFileService to better reflect their purpose. 🏷️

2. Interface Composition: The ProjectsService and RepositoryFilesService structs embed their respective interfaces. This pattern is useful for mocking in tests, but ensure that this is the intended use case. If not, consider whether embedding is necessary. 🤔

3. Interface Method Parameters: The use of interface{} for pid in the interface methods can lead to runtime errors if the wrong type is passed. If possible, specify a more concrete type to improve type safety. 🔍

4. Documentation: Adding comments to describe the purpose and usage of these interfaces and structs would improve code readability and maintainability. 📚

5. Security Consideration: Ensure that any user input passed to these methods is properly validated and sanitized to prevent injection attacks or other vulnerabilities. 🔒

---

😼 Mergecat review of pkg/vcs/github_client/client.go

@@ -150,7 +150,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
@@ -166,28 +166,44 @@ 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)
+			} 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 due to invalid action")
+			return nilPr, vcs.ErrInvalidType
+		}
 	default:
 		log.Error().Msg("invalid event provided to Github client")
 		return nilPr, vcs.ErrInvalidType
 	}
 }
 
-func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) vcs.PullRequest {
+func (c *Client) buildRepo(pullRequest *github.PullRequest) vcs.PullRequest {
+	repo := pullRequest.Head.Repo
+
 	var labels []string
-	for _, label := range event.PullRequest.Labels {
+	for _, label := range pullRequest.Labels {
 		labels = append(labels, label.GetName())
 	}
 
 	return vcs.PullRequest{
-		BaseRef:       *event.PullRequest.Base.Ref,
-		HeadRef:       *event.PullRequest.Head.Ref,
-		DefaultBranch: *event.Repo.DefaultBranch,
-		CloneURL:      *event.Repo.CloneURL,
-		FullName:      event.Repo.GetFullName(),
-		Owner:         *event.Repo.Owner.Login,
-		Name:          event.Repo.GetName(),
-		CheckID:       *event.PullRequest.Number,
-		SHA:           *event.PullRequest.Head.SHA,
+		BaseRef:       pullRequest.Base.GetRef(),
+		HeadRef:       pullRequest.Head.GetRef(),
+		DefaultBranch: repo.GetDefaultBranch(),
+		CloneURL:      repo.GetCloneURL(),
+		FullName:      repo.GetFullName(),
+		Owner:         repo.Owner.GetLogin(),
+		Name:          repo.GetName(),
+		CheckID:       pullRequest.GetNumber(),
+		SHA:           pullRequest.Head.GetSHA(),
 		Username:      c.username,
 		Email:         c.email,
 		Labels:        labels,
@@ -196,6 +212,25 @@ func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) vcs.PullRequ
 	}
 }
 
+func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) vcs.PullRequest {
+	return c.buildRepo(event.PullRequest)
+}
+
+// buildRepoFromComment builds a vcs.PullRequest from a github.IssueCommentEvent
+func (c *Client) buildRepoFromComment(context context.Context, comment *github.IssueCommentEvent) (vcs.PullRequest, error) {
+	owner := comment.GetRepo().GetOwner().GetLogin()
+	repoName := comment.GetRepo().GetName()
+	prNumber := comment.GetIssue().GetNumber()
+
+	log.Info().Str("owner", owner).Str("repo", repoName).Int("number", prNumber).Msg("getting pr")
+	pr, _, err := c.googleClient.PullRequests.Get(context, owner, repoName, prNumber)
+	if err != nil {
+		return nilPr, errors.Wrap(err, "failed to get pull request")
+	}
+
+	return c.buildRepo(pr), nil
+}
+
 func toGithubCommitStatus(state pkg.CommitState) *string {
 	switch state {
 	case pkg.StateError, pkg.StatePanic:
@@ -283,7 +318,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 Consideration: When handling GitHub webhooks, ensure that the payload is verified using a secret to prevent unauthorized requests. The VerifyHook function already checks for a secret, but make sure this is consistently used wherever webhooks are processed.

2. Error Handling: In buildRepoFromComment, consider logging the error with more context if the pull request retrieval fails. This can help in debugging issues related to API rate limits or permission errors.

3. Performance: The buildRepoFromComment function makes an additional API call to fetch the pull request details. If possible, try to minimize API calls by using available data from the event payload, especially if the event already contains the necessary information.

4. Code Clarity: The buildRepo function is a good refactor for reusability. Ensure that the naming of functions like buildRepoFromEvent and buildRepoFromComment clearly indicates their purpose and the type of event they handle.

5. Logging: Ensure that sensitive information is not logged. The current logging statements seem safe, but always double-check that no sensitive data (like tokens or secrets) is inadvertently logged.

6. Context Usage: The addition of context.Context to ParseHook and buildRepoFromComment is a good practice for managing request-scoped values and deadlines. Ensure that this context is propagated correctly throughout the call chain.

---

😼 Mergecat review of pkg/vcs/github_client/client_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/shurcooL/githubv4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	githubMocks "github.com/zapier/kubechecks/mocks/github_client/mocks"
 	"github.com/zapier/kubechecks/pkg/config"
 	"github.com/zapier/kubechecks/pkg/vcs"
@@ -25,6 +26,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 +335,130 @@ func TestClient_GetHookByUrl(t *testing.T) {
 		})
 	}
 }
+
+func TestClient_buildRepoFromComment_HappyPath(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("new-feature"),
+								SHA: github.String("dummySHAHead"),
+								Repo: &github.Repository{
+									CloneURL:      github.String("https://github.com/zapier/kubechecks/"),
+									DefaultBranch: github.String("main"),
+									FullName:      github.String("zapier/kubechecks"),
+									Owner:         &github.User{Login: github.String("fork")},
+									Name:          github.String("kubechecks"),
+								},
+							},
+							Base: &github.PullRequestBranch{
+								Ref: github.String("main"),
+								SHA: github.String("dummySHABase"),
+								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:    "[email protected]",
+			},
+			args: args{
+				context: context.TODO(),
+				comment: &github.IssueCommentEvent{
+					Issue: &github.Issue{
+						URL:    github.String("https://github.com/zapier/kubechecks/pull/250"),
+						Number: github.Int(250),
+						Repository: &github.Repository{
+							Name: github.String("kubechecks"),
+							Owner: &github.User{
+								Name: github.String("zapier"),
+							},
+						},
+					},
+					Repo: &github.Repository{
+						DefaultBranch: github.String("main"),
+						Name:          github.String("kubechecks"),
+						FullName:      github.String("zapier/kubechecks"),
+					},
+				},
+			},
+			want: vcs.PullRequest{
+				BaseRef:       "main",
+				HeadRef:       "new-feature",
+				DefaultBranch: "main",
+				CloneURL:      "https://github.com/zapier/kubechecks/",
+				Name:          "kubechecks",
+				Owner:         "fork",
+				CheckID:       123,
+				SHA:           "dummySHAHead",
+				FullName:      "zapier/kubechecks",
+				Username:      "unittestuser",
+				Email:         "[email protected]",
+				Labels:        []string{"test label1", "test label2"},
+				Config:        config.ServerConfig{},
+			},
+		},
+	}
+	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,
+			}
+			actual, err := c.buildRepoFromComment(tt.args.context, tt.args.comment)
+			require.NoError(t, err)
+			assert.Equal(t, tt.want.Name, actual.Name)
+			assert.Equal(t, tt.want.Labels, actual.Labels)
+			assert.Equal(t, tt.want.CheckID, actual.CheckID)
+			assert.Equal(t, tt.want.BaseRef, actual.BaseRef)
+			assert.Equal(t, tt.want.CloneURL, actual.CloneURL)
+			assert.Equal(t, tt.want.DefaultBranch, actual.DefaultBranch)
+			assert.Equal(t, tt.want.Email, actual.Email)
+			assert.Equal(t, tt.want.FullName, actual.FullName)
+			assert.Equal(t, tt.want.HeadRef, actual.HeadRef)
+			assert.Equal(t, tt.want.Owner, actual.Owner)
+			assert.Equal(t, tt.want.Remote, actual.Remote)
+			assert.Equal(t, tt.want.SHA, actual.SHA)
+			assert.Equal(t, tt.want.Username, actual.Username)
+		})
+	}
+}

Feedback & Suggestions:

1. Use of require Package: The addition of the require package is a good choice for ensuring that the test fails immediately if an error is encountered. This is useful for tests where subsequent assertions depend on the success of previous ones. 👍

2. Mocking Pull Request Methods: The introduction of MockGitHubPullRequestMethod is a useful addition for testing pull request-related functionalities. However, ensure that the mock expectations are correctly set and cover all necessary scenarios. 🛠️

3. Test Coverage: The new test TestClient_buildRepoFromComment_HappyPath is a good start for testing the buildRepoFromComment function. Consider adding more test cases to cover edge cases and potential error scenarios, such as invalid input or unexpected API responses. 📈

4. Error Handling: In the TestClient_buildRepoFromComment_HappyPath, while require.NoError is used, it might be beneficial to also test for specific error messages or types in negative test cases to ensure robustness. 🛡️

5. Code Duplication: The structure of the test cases is quite similar across different tests. Consider refactoring common setup code into helper functions to reduce duplication and improve maintainability. 🔄

6. Variable Naming: Ensure consistent naming conventions for variables and functions to maintain readability and clarity. For instance, unitestuser should be unittestuser for consistency. 📝

Overall, the changes are well-structured and enhance the test suite. Keep up the good work! 🚀

---

😼 Mergecat review of pkg/vcs/gitlab_client/client_test.go

@@ -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 Consideration: The webhookSecret is currently being passed as an empty string in the test cases. Ensure that in actual implementation, this secret is securely handled and not left empty to prevent unauthorized access. 🔒

2. Error Handling: In the TestClient_GetHookByUrl and TestClient_CreateHook functions, consider adding more detailed error messages in the fmt.Errorf calls to provide better context when errors occur. This will aid in debugging. 🐛

3. Mocking Improvements: The MockGitLabProjects function uses mock.Anything for all parameters. If possible, specify more precise expectations for the parameters to ensure the mock behaves as expected. This will make the tests more robust. 🎯

4. Code Duplication: The fields and args structs are repeated in both TestClient_GetHookByUrl and TestClient_CreateHook. Consider refactoring to reduce duplication, possibly by creating helper functions or shared test setup code. This will make the code cleaner and easier to maintain. 🧹

5. Test Coverage: Ensure that edge cases are covered in your tests, such as invalid URLs or network failures, to improve the reliability of your code. 📈

---

😼 Mergecat review of pkg/vcs/gitlab_client/client.go

@@ -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. Custom Client Struct: The introduction of the GLClient struct is a good move for abstraction. However, ensure that all the services in GLClient are correctly initialized and used throughout the code. This change can help in testing and mocking, but it also increases complexity. Make sure to document this change well for future maintainability. 📚

2. Error Handling: In the CreateHook function, the error handling logic has been updated to check the HTTP status code. Ensure that glStatus is not nil before accessing glStatus.StatusCode to avoid potential nil pointer dereference. Consider using a helper function to handle HTTP status code checks to keep the code DRY. 🛠️

3. Context Addition: Adding context.Context to the ParseHook function is a good practice for handling timeouts and cancellations. Ensure that this context is properly propagated and used in any downstream calls that support context. ⏳

4. Event Handling: The addition of handling MergeCommentEvent is a useful feature. Ensure that the ReplanCommentMessage is well-defined and documented, as it seems to be a critical part of the logic. Consider logging at different levels (e.g., Debug vs. Info) based on the importance of the message. 📝

5. Code Consistency: Ensure that the naming conventions are consistent across the codebase. For example, MergeRequestsServices vs. MergeRequestsService. Consistency helps in maintaining readability and understanding of the code. 🔍

6. Security Considerations: When dealing with webhook secrets and tokens, ensure that they are handled securely. Consider using environment variables or secure vaults for storing sensitive information. 🔒

Overall, the changes introduce useful abstractions and features, but ensure that they are well-documented and tested to maintain code quality and security. 🚀

---

---

Dependency Review

Click to read mergecats review!

No suggestions found