Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle multi source applications #298

Merged
merged 27 commits into from
Dec 16, 2024
Merged

handle multi source applications #298

merged 27 commits into from
Dec 16, 2024

Conversation

djeebus
Copy link
Collaborator

@djeebus djeebus commented Oct 21, 2024

this also removes the ability to reference a schemas dir relative to the root repo, but I'm hoping it's unused, as it's complicated to reason about or manage. a central schemas repo makes more sense.

We are now using argocd-repo-server to generate manifests.

  • ✅ This should also provide better performance re: cachability and better compatibility re: configuration, plugins, etc.
  • ✅ This might mean we'll do a better job of staying in sync with new upstream features.
  • ⚠️ This is an internal dependency of argocd, which means kubechecks will either have to be deployed to the same cluster that argocd is installed in, or a grpc communication tunnel needs to be open btween kubechecks and argocd.
  • ⚠️ This requires that we bundle up the application's sources and send them to the repo server, which required some fun complexity, but it also allowed us to package up source refs as well.

Copy link

github-actions bot commented Oct 21, 2024

Temporary image deleted.

Copy link
Collaborator

@Greyeye Greyeye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@djeebus
Copy link
Collaborator Author

djeebus commented Oct 29, 2024

Not sure if it works yet, but it definitely broke kubeconform schema checking.

@djeebus djeebus marked this pull request as draft October 29, 2024 14:08
pkg/argo_client/client.go Fixed Show fixed Hide fixed
@djeebus djeebus marked this pull request as ready for review November 8, 2024 17:53
@loadstar81
Copy link

appreciate the work on this!

Copy link
Collaborator

@MeNsaaH MeNsaaH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions. As for the unittest, you can generate easily them all using the genie integration on Vscode with OpenAI or with copilot, if you use either of them.

pkg/app_watcher/app_watcher.go Show resolved Hide resolved
pkg/argo_client/manifests.go Outdated Show resolved Hide resolved
pkg/argo_client/manifests.go Outdated Show resolved Hide resolved
pkg/argo_client/manifests.go Show resolved Hide resolved
pkg/argo_client/manifests.go Outdated Show resolved Hide resolved
return ctr, nil
}

func buildAppsMap(ctx context.Context, argoClient *argo_client.ArgoClient, result appdir.VcsToArgoMap) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unittest

return nil
}

func buildAppSetsMap(ctx context.Context, argoClient *argo_client.ArgoClient, result appdir.VcsToArgoMap) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unittest

@@ -227,20 +223,36 @@ func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneU
return repo, nil
}

func (ce *CheckEvent) mergeIntoTarget(ctx context.Context, repo *git.Repo, branch string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unittest?

pkg/events/worker.go Show resolved Hide resolved
pkg/repoUrl.go Show resolved Hide resolved
@djeebus
Copy link
Collaborator Author

djeebus commented Dec 4, 2024

A few questions. As per the unittest, you can generate easily them all using the genie integration on Vscode with OpenAI or with copilot, if you use either of them.

Thanks for the review! This gives me a good idea as to what needs to be better documented.

Copy link
Collaborator

@MeNsaaH MeNsaaH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are looking good. I've also tested them on my end and it works fine, Thanks!

The only thing is I think we need to add more tests. We're doing a lot of intricate file/app modifications. We need to catch when it breaks early. I've highlighted some funcs that we can add tests to.

Great work!

pkg/argo_client/manifests.go Outdated Show resolved Hide resolved
pkg/argo_client/manifests.go Outdated Show resolved Hide resolved
pkg/argo_client/manifests.go Show resolved Hide resolved
pkg/argo_client/manifests.go Show resolved Hide resolved
pkg/argo_client/manifests.go Show resolved Hide resolved
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of charts/kubechecks/Chart.yaml

@@ -1,7 +1,7 @@
 apiVersion: v2
 name: kubechecks
 description: A Helm chart for kubechecks
-version: 0.4.6
+version: 0.5.0
 type: application
 maintainers:
   - name: zapier

Feedback & Suggestions:

  • 📈 The version update from 0.4.6 to 0.5.0 suggests a minor version change, which typically indicates new features or improvements that are backward-compatible. Ensure that the changes in the codebase reflect this versioning convention. If there are breaking changes, consider updating to 1.0.0 instead.

  • 📝 It might be helpful to include a changelog or release notes to document what changes or improvements have been made in this new version. This can assist users in understanding the update's impact.


😼 Mergecat review of .tool-versions

@@ -6,5 +6,6 @@ helm-cr 1.6.1
 helm-ct 3.11.0
 kubeconform 0.6.7
 kustomize 5.5.0
+mockery 2.46.3
 staticcheck 2024.1.1
 tilt 0.33.2

Feedback & Suggestions:

  • 🛡️ Security & Compatibility: Ensure that the addition of mockery 2.46.3 is compatible with the rest of your toolchain. Check for any known vulnerabilities or compatibility issues with other tools listed.

  • 📈 Version Management: Consider documenting why this specific version of mockery was chosen. This can help in future updates or troubleshooting.

  • 🔄 Consistency: If mockery is a new addition to your workflow, ensure that any related configuration or usage documentation is updated accordingly.


😼 Mergecat review of cmd/locations.go

@@ -25,6 +25,8 @@ func processLocations(ctx context.Context, ctr container.Container, locations []
 		}
 	}
 
+	log.Debug().Strs("locations", locations).Msg("locations after processing")
+
 	return nil
 }
 

Feedback & Suggestions:

  1. Logging Sensitivity: Ensure that the locations array does not contain sensitive information before logging it. If it does, consider redacting or masking sensitive parts to prevent potential information leakage. 🔒

  2. Performance Consideration: If locations is a large array, logging it in its entirety could impact performance. Consider logging only a subset or a summary of the data if the full detail is not necessary for debugging. 🚀

  3. Conditional Logging: If this debug log is only needed under certain conditions, consider wrapping it with a condition to avoid unnecessary logging in production environments. This can help reduce log noise and improve performance. 📉


😼 Mergecat review of pkg/checks/kubeconform/check.go

@@ -8,8 +8,5 @@ import (
 )
 
 func Check(ctx context.Context, request checks.Request) (msg.Result, error) {
-	return argoCdAppValidate(
-		ctx, request.Container, request.AppName, request.KubernetesVersion, request.Repo.Directory,
-		request.YamlManifests,
-	)
+	return argoCdAppValidate(ctx, request.Container, request.AppName, request.KubernetesVersion, request.YamlManifests)
 }

Feedback & Suggestions:

  • 🛡️ Security: Ensure that request.YamlManifests is properly sanitized before being passed to argoCdAppValidate to prevent potential injection attacks.
  • Performance: If request.Repo.Directory is no longer needed, ensure that any related logic or dependencies are also removed to avoid unnecessary processing.
  • 🧹 Code Readability: The removal of request.Repo.Directory from the function call suggests a change in the function's signature or logic. Ensure that this change is documented and that all related code is updated accordingly to prevent confusion.

😼 Mergecat review of charts/kubechecks/values.yaml

@@ -1,4 +1,7 @@
 # Labels to apply to all resources created by this Helm chart
+argocd:
+  namespace: argocd
+
 commonLabels: {}
 
 configMap:

Feedback & Suggestions:

  1. Namespace Configuration: The addition of the argocd namespace is a good step towards organizing configurations specific to ArgoCD. However, ensure that this namespace is used consistently throughout the Helm chart to avoid any misconfigurations or deployment issues.

  2. Documentation: Consider adding comments or documentation to explain the purpose of the new argocd section. This will help other developers understand why this change was made and how it should be used.

  3. Validation: Ensure that the argocd namespace is created and available in the Kubernetes cluster before deploying this Helm chart. You might want to include a pre-install hook or a note in the documentation to guide users on this requirement.

  4. Security Considerations: If the argocd namespace is intended to interact with sensitive resources, ensure that appropriate role-based access control (RBAC) policies are in place to prevent unauthorized access.


😼 Mergecat review of mocks/gitlab_client/mocks/mock_RepositoryFilesServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package gitlab_client
 
@@ -32,6 +32,10 @@ func (_m *MockRepositoryFilesServices) GetRawFile(pid interface{}, fileName stri
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetRawFile")
+	}
+
 	var r0 []byte
 	var r1 *gitlab.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of a panic statement when no return value is specified for GetRawFile can be risky. Panics should be used sparingly as they can crash the program. Consider returning an error instead, which can be handled gracefully by the caller. This will improve the robustness of the code. 😬

  2. Version Update: The update from mockery v2.37.1 to v2.46.3 is noted. Ensure that this version change is compatible with the rest of your codebase and that any new features or changes in the mockery library are accounted for. 📦


😼 Mergecat review of .mockery.yaml

@@ -1,6 +1,9 @@
 with-expecter: true
 dir: "mocks/{{.PackageName}}/mocks"
 packages:
+  github.com/zapier/kubechecks/pkg/vcs:
+    config:
+      all: true
   github.com/zapier/kubechecks/pkg/vcs/github_client:
     # place your package-specific config here
     config:

Feedback & Suggestions:

  1. Redundancy Check: 🧐 Ensure that adding the github.com/zapier/kubechecks/pkg/vcs package with all: true does not overlap or conflict with the existing github_client and gitlab_client configurations. If all: true is intended to cover all sub-packages, you might not need the specific entries for github_client and gitlab_client unless they have unique configurations.

  2. Documentation: 📄 Consider adding a comment to explain why the github.com/zapier/kubechecks/pkg/vcs package was added. This can help future maintainers understand the purpose of this change.

  3. Testing: 🧪 After making this change, ensure that the mock generation behaves as expected for all packages. This includes verifying that no unintended mocks are generated or omitted.


😼 Mergecat review of mocks/gitlab_client/mocks/mock_CommitsServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package gitlab_client
 
@@ -32,6 +32,10 @@ func (_m *MockCommitsServices) SetCommitStatus(pid interface{}, sha string, opt
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for SetCommitStatus")
+	}
+
 	var r0 *gitlab.CommitStatus
 	var r1 *gitlab.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of a panic when no return value is specified for SetCommitStatus can be problematic. Panics should be used sparingly as they can cause the program to crash unexpectedly. Consider using a more graceful error handling approach, such as returning an error or logging a warning. This will make the code more robust and easier to debug. 🛑

  2. Version Update: The update from mockery v2.37.1 to v2.46.3 is noted. Ensure that this version change is compatible with the rest of your codebase and that any new features or changes in the mockery library are accounted for. 📦


😼 Mergecat review of mocks/gitlab_client/mocks/mock_PipelinesServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package gitlab_client
 
@@ -32,6 +32,10 @@ func (_m *MockPipelinesServices) ListProjectPipelines(pid interface{}, opt *gitl
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListProjectPipelines")
+	}
+
 	var r0 []*gitlab.PipelineInfo
 	var r1 *gitlab.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of a panic statement when no return value is specified for ListProjectPipelines can be problematic. Panics should be used sparingly as they can cause the program to crash unexpectedly. Consider using a more graceful error handling approach, such as returning an error, to allow the caller to handle the situation appropriately. This will make the code more robust and easier to maintain. 🛑

  2. Version Update Comment: The comment indicating the version change from v2.37.1 to v2.46.3 is clear and helpful for tracking changes. Ensure that any changes in the generated code due to the version update are compatible with the rest of your codebase. 🔄


😼 Mergecat review of mocks/affected_apps/mocks/mock_Matcher.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package affected_apps
 
@@ -29,6 +29,10 @@ func (_m *MockMatcher) EXPECT() *MockMatcher_Expecter {
 func (_m *MockMatcher) AffectedApps(ctx context.Context, changeList []string, targetBranch string, repo *git.Repo) (affected_apps.AffectedItems, error) {
 	ret := _m.Called(ctx, changeList, targetBranch, repo)
 
+	if len(ret) == 0 {
+		panic("no return value specified for AffectedApps")
+	}
+
 	var r0 affected_apps.AffectedItems
 	var r1 error
 	if rf, ok := ret.Get(0).(func(context.Context, []string, string, *git.Repo) (affected_apps.AffectedItems, error)); ok {

Feedback & Suggestions:

  1. Panic Usage: The addition of a panic statement when no return value is specified for AffectedApps can be problematic. Panics should be used sparingly as they can crash the program. Consider returning an error instead, which can be handled gracefully by the caller. For example:

    if len(ret) == 0 {
        return nil, errors.New("no return value specified for AffectedApps")
    }
  2. Error Handling: Ensure that the error returned is informative and provides enough context for debugging. This will help in identifying the issue quickly when the error is encountered.

  3. Version Update: The update in the mockery version comment is fine, but ensure that the rest of the code is compatible with the new version of the tool. Sometimes, updates in tools can introduce breaking changes or deprecations.


😼 Mergecat review of mocks/generator/mocks/mock_AppsGenerator.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package generator
 
@@ -29,6 +29,10 @@ func (_m *MockAppsGenerator) EXPECT() *MockAppsGenerator_Expecter {
 func (_m *MockAppsGenerator) GenerateApplicationSetApps(ctx context.Context, appset v1alpha1.ApplicationSet, ctr *container.Container) ([]v1alpha1.Application, error) {
 	ret := _m.Called(ctx, appset, ctr)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GenerateApplicationSetApps")
+	}
+
 	var r0 []v1alpha1.Application
 	var r1 error
 	if rf, ok := ret.Get(0).(func(context.Context, v1alpha1.ApplicationSet, *container.Container) ([]v1alpha1.Application, error)); ok {

Feedback & Suggestions:

  1. Panic Usage: The addition of a panic statement when no return value is specified for GenerateApplicationSetApps can be problematic. Panics should be used sparingly as they can crash the program. Consider returning a meaningful error instead. This will make the function more robust and easier to handle in tests. For example:

    if len(ret) == 0 {
        return nil, errors.New("no return value specified for GenerateApplicationSetApps")
    }
  2. Version Update: The update from mockery v2.37.1 to v2.46.3 is noted. Ensure that this version change is compatible with the rest of your codebase and that any new features or changes in the mockery library are accounted for in your tests.


😼 Mergecat review of pkg/repoUrl_test.go

+
+func TestAreSameRepos(t *testing.T) {
+	testcases := map[string]struct {
+		input1, input2 string
+		expected       bool
+	}{
+		"empty":                {"", "", true},
+		"empty1":               {"", "blah", false},
+		"empty2":               {"blah", "", false},
+		"git-to-git":           {"[email protected]:zapier/kubechecks.git", "[email protected]:zapier/kubechecks.git", true},
+		"no-git-suffix-to-git": {"[email protected]:zapier/kubechecks", "[email protected]:zapier/kubechecks.git", true},
+		"https-to-git":         {"https://github.com/zapier/kubechecks", "[email protected]:zapier/kubechecks.git", true},
+	}
+	for name, tc := range testcases {
+		t.Run(name, func(t *testing.T) {
+			actual := AreSameRepos(tc.input1, tc.input2)
+			assert.Equal(t, tc.expected, actual)
+		})
+	}
+}

Feedback & Suggestions:

  1. Test Coverage: The test cases cover a good range of scenarios, but consider adding more cases to cover different protocols and edge cases, such as:

    • Comparing URLs with different protocols (e.g., http vs https).
    • URLs with different subdomains or ports.
    • URLs with additional path segments or query parameters.
  2. Function Documentation: Ensure that the AreSameRepos function is well-documented, especially if it handles normalization or comparison logic that isn't immediately obvious.

  3. Variable Naming: The variable testcases could be renamed to testCases to maintain consistency with the naming convention used in TestNormalizeStrings.

  4. Error Handling: If AreSameRepos can return an error, consider adding error handling in the test cases to ensure robust testing.

  5. Code Readability: The test case names are descriptive, which is great for readability. Ensure that the logic within AreSameRepos is equally clear and maintainable.


😼 Mergecat review of pkg/repoUrl.go

@@ -6,6 +6,8 @@ import (
 	"strings"
 
 	"github.com/chainguard-dev/git-urls"
+	"github.com/pkg/errors"
+	"github.com/rs/zerolog/log"
 )
 
 type RepoURL struct {
@@ -41,3 +43,28 @@ func NormalizeRepoUrl(s string) (RepoURL, url.Values, error) {
 		Path: r.Path,
 	}, r.Query(), nil
 }
+
+func Canonicalize(cloneURL string) (RepoURL, error) {
+	parsed, _, err := NormalizeRepoUrl(cloneURL)
+	if err != nil {
+		return RepoURL{}, errors.Wrap(err, "failed to parse clone url")
+	}
+
+	return parsed, nil
+}
+
+func AreSameRepos(url1, url2 string) bool {
+	repo1, err := Canonicalize(url1)
+	if err != nil {
+		log.Warn().Msgf("failed to canonicalize %q", url1)
+		return false
+	}
+
+	repo2, err := Canonicalize(url2)
+	if err != nil {
+		log.Warn().Msgf("failed to canonicalize %q", url2)
+		return false
+	}
+
+	return repo1 == repo2
+}

Feedback & Suggestions:

  1. Error Handling Consistency: The Canonicalize function uses errors.Wrap for error handling, which is good for adding context. However, in AreSameRepos, the errors are logged but not returned. Consider returning the error from AreSameRepos to allow the caller to handle it appropriately. This would make the function more versatile and informative. 🛠️

  2. Logging Level: The use of log.Warn() in AreSameRepos is appropriate for non-critical issues. However, if failing to canonicalize a URL is a common occurrence, consider using log.Debug() to avoid cluttering logs with warnings. Adjust the logging level based on the expected frequency and severity of the issue. 📋

  3. Function Naming: The function AreSameRepos could be renamed to AreReposEqual for better readability and to follow Go naming conventions, which often use verbs for function names. This is a minor suggestion but can improve code clarity. ✍️

  4. Performance Consideration: The AreSameRepos function calls Canonicalize twice, which involves parsing URLs twice. If performance becomes a concern, consider optimizing by caching results or using a more efficient comparison method. 🚀


😼 Mergecat review of pkg/git/repo.go

@@ -81,6 +81,7 @@ func (r *Repo) Clone(ctx context.Context) error {
 		}
 	}
 
+	log.Info().Msg("repo has been cloned")
 	return nil
 }
 
@@ -107,22 +108,22 @@ func (r *Repo) GetRemoteHead() (string, error) {
 	return branchName, nil
 }
 
-func (r *Repo) MergeIntoTarget(ctx context.Context, sha string) error {
+func (r *Repo) MergeIntoTarget(ctx context.Context, ref string) error {
 	// Merge the last commit into a tmp branch off of the target branch
 	_, span := tracer.Start(ctx, "Repo - RepoMergeIntoTarget",
 		trace.WithAttributes(
 			attribute.String("branch_name", r.BranchName),
 			attribute.String("clone_url", r.CloneURL),
 			attribute.String("directory", r.Directory),
-			attribute.String("sha", sha),
+			attribute.String("sha", ref),
 		))
 	defer span.End()
 
-	cmd := r.execCommand("git", "merge", sha)
+	cmd := r.execCommand("git", "merge", ref)
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		telemetry.SetError(span, err, "merge commit into branch")
-		log.Error().Err(err).Msgf("unable to merge %s, %s", sha, out)
+		log.Error().Err(err).Msgf("unable to merge %s, %s", ref, out)
 		return err
 	}
 

Feedback & Suggestions:

  1. Logging Improvement: The addition of log.Info().Msg("repo has been cloned") is a good step for better logging. However, consider adding more context to the log message, such as the directory or branch name, to make it more informative. For example:

    log.Info().Str("directory", r.Directory).Str("branch", r.BranchName).Msg("repo has been cloned")
  2. Variable Naming: The change from sha to ref in the MergeIntoTarget function is a positive improvement for clarity, as it broadens the scope of what can be merged (not just SHA). Ensure that all related documentation and comments are updated to reflect this change.

  3. Trace Attribute Naming: The trace attribute name attribute.String("sha", ref) should be updated to reflect the new variable name. Consider using attribute.String("ref", ref) to maintain consistency and clarity.

  4. Error Handling: When logging errors, consider including more context about the operation that failed. This can help in debugging. For example, you could include the branch name or directory in the error log.


😼 Mergecat review of pkg/events/runner.go

@@ -11,7 +11,6 @@ import (
 	"github.com/zapier/kubechecks/pkg"
 	"github.com/zapier/kubechecks/pkg/checks"
 	"github.com/zapier/kubechecks/pkg/container"
-	"github.com/zapier/kubechecks/pkg/git"
 	"github.com/zapier/kubechecks/pkg/msg"
 	"github.com/zapier/kubechecks/telemetry"
 )
@@ -23,9 +22,13 @@ type Runner struct {
 }
 
 func newRunner(
-	ctr container.Container, app v1alpha1.Application, repo *git.Repo,
-	appName, k8sVersion string, jsonManifests, yamlManifests []string,
-	logger zerolog.Logger, note *msg.Message, queueApp, removeApp func(application v1alpha1.Application),
+	ctr container.Container,
+	app v1alpha1.Application,
+	appName, k8sVersion string,
+	jsonManifests, yamlManifests []string,
+	logger zerolog.Logger,
+	note *msg.Message,
+	queueApp, removeApp func(application v1alpha1.Application),
 ) *Runner {
 	return &Runner{
 		Request: checks.Request{
@@ -38,7 +41,6 @@ func newRunner(
 			Note:              note,
 			QueueApp:          queueApp,
 			RemoveApp:         removeApp,
-			Repo:              repo,
 			YamlManifests:     yamlManifests,
 		},
 	}

Feedback & Suggestions:

  1. Unused Import Removal: The removal of the github.com/zapier/kubechecks/pkg/git import is appropriate if the repo parameter is no longer used. Ensure that this change is intentional and that the repo is not needed elsewhere in the code. If repo is required in other parts of the code, consider refactoring instead of removing.

  2. Function Parameter Formatting: The reformatting of the newRunner function parameters improves readability by placing each parameter on a new line. This is a good practice for functions with many parameters, as it enhances clarity. 👍

  3. Code Consistency: Ensure that the removal of the repo parameter from the newRunner function is consistent with the rest of the codebase. If repo is used in other parts of the application, verify that those usages are also updated or removed as necessary.

  4. Documentation Update: If this change affects the functionality or usage of the newRunner function, consider updating any related documentation or comments to reflect the new function signature and behavior.


😼 Mergecat review of README.md

@@ -7,13 +7,13 @@
 
 ## Pull/Merge Request driven checks
 
-When using ArgoCD, it can be difficult to tell just how your Pull/Merge Request (PR/MR) will impact your live deployment. `kubechecks` was designed to address this problem; every time a new PR/MR is created, `kubechecks` will automatically determine what's changed and how it will impact your `main`/default branchs state, informing you of the details directly on the PR/MR. As a bonus, it also lints and checks your Kubernetes manifests to let you know ahead of time if something is outdated, invalid, or otherwise not good practice.
+When using ArgoCD, it can be difficult to tell just how your Pull/Merge Request (PR/MR) will impact your live deployment. `kubechecks` was designed to address this problem; every time a new PR/MR is created, `kubechecks` will automatically determine what's changed and how it will impact your `main`/default branch's state, informing you of the details directly on the PR/MR. As a bonus, it also lints and checks your Kubernetes manifests to let you know ahead of time if something is outdated, invalid, or otherwise not good practice.
 
 ![Demo](./docs/gif/kubechecks.gif)
 
 ### How it works
 
-This tool provides a server function that processes webhooks from Gitlab/Github, clones the repository at the `HEAD` SHA of the PR/MR, and runs various check suites, commenting the output of each check in a single comment on your PR/MR. `kubechecks` talks directly to ArgoCD to get the live state of your deployments to ensure that you have the most accurate information about how your changes will affect your production code.
+This tool provides a server function that processes webhooks from Gitlab/Github, clones the repository at the `HEAD` SHA of the PR/MR, and runs various check suites, commenting the output of each check in a single comment on your PR/MR. `kubechecks` talks directly to ArgoCD to get the live state of your deployments and talks directly to ArgoCD's repo server to generate the new resources to ensure that you have the most accurate information about how your changes will affect your production code.
 
 ### Architecture
 

Feedback & Suggestions:

  1. Grammar Correction: The correction of "branchs" to "branch's" is a good improvement. Proper grammar enhances readability. 👍

  2. Clarity Improvement: The addition of "and talks directly to ArgoCD's repo server to generate the new resources" provides more clarity on the process. However, consider rephrasing for better flow, such as: "kubechecks communicates with ArgoCD to retrieve the live state of your deployments and with ArgoCD's repo server to generate new resources, ensuring you have the most accurate information about how your changes will affect your production code." This reduces repetition and improves readability. 📚


😼 Mergecat review of pkg/config/config.go

 type ServerConfig struct {
 	// argocd
-	ArgoCDServerAddr    string `mapstructure:"argocd-api-server-addr"`
-	ArgoCDToken         string `mapstructure:"argocd-api-token"`
-	ArgoCDPathPrefix    string `mapstructure:"argocd-api-path-prefix"`
-	ArgoCDInsecure      bool   `mapstructure:"argocd-api-insecure"`
-	ArgoCDNamespace     string `mapstructure:"argocd-api-namespace"`
-	ArgoCDPlainText     bool   `mapstructure:"argocd-api-plaintext"`
-	KubernetesConfig    string `mapstructure:"kubernetes-config"`
-	KubernetesType      string `mapstructure:"kubernetes-type"`
-	KubernetesClusterID string `mapstructure:"kubernetes-clusterid"`
+	ArgoCDServerAddr         string `mapstructure:"argocd-api-server-addr"`
+	ArgoCDToken              string `mapstructure:"argocd-api-token"`
+	ArgoCDPathPrefix         string `mapstructure:"argocd-api-path-prefix"`
+	ArgoCDInsecure           bool   `mapstructure:"argocd-api-insecure"`
+	ArgoCDNamespace          string `mapstructure:"argocd-api-namespace"`
+	ArgoCDPlainText          bool   `mapstructure:"argocd-api-plaintext"`
+	ArgoCDRepositoryEndpoint string `mapstructure:"argocd-repository-endpoint"`
+	ArgoCDRepositoryInsecure bool   `mapstructure:"argocd-repository-insecure"`
+	ArgoCDSendFullRepository bool   `mapstructure:"argocd-send-full-repository"`
+	KubernetesConfig         string `mapstructure:"kubernetes-config"`
+	KubernetesType           string `mapstructure:"kubernetes-type"`
+	KubernetesClusterID      string `mapstructure:"kubernetes-clusterid"`
 
 	// otel
 	EnableOtel        bool   `mapstructure:"otel-enabled"`

Feedback & Suggestions:

  1. Security Considerations: The addition of ArgoCDRepositoryEndpoint, ArgoCDRepositoryInsecure, and ArgoCDSendFullRepository introduces new configuration options that may affect security. Ensure that any insecure options (like ArgoCDRepositoryInsecure) are well-documented and default to secure settings. Consider adding validation to prevent insecure configurations unless explicitly required.

  2. Documentation: Update the documentation to reflect the new fields. This includes any README files, configuration guides, or inline comments that describe the configuration options.

  3. Testing: Ensure that there are tests covering the new configuration options. This includes unit tests for any logic that uses these fields and integration tests to verify that the configuration is correctly applied.

  4. Backward Compatibility: Verify that the addition of these fields does not break existing configurations. Ensure that the system behaves as expected when these new fields are not set, maintaining backward compatibility.

  5. Consistency: The naming convention for the new fields is consistent with the existing ones, which is good. Make sure this consistency is maintained across the entire codebase.


😼 Mergecat review of pkg/app_watcher/appset_watcher.go

@@ -13,8 +13,8 @@ import (
 	"github.com/rs/zerolog/log"
 	"github.com/zapier/kubechecks/pkg/appdir"
 	"github.com/zapier/kubechecks/pkg/config"
+	"github.com/zapier/kubechecks/pkg/container"
 	"k8s.io/apimachinery/pkg/util/runtime"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -28,16 +28,16 @@ type ApplicationSetWatcher struct {
 }
 
 // NewApplicationSetWatcher creates new instance of ApplicationWatcher.
-func NewApplicationSetWatcher(kubeCfg *rest.Config, vcsToArgoMap appdir.VcsToArgoMap, cfg config.ServerConfig) (*ApplicationSetWatcher, error) {
-	if kubeCfg == nil {
+func NewApplicationSetWatcher(ctr container.Container) (*ApplicationSetWatcher, error) {
+	if ctr.KubeClientSet == nil {
 		return nil, fmt.Errorf("kubeCfg cannot be nil")
 	}
 	ctrl := ApplicationSetWatcher{
-		applicationClientset: appclientset.NewForConfigOrDie(kubeCfg),
-		vcsToArgoMap:         vcsToArgoMap,
+		applicationClientset: appclientset.NewForConfigOrDie(ctr.KubeClientSet.Config()),
+		vcsToArgoMap:         ctr.VcsToArgoMap,
 	}
 
-	appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*30, cfg)
+	appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*30, ctr.Config)
 
 	ctrl.appInformer = appInformer
 	ctrl.appLister = appLister

Feedback & Suggestions:

  1. Dependency Injection: The change to use a container.Container for dependency injection is a good practice as it centralizes configuration and dependencies. However, ensure that the container.Container struct is well-documented and that its fields are initialized correctly before being passed to NewApplicationSetWatcher.

  2. Error Message: The error message "kubeCfg cannot be nil" should be updated to reflect the new parameter. Consider changing it to "KubeClientSet cannot be nil" for clarity.

  3. Type Safety: Ensure that ctr.KubeClientSet.Config() returns the expected type and that any potential errors are handled appropriately. The use of NewForConfigOrDie suggests that the application will terminate if the configuration is invalid, which might not be ideal in all scenarios.

  4. Code Consistency: The removal of the rest package import is appropriate since it's no longer directly used. Ensure that all other unused imports are also removed to keep the code clean.

  5. Testing: Make sure to update any unit tests or integration tests to reflect the changes in the function signature and the use of the container.Container.

Overall, these changes improve the modularity and maintainability of the code. Keep up the good work! 🚀


😼 Mergecat review of pkg/checks/diff/diff.go

@@ -259,18 +259,25 @@ func getArgoSettings(ctx context.Context, request checks.Request) (*settings.Set
 var nilApp = argoappv1.Application{}
 
 func isApp(item objKeyLiveTarget, manifests []byte) (argoappv1.Application, bool) {
+	logger := log.With().
+		Str("kind", item.key.Kind).
+		Str("name", item.key.Name).
+		Str("namespace", item.key.Namespace).
+		Str("group", item.key.Group).
+		Logger()
+
 	if strings.ToLower(item.key.Group) != "argoproj.io" {
-		log.Debug().Str("group", item.key.Group).Msg("group is not correct")
+		logger.Debug().Msg("group is not correct")
 		return nilApp, false
 	}
 	if strings.ToLower(item.key.Kind) != "application" {
-		log.Debug().Str("kind", item.key.Kind).Msg("kind is not correct")
+		logger.Debug().Msg("kind is not correct")
 		return nilApp, false
 	}
 
 	var app argoappv1.Application
 	if err := json.Unmarshal(manifests, &app); err != nil {
-		log.Warn().Err(err).Msg("failed to deserialize application")
+		logger.Warn().Err(err).Msg("failed to deserialize application")
 		return nilApp, false
 	}
 
@@ -346,8 +353,9 @@ type resourceInfoProvider struct {
 	namespacedByGk map[schema.GroupKind]bool
 }
 
-// Infer if obj is namespaced or not from corresponding live objects list. If corresponding live object has namespace then target object is also namespaced.
-// If live object is missing then it does not matter if target is namespaced or not.
+// IsNamespaced infers if obj is namespaced or not from corresponding live objects list. If corresponding live object
+// has namespace then target object is also namespaced. If live object is missing then it does not matter if target is
+// namespaced or not.
 func (p *resourceInfoProvider) IsNamespaced(gk schema.GroupKind) (bool, error) {
 	return p.namespacedByGk[gk], nil
 }

Feedback & Suggestions:

  1. Logging Enhancement: The introduction of a logger variable to consolidate logging fields is a good improvement for readability and maintainability. However, ensure that this logger is used consistently throughout the function to avoid any discrepancies in logging context. 👍

  2. Comment Clarity: The updated comment for the IsNamespaced function is clearer and more descriptive. This improves code readability and helps future developers understand the function's purpose more easily. 📚

  3. Performance Consideration: While the logger enhancement is beneficial, be mindful of the performance impact of creating a logger with multiple fields if this function is called frequently. Consider caching the logger if the fields do not change. 🚀

Overall, these changes improve the code's readability and maintainability. Keep up the good work! 🎉


😼 Mergecat review of pkg/checks/kubeconform/validate_test.go

@@ -1,7 +1,6 @@
 package kubeconform
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"strings"
@@ -16,17 +15,15 @@ import (
 )
 
 func TestDefaultGetSchemaLocations(t *testing.T) {
-	ctx := context.TODO()
 	ctr := container.Container{}
-	schemaLocations := getSchemaLocations(ctx, ctr, "/some/other/path")
+	schemaLocations := getSchemaLocations(ctr)
 
 	// default schema location is "./schemas"
 	assert.Len(t, schemaLocations, 1)
 	assert.Equal(t, "default", schemaLocations[0])
 }
 
 func TestGetRemoteSchemaLocations(t *testing.T) {
-	ctx := context.TODO()
 	ctr := container.Container{}
 
 	if os.Getenv("CI") == "" {
@@ -39,7 +36,7 @@ func TestGetRemoteSchemaLocations(t *testing.T) {
 
 	// t.Setenv("KUBECHECKS_SCHEMAS_LOCATION", fixture.URL)  // doesn't work because viper needs to initialize from root, which doesn't happen
 	viper.Set("schemas-location", []string{fixture.URL})
-	schemaLocations := getSchemaLocations(ctx, ctr, "/some/other/path")
+	schemaLocations := getSchemaLocations(ctr)
 	hasTmpDirPrefix := strings.HasPrefix(schemaLocations[0], "/tmp/schemas")
 	assert.Equal(t, hasTmpDirPrefix, true, "invalid schemas location. Schema location should have prefix /tmp/schemas but has %s", schemaLocations[0])
 }

Feedback & Suggestions:

  1. Context Removal: The removal of context.TODO() from the test functions suggests that the getSchemaLocations function no longer requires a context parameter. Ensure that this change aligns with the function's intended use, as context is often used for managing timeouts and cancellations in Go applications. If the context is not needed, this change is fine. Otherwise, consider reintroducing it if necessary for future-proofing or if the function might need it later.

  2. Function Signature Change: The change in the getSchemaLocations function signature (removing the path parameter) should be reflected in the function's implementation. Ensure that the function still behaves as expected without the path parameter. If the path is hardcoded or managed differently, document this change for clarity.

  3. Testing Environment: The test TestGetRemoteSchemaLocations is designed to run only in CI environments. Ensure that this behavior is well-documented so that developers understand why the test might be skipped locally. This can prevent confusion during local testing.

  4. Code Comments: The commented-out line regarding t.Setenv provides useful context about why it doesn't work. Consider adding a comment explaining the alternative approach used to set the schema location, which can be helpful for future maintainers.

Overall, the changes seem to simplify the function calls, but ensure that these simplifications do not compromise the functionality or flexibility of the code. 🛠️


😼 Mergecat review of mocks/gitlab_client/mocks/mock_ProjectsServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package gitlab_client
 
@@ -32,6 +32,10 @@ func (_m *MockProjectsServices) AddProjectHook(pid interface{}, opt *gitlab.AddP
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for AddProjectHook")
+	}
+
 	var r0 *gitlab.ProjectHook
 	var r1 *gitlab.Response
 	var r2 error
@@ -111,6 +115,10 @@ func (_m *MockProjectsServices) EditProjectHook(pid interface{}, hook int, opt *
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for EditProjectHook")
+	}
+
 	var r0 *gitlab.ProjectHook
 	var r1 *gitlab.Response
 	var r2 error
@@ -191,6 +199,10 @@ func (_m *MockProjectsServices) GetProject(pid interface{}, opt *gitlab.GetProje
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetProject")
+	}
+
 	var r0 *gitlab.Project
 	var r1 *gitlab.Response
 	var r2 error
@@ -270,6 +282,10 @@ func (_m *MockProjectsServices) ListProjectHooks(pid interface{}, opt *gitlab.Li
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListProjectHooks")
+	}
+
 	var r0 []*gitlab.ProjectHook
 	var r1 *gitlab.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of panic statements when no return value is specified for the mocked methods (AddProjectHook, EditProjectHook, GetProject, ListProjectHooks) can be problematic. Panics should be used sparingly as they can abruptly terminate the program. Consider using a more graceful error handling approach, such as returning an error or logging a warning. This will make the code more robust and easier to debug. 🛑

  2. Version Update: The update from mockery v2.37.1 to v2.46.3 is noted. Ensure that this version change is compatible with the rest of your codebase and that any new features or changes in the mockery library are properly utilized or accounted for. 📦

  3. Testing: If you decide to keep the panic statements, ensure that your tests are designed to handle these cases appropriately. This might involve using defer and recover in your test cases to assert that the panic occurs as expected. 🧪


😼 Mergecat review of cmd/root.go

@@ -85,7 +85,7 @@ func init() {
 		newStringOpts().
 			withChoices("hide", "delete").
 			withDefault("hide"))
-	stringSliceFlag(flags, "schemas-location", "Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.")
+	stringSliceFlag(flags, "schemas-location", "Sets schema locations to be used for every check request. Can be a common path on the host or git urls in either git or http(s) format.")
 	boolFlag(flags, "enable-conftest", "Set to true to enable conftest policy checking of manifests.")
 	stringSliceFlag(flags, "policies-location", "Sets rego policy locations to be used for every check request. Can be common path inside the repos being checked or git urls in either git or http(s) format.",
 		newStringSliceOpts().
@@ -124,9 +124,6 @@ func init() {
 }
 
 func setupLogOutput() {
-	output := zerolog.ConsoleWriter{Out: os.Stdout}
-	log.Logger = log.Output(output)
-
 	// Default level is info, unless debug flag is present
 	levelFlag := viper.GetString("log-level")
 	level, err := zerolog.ParseLevel(levelFlag)
@@ -135,6 +132,10 @@ func setupLogOutput() {
 	}
 
 	zerolog.SetGlobalLevel(level)
+
+	output := zerolog.ConsoleWriter{Out: os.Stdout}
+	log.Logger = log.Output(output)
+
 	log.Debug().Msg("Debug level logging enabled.")
 	log.Trace().Msg("Trace level logging enabled.")
 	log.Info().Msg("Initialized logger.")

Feedback & Suggestions:

  1. Order of Operations: The change in the setupLogOutput function moves the initialization of the zerolog.ConsoleWriter and the logger output after setting the global log level. This is a logical improvement as it ensures that the logger is configured with the correct level before any output is set. However, ensure that this change does not affect any log messages that might be emitted before this point.

  2. Documentation Clarity: The change in the schemas-location flag description improves clarity by specifying "a common path on the host." This is a good improvement for user understanding. Ensure that this change is reflected in any related documentation or user guides to maintain consistency.

  3. Potential Impact on Logging: Moving the logger output initialization might affect the logging behavior if any logs are emitted before this point. Double-check that no logs are intended to be output before the logger is fully configured.

Overall, these changes seem to improve the clarity and logical flow of the code. Just ensure that the logging behavior is as expected after these modifications. 🛠️


😼 Mergecat review of mocks/gitlab_client/mocks/mock_NotesServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package gitlab_client
 
@@ -32,6 +32,10 @@ func (_m *MockNotesServices) CreateMergeRequestNote(pid interface{}, mergeReques
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for CreateMergeRequestNote")
+	}
+
 	var r0 *gitlab.Note
 	var r1 *gitlab.Response
 	var r2 error
@@ -112,6 +116,10 @@ func (_m *MockNotesServices) DeleteMergeRequestNote(pid interface{}, mergeReques
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for DeleteMergeRequestNote")
+	}
+
 	var r0 *gitlab.Response
 	var r1 error
 	if rf, ok := ret.Get(0).(func(interface{}, int, int, ...gitlab.RequestOptionFunc) (*gitlab.Response, error)); ok {
@@ -183,6 +191,10 @@ func (_m *MockNotesServices) ListMergeRequestNotes(pid interface{}, mergeRequest
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListMergeRequestNotes")
+	}
+
 	var r0 []*gitlab.Note
 	var r1 *gitlab.Response
 	var r2 error
@@ -263,6 +275,10 @@ func (_m *MockNotesServices) UpdateMergeRequestNote(pid interface{}, mergeReques
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for UpdateMergeRequestNote")
+	}
+
 	var r0 *gitlab.Note
 	var r1 *gitlab.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of panic when no return value is specified can be problematic. Panics should be used sparingly as they can crash the program. Consider returning an error instead, which can be handled gracefully by the caller. This will improve the robustness of the code. 😱

  2. Error Handling: If you decide to return an error instead of panicking, ensure that the calling functions are updated to handle these errors appropriately. This will make the code more resilient and easier to debug. 🛠️

  3. Documentation Update: Since the behavior of the functions has changed (with the introduction of panic), it would be beneficial to update any relevant documentation or comments to reflect this change. This will help other developers understand the new behavior. 📚


😼 Mergecat review of mocks/generator/mocks/mock_Generator.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package generator
 
@@ -27,6 +27,10 @@ func (_m *MockGenerator) EXPECT() *MockGenerator_Expecter {
 func (_m *MockGenerator) GenerateParams(appSetGenerator *v1alpha1.ApplicationSetGenerator, applicationSetInfo *v1alpha1.ApplicationSet) ([]map[string]interface{}, error) {
 	ret := _m.Called(appSetGenerator, applicationSetInfo)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GenerateParams")
+	}
+
 	var r0 []map[string]interface{}
 	var r1 error
 	if rf, ok := ret.Get(0).(func(*v1alpha1.ApplicationSetGenerator, *v1alpha1.ApplicationSet) ([]map[string]interface{}, error)); ok {
@@ -82,6 +86,10 @@ func (_c *MockGenerator_GenerateParams_Call) RunAndReturn(run func(*v1alpha1.App
 func (_m *MockGenerator) GetRequeueAfter(appSetGenerator *v1alpha1.ApplicationSetGenerator) time.Duration {
 	ret := _m.Called(appSetGenerator)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetRequeueAfter")
+	}
+
 	var r0 time.Duration
 	if rf, ok := ret.Get(0).(func(*v1alpha1.ApplicationSetGenerator) time.Duration); ok {
 		r0 = rf(appSetGenerator)
@@ -124,6 +132,10 @@ func (_c *MockGenerator_GetRequeueAfter_Call) RunAndReturn(run func(*v1alpha1.Ap
 func (_m *MockGenerator) GetTemplate(appSetGenerator *v1alpha1.ApplicationSetGenerator) *v1alpha1.ApplicationSetTemplate {
 	ret := _m.Called(appSetGenerator)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetTemplate")
+	}
+
 	var r0 *v1alpha1.ApplicationSetTemplate
 	if rf, ok := ret.Get(0).(func(*v1alpha1.ApplicationSetGenerator) *v1alpha1.ApplicationSetTemplate); ok {
 		r0 = rf(appSetGenerator)

Feedback & Suggestions:

  1. Use of Panic: The introduction of panic when no return value is specified for GenerateParams, GetRequeueAfter, and GetTemplate can lead to abrupt termination of the program, which is generally not recommended in production code. Instead, consider returning an error or using a default value to handle such cases gracefully. This will improve the robustness of the code. 🚨

  2. Error Handling: If you decide to return an error instead of panicking, ensure that the calling functions are equipped to handle these errors appropriately. This will make the code more resilient and easier to debug. 🛠️

  3. Testing Implications: If these changes are meant for testing purposes, ensure that your test cases are updated to handle these new panic scenarios. This will help in maintaining the integrity of your test suite. 🧪


😼 Mergecat review of mocks/gitlab_client/mocks/mock_MergeRequestsServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package gitlab_client
 
@@ -32,6 +32,10 @@ func (_m *MockMergeRequestsServices) GetMergeRequest(pid interface{}, mergeReque
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetMergeRequest")
+	}
+
 	var r0 *gitlab.MergeRequest
 	var r1 *gitlab.Response
 	var r2 error
@@ -112,6 +116,10 @@ func (_m *MockMergeRequestsServices) GetMergeRequestChanges(pid interface{}, mer
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetMergeRequestChanges")
+	}
+
 	var r0 *gitlab.MergeRequest
 	var r1 *gitlab.Response
 	var r2 error
@@ -192,6 +200,10 @@ func (_m *MockMergeRequestsServices) GetMergeRequestDiffVersions(pid interface{}
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetMergeRequestDiffVersions")
+	}
+
 	var r0 []*gitlab.MergeRequestDiffVersion
 	var r1 *gitlab.Response
 	var r2 error
@@ -272,6 +284,10 @@ func (_m *MockMergeRequestsServices) ListMergeRequestDiffs(pid interface{}, merg
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListMergeRequestDiffs")
+	}
+
 	var r0 []*gitlab.MergeRequestDiff
 	var r1 *gitlab.Response
 	var r2 error
@@ -352,6 +368,10 @@ func (_m *MockMergeRequestsServices) UpdateMergeRequest(pid interface{}, mergeRe
 	_ca = append(_ca, _va...)
 	ret := _m.Called(_ca...)
 
+	if len(ret) == 0 {
+		panic("no return value specified for UpdateMergeRequest")
+	}
+
 	var r0 *gitlab.MergeRequest
 	var r1 *gitlab.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of panic when no return value is specified for the mocked methods is a bit aggressive. While it ensures that the mock is used correctly, it might lead to abrupt termination of tests. Consider using a more graceful error handling approach, such as returning an error or logging a warning. This will make your tests more robust and easier to debug. 🛑

  2. Version Update: The update from mockery v2.37.1 to v2.46.3 is noted. Ensure that this version change is compatible with your existing codebase and that any new features or changes in the mockery library are accounted for in your tests. 📦

  3. Code Consistency: Ensure that similar checks for return values are consistently applied across all mocked methods. This will help maintain uniform behavior and expectations across your test suite. 🔄


😼 Mergecat review of mocks/affected_apps/mocks/mock_argoClient.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package affected_apps
 
@@ -26,6 +26,10 @@ func (_m *MockargoClient) EXPECT() *MockargoClient_Expecter {
 func (_m *MockargoClient) GetApplications(ctx context.Context) (*v1alpha1.ApplicationList, error) {
 	ret := _m.Called(ctx)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetApplications")
+	}
+
 	var r0 *v1alpha1.ApplicationList
 	var r1 error
 	if rf, ok := ret.Get(0).(func(context.Context) (*v1alpha1.ApplicationList, error)); ok {
@@ -80,6 +84,10 @@ func (_c *MockargoClient_GetApplications_Call) RunAndReturn(run func(context.Con
 func (_m *MockargoClient) GetApplicationsByAppset(ctx context.Context, appsetName string) (*v1alpha1.ApplicationList, error) {
 	ret := _m.Called(ctx, appsetName)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetApplicationsByAppset")
+	}
+
 	var r0 *v1alpha1.ApplicationList
 	var r1 error
 	if rf, ok := ret.Get(0).(func(context.Context, string) (*v1alpha1.ApplicationList, error)); ok {

Feedback & Suggestions:

  1. Use of Panic: The introduction of panic when no return value is specified for GetApplications and GetApplicationsByAppset is not ideal. Panics should be avoided in production code as they can cause the application to crash. Instead, consider returning a default value or an error indicating the missing return value. This will make the code more robust and easier to handle in tests. 😱

  2. Error Handling: If you decide to return an error instead of panicking, ensure that the error message is clear and informative. This will help in debugging and understanding the context of the error. 🛠️

  3. Code Consistency: Ensure that any changes made to handle missing return values are consistent across all similar functions. This will help maintain uniformity and reduce the likelihood of bugs. 🔄


😼 Mergecat review of mocks/github_client/mocks/mock_PullRequestsServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package github_client
 
@@ -27,6 +27,10 @@ func (_m *MockPullRequestsServices) EXPECT() *MockPullRequestsServices_Expecter
 func (_m *MockPullRequestsServices) Get(ctx context.Context, owner string, repo string, number int) (*github.PullRequest, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, number)
 
+	if len(ret) == 0 {
+		panic("no return value specified for Get")
+	}
+
 	var r0 *github.PullRequest
 	var r1 *github.Response
 	var r2 error
@@ -93,6 +97,10 @@ func (_c *MockPullRequestsServices_Get_Call) RunAndReturn(run func(context.Conte
 func (_m *MockPullRequestsServices) GetRaw(ctx context.Context, owner string, repo string, number int, opts github.RawOptions) (string, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, number, opts)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetRaw")
+	}
+
 	var r0 string
 	var r1 *github.Response
 	var r2 error
@@ -158,6 +166,10 @@ func (_c *MockPullRequestsServices_GetRaw_Call) RunAndReturn(run func(context.Co
 func (_m *MockPullRequestsServices) List(ctx context.Context, owner string, repo string, opts *github.PullRequestListOptions) ([]*github.PullRequest, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, opts)
 
+	if len(ret) == 0 {
+		panic("no return value specified for List")
+	}
+
 	var r0 []*github.PullRequest
 	var r1 *github.Response
 	var r2 error
@@ -224,6 +236,10 @@ func (_c *MockPullRequestsServices_List_Call) RunAndReturn(run func(context.Cont
 func (_m *MockPullRequestsServices) ListFiles(ctx context.Context, owner string, repo string, number int, opts *github.ListOptions) ([]*github.CommitFile, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, number, opts)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListFiles")
+	}
+
 	var r0 []*github.CommitFile
 	var r1 *github.Response
 	var r2 error

Feedback & Suggestions:

  1. Use of panic: The introduction of panic when no return value is specified can lead to abrupt termination of the program, which is generally not recommended in production code. Instead, consider returning a default value or an error that can be handled gracefully by the caller. This will make the code more robust and easier to debug. 😱

  2. Error Handling: If you decide to keep the panic, ensure that it is well-documented and that the calling code is aware of this behavior. Alternatively, you could log an error message before panicking to provide more context about the failure. 📜

  3. Code Consistency: Ensure that the behavior of all methods is consistent. If one method panics on missing return values, all should, or none should. This consistency will help maintain the predictability of the mock's behavior. 🔄

  4. Testing: If you haven't already, consider adding tests to ensure that the new panic behavior is triggered appropriately and that the rest of the code handles it as expected. 🧪


😼 Mergecat review of docs/usage.md

@@ -41,6 +41,9 @@ The full list of supported environment variables is described below:
 |`KUBECHECKS_ARGOCD_API_PLAINTEXT`|Enable to use plaintext connections without TLS.|`false`|
 |`KUBECHECKS_ARGOCD_API_SERVER_ADDR`|ArgoCD API Server Address.|`argocd-server`|
 |`KUBECHECKS_ARGOCD_API_TOKEN`|ArgoCD API token.||
+|`KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT`|Location of the argocd repository service endpoint.|`argocd-repo-server.argocd:8081`|
+|`KUBECHECKS_ARGOCD_REPOSITORY_INSECURE`|True if you need to skip validating the grpc tls certificate.|`true`|
+|`KUBECHECKS_ARGOCD_SEND_FULL_REPOSITORY`|Set to true if you want to try to send the full repository to ArgoCD when generating manifests.|`false`|
 |`KUBECHECKS_ENABLE_CONFTEST`|Set to true to enable conftest policy checking of manifests.|`false`|
 |`KUBECHECKS_ENABLE_HOOKS_RENDERER`|Render hooks.|`true`|
 |`KUBECHECKS_ENABLE_KUBECONFORM`|Enable kubeconform checks.|`true`|
@@ -66,7 +69,7 @@ The full list of supported environment variables is described below:
 |`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_SCHEMAS_LOCATION`|Sets schema locations to be used for every check request. Can be a common path on the host or git urls in either git or http(s) format.|`[]`|
 |`KUBECHECKS_SHOW_DEBUG_INFO`|Set to true to print debug info to the footer of MR comments.|`false`|
 |`KUBECHECKS_TIDY_OUTDATED_COMMENTS_MODE`|Sets the mode to use when tidying outdated comments. One of hide, delete.|`hide`|
 |`KUBECHECKS_VCS_BASE_URL`|VCS base url, useful if self hosting gitlab, enterprise github, etc.||

Feedback & Suggestions:

  1. Security Concerns: The addition of KUBECHECKS_ARGOCD_REPOSITORY_INSECURE with a default value of true is a potential security risk. It is generally advisable to validate TLS certificates to ensure secure communication. Consider setting the default to false and providing guidance on when it might be appropriate to set it to true. 🔒

  2. Clarity on New Variables: The new environment variables KUBECHECKS_ARGOCD_REPOSITORY_ENDPOINT, KUBECHECKS_ARGOCD_REPOSITORY_INSECURE, and KUBECHECKS_ARGOCD_SEND_FULL_REPOSITORY should have clear descriptions and usage examples if possible. This will help users understand when and how to use them effectively. 📘

  3. Consistency in Descriptions: Ensure that the description for KUBECHECKS_SCHEMAS_LOCATION is consistent with the rest of the document. The change from "common paths inside the repos" to "a common path on the host" should be clearly explained if it reflects a change in functionality. 🔄


😼 Mergecat review of mocks/github_client/mocks/mock_IssuesServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package github_client
 
@@ -27,6 +27,10 @@ func (_m *MockIssuesServices) EXPECT() *MockIssuesServices_Expecter {
 func (_m *MockIssuesServices) CreateComment(ctx context.Context, owner string, repo string, number int, comment *github.IssueComment) (*github.IssueComment, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, number, comment)
 
+	if len(ret) == 0 {
+		panic("no return value specified for CreateComment")
+	}
+
 	var r0 *github.IssueComment
 	var r1 *github.Response
 	var r2 error
@@ -94,6 +98,10 @@ func (_c *MockIssuesServices_CreateComment_Call) RunAndReturn(run func(context.C
 func (_m *MockIssuesServices) DeleteComment(ctx context.Context, owner string, repo string, commentID int64) (*github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, commentID)
 
+	if len(ret) == 0 {
+		panic("no return value specified for DeleteComment")
+	}
+
 	var r0 *github.Response
 	var r1 error
 	if rf, ok := ret.Get(0).(func(context.Context, string, string, int64) (*github.Response, error)); ok {
@@ -151,6 +159,10 @@ func (_c *MockIssuesServices_DeleteComment_Call) RunAndReturn(run func(context.C
 func (_m *MockIssuesServices) EditComment(ctx context.Context, owner string, repo string, commentID int64, comment *github.IssueComment) (*github.IssueComment, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, commentID, comment)
 
+	if len(ret) == 0 {
+		panic("no return value specified for EditComment")
+	}
+
 	var r0 *github.IssueComment
 	var r1 *github.Response
 	var r2 error
@@ -218,6 +230,10 @@ func (_c *MockIssuesServices_EditComment_Call) RunAndReturn(run func(context.Con
 func (_m *MockIssuesServices) ListComments(ctx context.Context, owner string, repo string, number int, opts *github.IssueListCommentsOptions) ([]*github.IssueComment, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, number, opts)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListComments")
+	}
+
 	var r0 []*github.IssueComment
 	var r1 *github.Response
 	var r2 error

Feedback & Suggestions:

  1. Use of panic: The addition of panic statements when no return value is specified for the mocked methods (CreateComment, DeleteComment, EditComment, ListComments) is a bit aggressive. While it ensures that the mock is used correctly, it can lead to abrupt termination of the program, which might not be ideal in all testing scenarios. Consider using a more graceful error handling approach, such as returning a default error or logging a warning. This will make the tests more robust and easier to debug. 🛑

  2. Error Handling: Instead of panicking, you could return a meaningful error that can be checked in the test cases. This would allow the tests to handle the error gracefully and provide more informative feedback. For example, you could return an error like errors.New("mock: no return value specified for CreateComment"). 🐛

  3. Documentation Update: If you decide to keep the panic approach, it would be helpful to document this behavior clearly, so that developers using these mocks are aware of the potential for panics and can write their tests accordingly. 📚


😼 Mergecat review of pkg/affected_apps/argocd_matcher.go

@@ -6,7 +6,6 @@ import (
 
 	"github.com/rs/zerolog/log"
 	"github.com/zapier/kubechecks/pkg/appdir"
-	"github.com/zapier/kubechecks/pkg/container"
 	"github.com/zapier/kubechecks/pkg/git"
 )
 
@@ -15,7 +14,7 @@ type ArgocdMatcher struct {
 	appSetsDirectory *appdir.AppSetDirectory
 }
 
-func NewArgocdMatcher(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) (*ArgocdMatcher, error) {
+func NewArgocdMatcher(vcsToArgoMap appdir.VcsToArgoMap, repo *git.Repo) (*ArgocdMatcher, error) {
 	repoApps := getArgocdApps(vcsToArgoMap, repo)
 	kustomizeAppFiles := getKustomizeApps(vcsToArgoMap, repo, repo.Directory)
 
@@ -41,7 +40,7 @@ func logCounts(repoApps *appdir.AppDirectory) {
 	}
 }
 
-func getKustomizeApps(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo, repoPath string) *appdir.AppDirectory {
+func getKustomizeApps(vcsToArgoMap appdir.VcsToArgoMap, repo *git.Repo, repoPath string) *appdir.AppDirectory {
 	log.Debug().Msgf("creating fs for %s", repoPath)
 	fs := os.DirFS(repoPath)
 	log.Debug().Msg("following kustomize apps")
@@ -51,15 +50,15 @@ func getKustomizeApps(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo, repoP
 	return kustomizeAppFiles
 }
 
-func getArgocdApps(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) *appdir.AppDirectory {
+func getArgocdApps(vcsToArgoMap appdir.VcsToArgoMap, repo *git.Repo) *appdir.AppDirectory {
 	log.Debug().Msgf("looking for %s repos", repo.CloneURL)
 	repoApps := vcsToArgoMap.GetAppsInRepo(repo.CloneURL)
 
 	logCounts(repoApps)
 	return repoApps
 }
 
-func getArgocdAppSets(vcsToArgoMap container.VcsToArgoMap, repo *git.Repo) *appdir.AppSetDirectory {
+func getArgocdAppSets(vcsToArgoMap appdir.VcsToArgoMap, repo *git.Repo) *appdir.AppSetDirectory {
 	log.Debug().Msgf("looking for %s repos", repo.CloneURL)
 	repoApps := vcsToArgoMap.GetAppSetsInRepo(repo.CloneURL)
 

Feedback & Suggestions:

  1. Import Removal: The removal of the container package import is appropriate if VcsToArgoMap is no longer used from that package. Ensure that all references to container.VcsToArgoMap are correctly updated to appdir.VcsToArgoMap throughout the codebase to prevent any runtime errors. 🧹

  2. Type Consistency: The change from container.VcsToArgoMap to appdir.VcsToArgoMap suggests a refactor or a change in the package structure. Verify that the VcsToArgoMap type in appdir has the same methods and behavior as the one in container to avoid unexpected issues. 🔄

  3. Testing: After such changes, it's crucial to run existing tests and add new ones if necessary to ensure that the functionality remains intact. This is especially important if the VcsToArgoMap type has been altered. 🧪

  4. Documentation: If this change affects the public API or usage of the ArgocdMatcher, update any relevant documentation to reflect the new import paths and usage patterns. 📚


😼 Mergecat review of cmd/process.go

@@ -1,8 +1,13 @@
 package cmd
 
 import (
+	"os"
+	"path/filepath"
+
+	"github.com/argoproj/argo-cd/v2/common"
 	"github.com/rs/zerolog/log"
 	"github.com/spf13/cobra"
+	"github.com/zapier/kubechecks/pkg/container"
 
 	"github.com/zapier/kubechecks/pkg/config"
 	"github.com/zapier/kubechecks/pkg/server"
@@ -15,14 +20,42 @@ var processCmd = &cobra.Command{
 	Run: func(cmd *cobra.Command, args []string) {
 		ctx := cmd.Context()
 
+		tempPath, err := os.MkdirTemp("", "")
+		if err != nil {
+			log.Fatal().Err(err).Msg("fail to create ssh data dir")
+		}
+		defer func() {
+			os.RemoveAll(tempPath)
+		}()
+
+		// symlink local ssh known hosts to argocd ssh known hosts
+		homeDir, err := os.UserHomeDir()
+		if err != nil {
+			log.Fatal().Err(err).Msg("failed to get user home dir")
+		}
+		source := filepath.Join(homeDir, ".ssh", "known_hosts")
+		target := filepath.Join(tempPath, common.DefaultSSHKnownHostsName)
+
+		if err := os.Symlink(source, target); err != nil {
+			log.Fatal().Err(err).Msg("fail to symlink ssh_known_hosts file")
+		}
+
+		if err := os.Setenv("ARGOCD_SSH_DATA_PATH", tempPath); err != nil {
+			log.Fatal().Err(err).Msg("fail to set ARGOCD_SSH_DATA_PATH")
+		}
+
 		cfg, err := config.New()
 		if err != nil {
 			log.Fatal().Err(err).Msg("failed to generate config")
 		}
 
-		ctr, err := newContainer(ctx, cfg, false)
+		if len(args) != 1 {
+			log.Fatal().Msg("usage: kubechecks process PR_REF")
+		}
+
+		ctr, err := container.New(ctx, cfg)
 		if err != nil {
-			log.Fatal().Err(err).Msg("failed to create container")
+			log.Fatal().Err(err).Msg("failed to create clients")
 		}
 
 		log.Info().Msg("initializing git settings")

Feedback & Suggestions:

  1. Error Handling for os.Symlink and os.Setenv:

    • Consider using log.Error() instead of log.Fatal() for non-critical errors like failing to create a symlink or set an environment variable. This allows the program to continue running if these operations are not essential.
    • 💡 Suggestion: Use log.Error() and handle the error gracefully if possible.
  2. Security Consideration for os.MkdirTemp:

    • Ensure that the temporary directory created with os.MkdirTemp is used securely. The current implementation removes the directory after use, which is good, but ensure no sensitive data is stored there longer than necessary.
    • 🔒 Suggestion: Double-check that no sensitive data is written to this temporary directory.
  3. Argument Validation:

    • The check for len(args) != 1 is a good addition. However, consider providing more detailed feedback to the user about what PR_REF should be.
    • 🗣️ Suggestion: Enhance the error message to guide the user on what PR_REF represents.
  4. Resource Cleanup:

    • The use of defer os.RemoveAll(tempPath) is appropriate for cleaning up resources. Ensure that this cleanup is sufficient for all scenarios, especially if the program exits unexpectedly.
    • 🧹 Suggestion: Consider additional cleanup strategies if the program might exit before reaching the defer statement.
  5. Import Management:

    • The addition of new imports should be reviewed to ensure they are necessary and do not introduce any unwanted dependencies.
    • 📦 Suggestion: Verify that all new imports are essential and do not bloat the application unnecessarily.

😼 Mergecat review of pkg/appdir/vcstoargomap.go

@@ -79,37 +79,39 @@ func (v2a VcsToArgoMap) WalkKustomizeApps(cloneURL string, fs fs.FS) *AppDirecto
 	return result
 }
 
-func (v2a VcsToArgoMap) AddApp(app *v1alpha1.Application) {
-	if app.Spec.Source == nil {
-		log.Warn().Msgf("%s/%s: no source, skipping", app.Namespace, app.Name)
-		return
+func (v2a VcsToArgoMap) processApp(app v1alpha1.Application, fn func(*AppDirectory)) {
+
+	if src := app.Spec.Source; src != nil {
+		appDirectory := v2a.GetAppsInRepo(src.RepoURL)
+		fn(appDirectory)
 	}
 
-	appDirectory := v2a.GetAppsInRepo(app.Spec.Source.RepoURL)
-	appDirectory.ProcessApp(*app)
+	for _, src := range app.Spec.Sources {
+		appDirectory := v2a.GetAppsInRepo(src.RepoURL)
+		fn(appDirectory)
+	}
 }
 
-func (v2a VcsToArgoMap) UpdateApp(old *v1alpha1.Application, new *v1alpha1.Application) {
-	if new.Spec.Source == nil {
-		log.Warn().Msgf("%s/%s: no source, skipping", new.Namespace, new.Name)
-		return
-	}
+func (v2a VcsToArgoMap) AddApp(app *v1alpha1.Application) {
+	v2a.processApp(*app, func(directory *AppDirectory) {
+		directory.AddApp(*app)
+	})
+}
 
-	oldAppDirectory := v2a.GetAppsInRepo(old.Spec.Source.RepoURL)
-	oldAppDirectory.RemoveApp(*old)
+func (v2a VcsToArgoMap) UpdateApp(old *v1alpha1.Application, new *v1alpha1.Application) {
+	v2a.processApp(*old, func(directory *AppDirectory) {
+		directory.RemoveApp(*old)
+	})
 
-	newAppDirectory := v2a.GetAppsInRepo(new.Spec.Source.RepoURL)
-	newAppDirectory.ProcessApp(*new)
+	v2a.processApp(*new, func(directory *AppDirectory) {
+		directory.AddApp(*new)
+	})
 }
 
 func (v2a VcsToArgoMap) DeleteApp(app *v1alpha1.Application) {
-	if app.Spec.Source == nil {
-		log.Warn().Msgf("%s/%s: no source, skipping", app.Namespace, app.Name)
-		return
-	}
-
-	oldAppDirectory := v2a.GetAppsInRepo(app.Spec.Source.RepoURL)
-	oldAppDirectory.RemoveApp(*app)
+	v2a.processApp(*app, func(directory *AppDirectory) {
+		directory.RemoveApp(*app)
+	})
 }
 
 func (v2a VcsToArgoMap) GetVcsRepos() []string {

Feedback & Suggestions:

  1. Functionality Change: The refactoring introduces a new processApp function that handles both single and multiple sources. This is a good abstraction, but ensure that the app.Spec.Sources field is correctly populated and intended to be used this way. If Sources is not always populated, this could lead to unexpected behavior.

  2. Error Handling: The original code logged a warning if app.Spec.Source was nil. The new code does not log a warning if both app.Spec.Source and app.Spec.Sources are nil or empty. Consider adding a check to log a warning if no sources are found to maintain the original behavior and provide better traceability. 🛑

  3. Performance Consideration: The loop over app.Spec.Sources could potentially lead to performance issues if the list is large. Ensure that this is the intended behavior and that the list size is manageable. Consider adding logging or metrics to monitor the performance impact. 📈

  4. Code Clarity: The use of a function parameter fn to perform operations on AppDirectory is a flexible approach, but it might reduce readability for some developers. Consider adding comments or documentation to explain the purpose and usage of processApp. 📚

  5. Testing: Ensure that there are adequate tests covering both single and multiple source scenarios to verify that the refactoring does not introduce any regressions. 🧪


😼 Mergecat review of mocks/github_client/mocks/mock_RepositoriesServices.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.37.1. DO NOT EDIT.
+// Code generated by mockery v2.46.3. DO NOT EDIT.
 
 package github_client
 
@@ -27,6 +27,10 @@ func (_m *MockRepositoriesServices) EXPECT() *MockRepositoriesServices_Expecter
 func (_m *MockRepositoriesServices) CreateHook(ctx context.Context, owner string, repo string, hook *github.Hook) (*github.Hook, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, hook)
 
+	if len(ret) == 0 {
+		panic("no return value specified for CreateHook")
+	}
+
 	var r0 *github.Hook
 	var r1 *github.Response
 	var r2 error
@@ -93,6 +97,10 @@ func (_c *MockRepositoriesServices_CreateHook_Call) RunAndReturn(run func(contex
 func (_m *MockRepositoriesServices) CreateStatus(ctx context.Context, owner string, repo string, ref string, status *github.RepoStatus) (*github.RepoStatus, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, ref, status)
 
+	if len(ret) == 0 {
+		panic("no return value specified for CreateStatus")
+	}
+
 	var r0 *github.RepoStatus
 	var r1 *github.Response
 	var r2 error
@@ -160,6 +168,10 @@ func (_c *MockRepositoriesServices_CreateStatus_Call) RunAndReturn(run func(cont
 func (_m *MockRepositoriesServices) Get(ctx context.Context, owner string, repo string) (*github.Repository, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo)
 
+	if len(ret) == 0 {
+		panic("no return value specified for Get")
+	}
+
 	var r0 *github.Repository
 	var r1 *github.Response
 	var r2 error
@@ -225,6 +237,10 @@ func (_c *MockRepositoriesServices_Get_Call) RunAndReturn(run func(context.Conte
 func (_m *MockRepositoriesServices) GetContents(ctx context.Context, owner string, repo string, path string, opts *github.RepositoryContentGetOptions) (*github.RepositoryContent, []*github.RepositoryContent, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, path, opts)
 
+	if len(ret) == 0 {
+		panic("no return value specified for GetContents")
+	}
+
 	var r0 *github.RepositoryContent
 	var r1 []*github.RepositoryContent
 	var r2 *github.Response
@@ -301,6 +317,10 @@ func (_c *MockRepositoriesServices_GetContents_Call) RunAndReturn(run func(conte
 func (_m *MockRepositoriesServices) ListHooks(ctx context.Context, owner string, repo string, opts *github.ListOptions) ([]*github.Hook, *github.Response, error) {
 	ret := _m.Called(ctx, owner, repo, opts)
 
+	if len(ret) == 0 {
+		panic("no return value specified for ListHooks")
+	}
+
 	var r0 []*github.Hook
 	var r1 *github.Response
 	var r2 error

Feedback & Suggestions:

  1. Panic Usage: The addition of panic when no return value is specified can be risky. Panics should be used sparingly as they can crash the program. Consider using a more graceful error handling approach, such as returning an error or logging a warning. This will make the code more robust and easier to debug. 🛑

  2. Error Handling: Instead of panicking, you could return a default value along with an error indicating that no return value was specified. This approach would allow the caller to handle the error appropriately. For example:

    if len(ret) == 0 {
        return nil, nil, fmt.Errorf("no return value specified for CreateHook")
    }
  3. Version Update: The update from mockery v2.37.1 to v2.46.3 is noted. Ensure that this version change is compatible with the rest of your codebase and that any new features or changes in the mockery library are accounted for. 📦

By addressing these points, you can improve the stability and maintainability of your code. Happy coding! 😺


😼 Mergecat review of pkg/checks/kubeconform/validate.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -20,7 +19,7 @@ import (
 
 var tracer = otel.Tracer("pkg/checks/kubeconform")
 
-func getSchemaLocations(ctx context.Context, ctr container.Container, tempRepoPath string) []string {
+func getSchemaLocations(ctr container.Container) []string {
 	cfg := ctr.Config
 
 	locations := []string{
@@ -29,28 +28,13 @@ func getSchemaLocations(ctx context.Context, ctr container.Container, tempRepoPa
 	}
 
 	// schemas configured globally
-	for _, schemasLocation := range cfg.SchemasLocations {
-		if strings.HasPrefix(schemasLocation, "http://") || strings.HasPrefix(schemasLocation, "https://") {
-			locations = append(locations, schemasLocation)
-		} else {
-			if !filepath.IsAbs(schemasLocation) {
-				schemasLocation = filepath.Join(tempRepoPath, schemasLocation)
-			}
-
-			if _, err := os.Stat(schemasLocation); err != nil {
-				log.Warn().
-					Err(err).
-					Str("path", schemasLocation).
-					Msg("schemas location is invalid, skipping")
-			} else {
-				locations = append(locations, schemasLocation)
-			}
-		}
-	}
+	locations = append(locations, cfg.SchemasLocations...)
 
 	for index := range locations {
 		location := locations[index]
+		oldLocation := location
 		if location == "default" || strings.Contains(location, "{{") {
+			log.Debug().Str("location", location).Msg("location requires no processing to be valid")
 			continue
 		}
 
@@ -60,12 +44,14 @@ func getSchemaLocations(ctx context.Context, ctr container.Container, tempRepoPa
 
 		location += "{{ .NormalizedKubernetesVersion }}/{{ .ResourceKind }}{{ .KindSuffix }}.json"
 		locations[index] = location
+
+		log.Debug().Str("old", oldLocation).Str("new", location).Msg("processed schema location")
 	}
 
 	return locations
 }
 
-func argoCdAppValidate(ctx context.Context, ctr container.Container, appName, targetKubernetesVersion, tempRepoPath string, appManifests []string) (msg.Result, error) {
+func argoCdAppValidate(ctx context.Context, ctr container.Container, appName, targetKubernetesVersion string, appManifests []string) (msg.Result, error) {
 	_, span := tracer.Start(ctx, "ArgoCdAppValidate")
 	defer span.End()
 
@@ -92,7 +78,7 @@ func argoCdAppValidate(ctx context.Context, ctr container.Container, appName, ta
 
 	var (
 		outputString    []string
-		schemaLocations = getSchemaLocations(ctx, ctr, tempRepoPath)
+		schemaLocations = getSchemaLocations(ctr)
 	)
 
 	log.Debug().Msgf("cache location: %s", vOpts.Cache)

Feedback & Suggestions:

  1. Security Concerns: By removing the check for whether a schema location is a valid path or URL, you may introduce security vulnerabilities. Ensure that the cfg.SchemasLocations entries are validated to prevent potential path traversal or invalid URL issues. Consider reintroducing validation for schema locations to ensure they are either valid URLs or valid paths.

  2. Logging: The addition of debug logs for schema location processing is a good practice for traceability. However, ensure that these logs do not expose sensitive information. Review the log messages to confirm they are appropriate for the level of detail being logged.

  3. Function Signature Change: The removal of tempRepoPath from the getSchemaLocations function signature simplifies the function call but may limit flexibility if the path is needed for other purposes. Ensure that this change aligns with the overall design and requirements of the application.

  4. Code Clarity: The refactoring has improved code clarity by reducing complexity in getSchemaLocations. However, ensure that the logic for handling schema locations is still clear and maintainable, especially if additional processing is required in the future.

  5. Backward Compatibility: Verify that these changes do not break any existing functionality or assumptions in other parts of the codebase that rely on the previous behavior of getSchemaLocations.

Overall, the refactoring improves code simplicity but requires careful consideration of security and functionality impacts. 🛡️🔍


😼 Mergecat review of pkg/app_watcher/app_watcher.go

@@ -12,11 +12,11 @@ import (
 	informers "github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions/application/v1alpha1"
 	applisters "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1"
 	"github.com/rs/zerolog/log"
+	"github.com/zapier/kubechecks/pkg/appdir"
+	"github.com/zapier/kubechecks/pkg/container"
 	"k8s.io/apimachinery/pkg/util/runtime"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
 
-	"github.com/zapier/kubechecks/pkg/appdir"
 	"github.com/zapier/kubechecks/pkg/config"
 )
 
@@ -34,16 +34,16 @@ type ApplicationWatcher struct {
 //   - kubeCfg is the Kubernetes configuration.
 //   - vcsToArgoMap is the mapping between VCS and Argo applications.
 //   - cfg is the server configuration.
-func NewApplicationWatcher(kubeCfg *rest.Config, vcsToArgoMap appdir.VcsToArgoMap, cfg config.ServerConfig) (*ApplicationWatcher, error) {
-	if kubeCfg == nil {
+func NewApplicationWatcher(ctr container.Container) (*ApplicationWatcher, error) {
+	if ctr.KubeClientSet == nil {
 		return nil, fmt.Errorf("kubeCfg cannot be nil")
 	}
 	ctrl := ApplicationWatcher{
-		applicationClientset: appclientset.NewForConfigOrDie(kubeCfg),
-		vcsToArgoMap:         vcsToArgoMap,
+		applicationClientset: appclientset.NewForConfigOrDie(ctr.KubeClientSet.Config()),
+		vcsToArgoMap:         ctr.VcsToArgoMap,
 	}
 
-	appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, cfg)
+	appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, ctr.Config)
 
 	ctrl.appInformer = appInformer
 	ctrl.appLister = appLister
@@ -152,14 +152,14 @@ func canProcessApp(obj interface{}) (*appv1alpha1.Application, bool) {
 		return nil, false
 	}
 
-	for _, src := range app.Spec.Sources {
+	if src := app.Spec.Source; src != nil {
 		if isGitRepo(src.RepoURL) {
 			return app, true
 		}
 	}
 
-	if app.Spec.Source != nil {
-		if isGitRepo(app.Spec.Source.RepoURL) {
+	for _, src := range app.Spec.Sources {
+		if isGitRepo(src.RepoURL) {
 			return app, true
 		}
 	}

Feedback & Suggestions:

  1. Dependency Management: The import of k8s.io/client-go/rest was removed, but it seems like it might still be needed for appclientset.NewForConfigOrDie. Ensure that the rest.Config is being correctly handled within the container.Container struct. If rest.Config is still required, consider re-adding the import to avoid potential runtime errors. 🛠️

  2. Error Message Update: The error message "kubeCfg cannot be nil" should be updated to reflect the new parameter name, e.g., "KubeClientSet cannot be nil". This will make debugging easier and the error message more accurate. 📝

  3. Logic Change in canProcessApp: The order of checking app.Spec.Source and app.Spec.Sources was swapped. Ensure that this change is intentional and aligns with the desired logic, as it could affect which source is prioritized for processing. 🔄

  4. Code Consistency: The change in the NewApplicationWatcher function signature and logic requires updates to any calling code to pass a container.Container instead of separate parameters. Ensure that all dependent code is updated accordingly to prevent integration issues. 🔗

  5. Documentation Update: The function documentation for NewApplicationWatcher should be updated to reflect the new parameter type (container.Container) and its expected contents. This will help maintain clarity for future developers. 📚


😼 Mergecat review of pkg/events/check.go

@@ -42,7 +42,7 @@ type CheckEvent struct {
 	repoManager repoManager
 	processors  []checks.ProcessorEntry
 	repoLock    sync.Mutex
-	clonedRepos map[string]*git.Repo
+	clonedRepos map[repoKey]*git.Repo
 
 	addedAppsSet     map[string]v1alpha1.Application
 	addedAppsSetLock sync.Mutex
@@ -82,7 +82,7 @@ func NewCheckEvent(pullRequest vcs.PullRequest, ctr container.Container, repoMan
 		addedAppsSet: make(map[string]v1alpha1.Application),
 		appChannel:   make(chan *v1alpha1.Application, ctr.Config.MaxQueueSize),
 		ctr:          ctr,
-		clonedRepos:  make(map[string]*git.Repo),
+		clonedRepos:  make(map[repoKey]*git.Repo),
 		processors:   processors,
 		pullRequest:  pullRequest,
 		repoManager:  repoManager,
@@ -160,15 +160,14 @@ func canonicalize(cloneURL string) (pkg.RepoURL, error) {
 	return parsed, nil
 }
 
-func generateRepoKey(cloneURL pkg.RepoURL, branchName string) string {
-	return fmt.Sprintf("%s|||%s", cloneURL.CloneURL(""), branchName)
-}
+type repoKey string
 
-type hasUsername interface {
-	Username() string
+func generateRepoKey(cloneURL pkg.RepoURL, branchName string) repoKey {
+	key := fmt.Sprintf("%s|||%s", cloneURL.CloneURL(""), branchName)
+	return repoKey(key)
 }
 
-func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneURL, branchName string) (*git.Repo, error) {
+func (ce *CheckEvent) getRepo(ctx context.Context, cloneURL, branchName string) (*git.Repo, error) {
 	var (
 		err  error
 		repo *git.Repo
@@ -181,7 +180,7 @@ func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneU
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to parse clone url")
 	}
-	cloneURL = parsed.CloneURL(vcsClient.Username())
+	cloneURL = parsed.CloneURL(ce.ctr.VcsClient.Username())
 
 	branchName = strings.TrimSpace(branchName)
 	if branchName == "" {
@@ -227,20 +226,36 @@ func (ce *CheckEvent) getRepo(ctx context.Context, vcsClient hasUsername, cloneU
 	return repo, nil
 }
 
+func (ce *CheckEvent) mergeIntoTarget(ctx context.Context, repo *git.Repo, branch string) error {
+	if err := repo.MergeIntoTarget(ctx, fmt.Sprintf("origin/%s", branch)); err != nil {
+		return errors.Wrap(err, "failed to merge into target")
+	}
+
+	parsed, err := canonicalize(repo.CloneURL)
+	if err != nil {
+		return errors.Wrap(err, "failed to canonicalize url")
+	}
+
+	reposKey := generateRepoKey(parsed, branch)
+	ce.clonedRepos[reposKey] = repo
+
+	return nil
+}
+
 func (ce *CheckEvent) Process(ctx context.Context) error {
 	start := time.Now()
 
 	_, span := tracer.Start(ctx, "GenerateListOfAffectedApps")
 	defer span.End()
 
 	// Clone the repo's BaseRef (main, etc.) locally into the temp dir we just made
-	repo, err := ce.getRepo(ctx, ce.ctr.VcsClient, ce.pullRequest.CloneURL, ce.pullRequest.BaseRef)
+	repo, err := ce.getRepo(ctx, ce.pullRequest.CloneURL, ce.pullRequest.BaseRef)
 	if err != nil {
 		return errors.Wrap(err, "failed to clone repo")
 	}
 
 	// Merge the most recent changes into the branch we just cloned
-	if err = repo.MergeIntoTarget(ctx, ce.pullRequest.SHA); err != nil {
+	if err = ce.mergeIntoTarget(ctx, repo, ce.pullRequest.HeadRef); err != nil {
 		return errors.Wrap(err, "failed to merge into target")
 	}
 
@@ -275,11 +290,12 @@ func (ce *CheckEvent) Process(ctx context.Context) error {
 	for num := 0; num <= ce.ctr.Config.MaxConcurrenctChecks; num++ {
 
 		w := worker{
-			appChannel: ce.appChannel,
-			ctr:        ce.ctr,
-			logger:     ce.logger.With().Int("workerID", num).Logger(),
-			processors: ce.processors,
-			vcsNote:    ce.vcsNote,
+			appChannel:  ce.appChannel,
+			ctr:         ce.ctr,
+			logger:      ce.logger.With().Int("workerID", num).Logger(),
+			pullRequest: ce.pullRequest,
+			processors:  ce.processors,
+			vcsNote:     ce.vcsNote,
 
 			done:      ce.wg.Done,
 			getRepo:   ce.getRepo,

Feedback & Suggestions:

  1. Type Safety with repoKey: 🛡️ The change from map[string]*git.Repo to map[repoKey]*git.Repo is a good move for type safety. However, ensure that all usages of keys in this map are consistently using the repoKey type to avoid potential type mismatches.

  2. mergeIntoTarget Function: 🔄 The new mergeIntoTarget function encapsulates the merge logic, which is a good refactor for clarity and reusability. However, consider logging the success of the merge operation for better traceability.

  3. Removed hasUsername Interface: 🚫 The removal of the hasUsername interface simplifies the code, but ensure that the VcsClient always implements the Username method to avoid runtime errors.

  4. Concurrency Control: ⚙️ In the Process function, the loop for creating workers uses <= ce.ctr.Config.MaxConcurrenctChecks. Ensure that MaxConcurrenctChecks is correctly set to avoid creating more workers than intended, which could lead to resource exhaustion.

  5. Error Handling: ⚠️ Consider adding more context to error messages, especially in the mergeIntoTarget function, to make debugging easier.

  6. Code Consistency: 📏 Ensure consistent formatting and naming conventions across the codebase to maintain readability and ease of maintenance.


😼 Mergecat review of pkg/argo_client/applications.go

@@ -24,11 +24,11 @@ var ErrNoVersionFound = errors.New("no kubernetes version found")
 // GetApplicationByName takes a context and a name, then queries the Argo Application client to retrieve the Application with the specified name.
 // It returns the found Application and any error encountered during the process.
 // If successful, the Application client connection is closed before returning.
-func (argo *ArgoClient) GetApplicationByName(ctx context.Context, name string) (*v1alpha1.Application, error) {
+func (a *ArgoClient) GetApplicationByName(ctx context.Context, name string) (*v1alpha1.Application, error) {
 	ctx, span := tracer.Start(ctx, "GetApplicationByName")
 	defer span.End()
 
-	closer, appClient := argo.GetApplicationClient()
+	closer, appClient := a.GetApplicationClient()
 	defer closer.Close()
 
 	resp, err := appClient.Get(ctx, &application.ApplicationQuery{Name: &name})
@@ -43,7 +43,7 @@ func (argo *ArgoClient) GetApplicationByName(ctx context.Context, name string) (
 // GetKubernetesVersionByApplication is a method on the ArgoClient struct that takes a context and an application name as parameters,
 // and returns the Kubernetes version of the destination cluster where the specified application is running.
 // It returns an error if the application or cluster information cannot be retrieved.
-func (argo *ArgoClient) GetKubernetesVersionByApplication(ctx context.Context, app v1alpha1.Application) (string, error) {
+func (a *ArgoClient) GetKubernetesVersionByApplication(ctx context.Context, app v1alpha1.Application) (string, error) {
 	ctx, span := tracer.Start(ctx, "GetKubernetesVersionByApplicationName")
 	defer span.End()
 
@@ -58,7 +58,7 @@ func (argo *ArgoClient) GetKubernetesVersionByApplication(ctx context.Context, a
 	}
 
 	// Get cluster client
-	clusterCloser, clusterClient := argo.GetClusterClient()
+	clusterCloser, clusterClient := a.GetClusterClient()
 	defer clusterCloser.Close()
 
 	// Get cluster
@@ -85,11 +85,11 @@ func (argo *ArgoClient) GetKubernetesVersionByApplication(ctx context.Context, a
 // GetApplicationsByLabels takes a context and a labelselector, then queries the Argo Application client to retrieve the Applications with the specified label.
 // It returns the found ApplicationList and any error encountered during the process.
 // If successful, the Application client connection is closed before returning.
-func (argo *ArgoClient) GetApplicationsByLabels(ctx context.Context, labels string) (*v1alpha1.ApplicationList, error) {
+func (a *ArgoClient) GetApplicationsByLabels(ctx context.Context, labels string) (*v1alpha1.ApplicationList, error) {
 	ctx, span := tracer.Start(ctx, "GetApplicationsByLabels")
 	defer span.End()
 
-	closer, appClient := argo.GetApplicationClient()
+	closer, appClient := a.GetApplicationClient()
 	defer closer.Close()
 
 	resp, err := appClient.List(ctx, &application.ApplicationQuery{Selector: &labels})
@@ -103,31 +103,31 @@ func (argo *ArgoClient) GetApplicationsByLabels(ctx context.Context, labels stri
 
 // GetApplicationsByAppset takes a context and an appset, then queries the Argo Application client to retrieve the Applications managed by the appset
 // It returns the found ApplicationList and any error encountered during the process.
-func (argo *ArgoClient) GetApplicationsByAppset(ctx context.Context, name string) (*v1alpha1.ApplicationList, error) {
+func (a *ArgoClient) GetApplicationsByAppset(ctx context.Context, name string) (*v1alpha1.ApplicationList, error) {
 	appsetLabelSelector := "argocd.argoproj.io/application-set-name=" + name
-	return argo.GetApplicationsByLabels(ctx, appsetLabelSelector)
+	return a.GetApplicationsByLabels(ctx, appsetLabelSelector)
 }
 
-func (argo *ArgoClient) GetApplications(ctx context.Context) (*v1alpha1.ApplicationList, error) {
+func (a *ArgoClient) GetApplications(ctx context.Context) (*v1alpha1.ApplicationList, error) {
 	ctx, span := tracer.Start(ctx, "GetApplications")
 	defer span.End()
 
-	closer, appClient := argo.GetApplicationClient()
+	closer, appClient := a.GetApplicationClient()
 	defer closer.Close()
 
 	resp, err := appClient.List(ctx, new(application.ApplicationQuery))
 	if err != nil {
 		telemetry.SetError(span, err, "Argo List All Applications error")
-		return nil, errors.Wrap(err, "failed to applications")
+		return nil, errors.Wrap(err, "failed to list applications")
 	}
 	return resp, nil
 }
 
-func (argo *ArgoClient) GetApplicationSets(ctx context.Context) (*v1alpha1.ApplicationSetList, error) {
+func (a *ArgoClient) GetApplicationSets(ctx context.Context) (*v1alpha1.ApplicationSetList, error) {
 	ctx, span := tracer.Start(ctx, "GetApplications")
 	defer span.End()
 
-	closer, appClient := argo.GetApplicationSetClient()
+	closer, appClient := a.GetApplicationSetClient()
 	defer closer.Close()
 
 	resp, err := appClient.List(ctx, new(applicationset.ApplicationSetListQuery))

Feedback & Suggestions:

  1. Variable Naming Consistency: The change from argo to a for the receiver variable is a matter of style. While shorter names can be more concise, they may also reduce readability, especially in larger codebases. Consider using a more descriptive name if the context isn't clear.

  2. Error Message Correction: The change from "failed to applications" to "failed to list applications" is a good fix for clarity. Ensure all error messages are clear and descriptive to aid in debugging.

  3. Function Naming in Tracing: The function name in the tracing call for GetApplicationSets is still "GetApplications". Consider updating it to "GetApplicationSets" for consistency and clarity in tracing logs.

  4. Code Readability: While the changes are minor, ensure that the naming conventions and style are consistent throughout the codebase to maintain readability and ease of understanding for future developers.


😼 Mergecat review of pkg/argo_client/client.go

 package argo_client
 
 import (
+	"crypto/tls"
 	"io"
-	"sync"
 
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/application"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/applicationset"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/settings"
+	repoapiclient "github.com/argoproj/argo-cd/v2/reposerver/apiclient"
+	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
+	client "github.com/zapier/kubechecks/pkg/kubernetes"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
 
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
 
@@ -18,10 +25,17 @@ import (
 type ArgoClient struct {
 	client apiclient.Client
 
-	manifestsLock sync.Mutex
+	repoClient         repoapiclient.RepoServerServiceClient
+	namespace          string
+	k8s                kubernetes.Interface
+	k8sConfig          *rest.Config
+	sendFullRepository bool
 }
 
-func NewArgoClient(cfg config.ServerConfig) (*ArgoClient, error) {
+func NewArgoClient(
+	cfg config.ServerConfig,
+	k8s client.Interface,
+) (*ArgoClient, error) {
 	opts := &apiclient.ClientOptions{
 		ServerAddr:      cfg.ArgoCDServerAddr,
 		AuthToken:       cfg.ArgoCDToken,
@@ -42,38 +56,54 @@ func NewArgoClient(cfg config.ServerConfig) (*ArgoClient, error) {
 		return nil, err
 	}
 
+	log.Info().Msg("creating client")
+	tlsConfig := tls.Config{InsecureSkipVerify: cfg.ArgoCDRepositoryInsecure}
+	conn, err := grpc.NewClient(cfg.ArgoCDRepositoryEndpoint,
+		grpc.WithTransportCredentials(
+			credentials.NewTLS(&tlsConfig),
+		),
+	)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to create client")
+	}
+
 	return &ArgoClient{
-		client: argo,
+		repoClient:         repoapiclient.NewRepoServerServiceClient(conn),
+		client:             argo,
+		namespace:          cfg.ArgoCDNamespace,
+		k8s:                k8s.ClientSet(),
+		k8sConfig:          k8s.Config(),
+		sendFullRepository: cfg.ArgoCDSendFullRepository,
 	}, nil
 }
 
 // GetApplicationClient has related argocd diff code https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L899
-func (argo *ArgoClient) GetApplicationClient() (io.Closer, application.ApplicationServiceClient) {
-	closer, appClient, err := argo.client.NewApplicationClient()
+func (a *ArgoClient) GetApplicationClient() (io.Closer, application.ApplicationServiceClient) {
+	closer, appClient, err := a.client.NewApplicationClient()
 	if err != nil {
 		log.Fatal().Err(err).Msg("could not create ArgoCD Application Client")
 	}
 	return closer, appClient
 }
 
-func (argo *ArgoClient) GetApplicationSetClient() (io.Closer, applicationset.ApplicationSetServiceClient) {
-	closer, appClient, err := argo.client.NewApplicationSetClient()
+func (a *ArgoClient) GetApplicationSetClient() (io.Closer, applicationset.ApplicationSetServiceClient) {
+	closer, appClient, err := a.client.NewApplicationSetClient()
 	if err != nil {
 		log.Fatal().Err(err).Msg("could not create ArgoCD Application Set Client")
 	}
 	return closer, appClient
 }
 
-func (argo *ArgoClient) GetSettingsClient() (io.Closer, settings.SettingsServiceClient) {
-	closer, appClient, err := argo.client.NewSettingsClient()
+func (a *ArgoClient) GetSettingsClient() (io.Closer, settings.SettingsServiceClient) {
+	closer, appClient, err := a.client.NewSettingsClient()
 	if err != nil {
 		log.Fatal().Err(err).Msg("could not create ArgoCD Settings Client")
 	}
 	return closer, appClient
 }
 
-func (argo *ArgoClient) GetClusterClient() (io.Closer, cluster.ClusterServiceClient) {
-	closer, clusterClient, err := argo.client.NewClusterClient()
+func (a *ArgoClient) GetClusterClient() (io.Closer, cluster.ClusterServiceClient) {
+	closer, clusterClient, err := a.client.NewClusterClient()
 	if err != nil {
 		log.Fatal().Err(err).Msg("could not create ArgoCD Cluster Client")
 	}

Feedback & Suggestions:

  1. Security Concern with InsecureSkipVerify: The use of InsecureSkipVerify: cfg.ArgoCDRepositoryInsecure in the TLS configuration can expose the application to man-in-the-middle attacks. Consider validating the server's certificate to ensure secure communication. If this is for development purposes, ensure it's not enabled in production. 🔒

  2. Error Handling Improvement: The use of log.Fatal() for error handling in the client methods will terminate the program. Consider returning the error to allow the caller to handle it appropriately, which can improve the robustness of the application. For example, replace log.Fatal().Err(err).Msg("could not create ArgoCD Application Client") with return nil, nil, err and handle the error in the calling function. 🛠️

  3. Use of errors.Wrap: The use of errors.Wrap is a good practice for providing context to errors. Ensure this is consistently applied across all error handling to maintain clarity in logs and debugging. 📜

  4. Namespace and Client Initialization: The addition of namespace, k8s, and k8sConfig fields in ArgoClient is a good enhancement. Ensure that these are properly initialized and used in the methods where applicable. 🏗️

  5. Function Parameter Naming: The parameter k8s in NewArgoClient is a bit ambiguous. Consider renaming it to something more descriptive like k8sClient to improve code readability. 📚


😼 Mergecat review of pkg/container/main.go

@@ -2,13 +2,14 @@ package container
 
 import (
 	"context"
-	"io/fs"
+	"fmt"
 
-	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	"github.com/pkg/errors"
+	"github.com/rs/zerolog/log"
 	client "github.com/zapier/kubechecks/pkg/kubernetes"
+	"github.com/zapier/kubechecks/pkg/vcs/github_client"
+	"github.com/zapier/kubechecks/pkg/vcs/gitlab_client"
 
-	"github.com/zapier/kubechecks/pkg"
-	"github.com/zapier/kubechecks/pkg/app_watcher"
 	"github.com/zapier/kubechecks/pkg/appdir"
 	"github.com/zapier/kubechecks/pkg/argo_client"
 	"github.com/zapier/kubechecks/pkg/config"
@@ -17,35 +18,107 @@ import (
 )
 
 type Container struct {
-	ApplicationWatcher    *app_watcher.ApplicationWatcher
-	ApplicationSetWatcher *app_watcher.ApplicationSetWatcher
-	ArgoClient            *argo_client.ArgoClient
+	ArgoClient *argo_client.ArgoClient
 
 	Config config.ServerConfig
 
 	RepoManager *git.RepoManager
 
 	VcsClient    vcs.Client
-	VcsToArgoMap VcsToArgoMap
+	VcsToArgoMap appdir.VcsToArgoMap
 
 	KubeClientSet client.Interface
 }
 
-type VcsToArgoMap interface {
-	AddApp(*v1alpha1.Application)
-	AddAppSet(*v1alpha1.ApplicationSet)
-	UpdateApp(old, new *v1alpha1.Application)
-	UpdateAppSet(old *v1alpha1.ApplicationSet, new *v1alpha1.ApplicationSet)
-	DeleteApp(*v1alpha1.Application)
-	DeleteAppSet(app *v1alpha1.ApplicationSet)
-	GetVcsRepos() []string
-	GetAppsInRepo(string) *appdir.AppDirectory
-	GetAppSetsInRepo(string) *appdir.AppSetDirectory
-	GetMap() map[pkg.RepoURL]*appdir.AppDirectory
-	WalkKustomizeApps(cloneURL string, fs fs.FS) *appdir.AppDirectory
-}
-
 type ReposCache interface {
 	Clone(ctx context.Context, repoUrl string) (string, error)
 	CloneWithBranch(ctx context.Context, repoUrl, targetBranch string) (string, error)
 }
+
+func New(ctx context.Context, cfg config.ServerConfig) (Container, error) {
+	var err error
+
+	var ctr = Container{
+		Config:      cfg,
+		RepoManager: git.NewRepoManager(cfg),
+	}
+
+	// create vcs client
+	switch cfg.VcsType {
+	case "gitlab":
+		ctr.VcsClient, err = gitlab_client.CreateGitlabClient(cfg)
+	case "github":
+		ctr.VcsClient, err = github_client.CreateGithubClient(cfg)
+	default:
+		err = fmt.Errorf("unknown vcs-type: %q", cfg.VcsType)
+	}
+	if err != nil {
+		return ctr, errors.Wrap(err, "failed to create vcs client")
+	}
+	var kubeClient client.Interface
+
+	switch cfg.KubernetesType {
+	// TODO: expand with other cluster types
+	case client.ClusterTypeLOCAL:
+		kubeClient, err = client.New(&client.NewClientInput{
+			KubernetesConfigPath: cfg.KubernetesConfig,
+			ClusterType:          cfg.KubernetesType,
+		})
+		if err != nil {
+			return ctr, errors.Wrap(err, "failed to create kube client")
+		}
+	case client.ClusterTypeEKS:
+		kubeClient, err = client.New(&client.NewClientInput{
+			KubernetesConfigPath: cfg.KubernetesConfig,
+			ClusterType:          cfg.KubernetesType,
+		},
+			client.EKSClientOption(ctx, cfg.KubernetesClusterID),
+		)
+		if err != nil {
+			return ctr, errors.Wrap(err, "failed to create kube client")
+		}
+	}
+	ctr.KubeClientSet = kubeClient
+	// create argo client
+	if ctr.ArgoClient, err = argo_client.NewArgoClient(cfg, kubeClient); err != nil {
+		return ctr, errors.Wrap(err, "failed to create argo client")
+	}
+
+	// create vcs to argo map
+	vcsToArgoMap := appdir.NewVcsToArgoMap(ctr.VcsClient.Username())
+	ctr.VcsToArgoMap = vcsToArgoMap
+
+	if cfg.MonitorAllApplications {
+		if err = buildAppsMap(ctx, ctr.ArgoClient, ctr.VcsToArgoMap); err != nil {
+			log.Fatal().Err(err).Msg("failed to build apps map")
+		}
+
+		if err = buildAppSetsMap(ctx, ctr.ArgoClient, ctr.VcsToArgoMap); err != nil {
+			log.Fatal().Err(err).Msg("failed to build appsets map")
+		}
+	}
+
+	return ctr, nil
+}
+
+func buildAppsMap(ctx context.Context, argoClient *argo_client.ArgoClient, result appdir.VcsToArgoMap) error {
+	apps, err := argoClient.GetApplications(ctx)
+	if err != nil {
+		return errors.Wrap(err, "failed to list applications")
+	}
+	for _, app := range apps.Items {
+		result.AddApp(&app)
+	}
+	return nil
+}
+
+func buildAppSetsMap(ctx context.Context, argoClient *argo_client.ArgoClient, result appdir.VcsToArgoMap) error {
+	appSets, err := argoClient.GetApplicationSets(ctx)
+	if err != nil {
+		return errors.Wrap(err, "failed to list application sets")
+	}
+	for _, appSet := range appSets.Items {
+		result.AddAppSet(&appSet)
+	}
+	return nil
+}

Feedback & Suggestions:

  1. Error Handling Consistency: The code uses log.Fatal() for error handling in some places, which will terminate the program. Consider using log.Error() or returning the error to allow for more graceful error handling and recovery. 💡

  2. Missing Default Case for KubernetesType: In the switch statement for cfg.KubernetesType, there is no default case. Consider adding a default case to handle unexpected values, which can help prevent potential runtime errors. 🚦

  3. Code Duplication: The creation of the kubeClient in both client.ClusterTypeLOCAL and client.ClusterTypeEKS cases is quite similar. Consider refactoring to reduce code duplication, which can improve maintainability. 🔄

  4. Security Consideration: Ensure that any sensitive information in cfg (like credentials) is not logged or exposed in error messages. This is crucial for maintaining security. 🔒

  5. Function Documentation: Adding comments or documentation for the new functions New, buildAppsMap, and buildAppSetsMap would improve code readability and maintainability. 📚


😼 Mergecat review of pkg/events/check_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 	affectedappsmocks "github.com/zapier/kubechecks/mocks/affected_apps/mocks"
 	generatorsmocks "github.com/zapier/kubechecks/mocks/generator/mocks"
+	vcsmocks "github.com/zapier/kubechecks/mocks/vcs/mocks"
 	"github.com/zapier/kubechecks/pkg/affected_apps"
 	"github.com/zapier/kubechecks/pkg/checks"
 	"github.com/zapier/kubechecks/pkg/config"
@@ -65,12 +66,6 @@ func TestCleanupGetManifestsError(t *testing.T) {
 	}
 }
 
-type mockVcsClient struct{}
-
-func (m mockVcsClient) Username() string {
-	return "username"
-}
-
 func TestCheckEventGetRepo(t *testing.T) {
 	cloneURL := "https://github.com/zapier/kubechecks.git"
 	canonical, err := canonicalize(cloneURL)
@@ -80,12 +75,16 @@ func TestCheckEventGetRepo(t *testing.T) {
 	ctx := context.TODO()
 
 	t.Run("empty branch name", func(t *testing.T) {
+		vcsClient := new(vcsmocks.MockClient)
+		vcsClient.EXPECT().Username().Return("username")
+
 		ce := CheckEvent{
-			clonedRepos: make(map[string]*git.Repo),
+			clonedRepos: make(map[repoKey]*git.Repo),
 			repoManager: git.NewRepoManager(cfg),
+			ctr:         container.Container{VcsClient: vcsClient},
 		}
 
-		repo, err := ce.getRepo(ctx, mockVcsClient{}, cloneURL, "")
+		repo, err := ce.getRepo(ctx, cloneURL, "")
 		require.NoError(t, err)
 		assert.Equal(t, "main", repo.BranchName)
 		assert.Len(t, ce.clonedRepos, 2)
@@ -94,12 +93,16 @@ func TestCheckEventGetRepo(t *testing.T) {
 	})
 
 	t.Run("branch is HEAD", func(t *testing.T) {
+		vcsClient := new(vcsmocks.MockClient)
+		vcsClient.EXPECT().Username().Return("username")
+
 		ce := CheckEvent{
-			clonedRepos: make(map[string]*git.Repo),
+			clonedRepos: make(map[repoKey]*git.Repo),
 			repoManager: git.NewRepoManager(cfg),
+			ctr:         container.Container{VcsClient: vcsClient},
 		}
 
-		repo, err := ce.getRepo(ctx, mockVcsClient{}, cloneURL, "HEAD")
+		repo, err := ce.getRepo(ctx, cloneURL, "HEAD")
 		require.NoError(t, err)
 		assert.Equal(t, "main", repo.BranchName)
 		assert.Len(t, ce.clonedRepos, 2)
@@ -108,12 +111,16 @@ func TestCheckEventGetRepo(t *testing.T) {
 	})
 
 	t.Run("branch is the same as HEAD", func(t *testing.T) {
+		vcsClient := new(vcsmocks.MockClient)
+		vcsClient.EXPECT().Username().Return("username")
+
 		ce := CheckEvent{
-			clonedRepos: make(map[string]*git.Repo),
+			clonedRepos: make(map[repoKey]*git.Repo),
 			repoManager: git.NewRepoManager(cfg),
+			ctr:         container.Container{VcsClient: vcsClient},
 		}
 
-		repo, err := ce.getRepo(ctx, mockVcsClient{}, cloneURL, "main")
+		repo, err := ce.getRepo(ctx, cloneURL, "main")
 		require.NoError(t, err)
 		assert.Equal(t, "main", repo.BranchName)
 		assert.Len(t, ce.clonedRepos, 2)
@@ -122,12 +129,16 @@ func TestCheckEventGetRepo(t *testing.T) {
 	})
 
 	t.Run("branch is not the same as HEAD", func(t *testing.T) {
+		vcsClient := new(vcsmocks.MockClient)
+		vcsClient.EXPECT().Username().Return("username")
+
 		ce := CheckEvent{
-			clonedRepos: make(map[string]*git.Repo),
+			clonedRepos: make(map[repoKey]*git.Repo),
 			repoManager: git.NewRepoManager(cfg),
+			ctr:         container.Container{VcsClient: vcsClient},
 		}
 
-		repo, err := ce.getRepo(ctx, mockVcsClient{}, cloneURL, "gh-pages")
+		repo, err := ce.getRepo(ctx, cloneURL, "gh-pages")
 		require.NoError(t, err)
 		assert.Equal(t, "gh-pages", repo.BranchName)
 		assert.Len(t, ce.clonedRepos, 1)
@@ -145,7 +156,7 @@ func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) {
 		ctr           container.Container
 		repoManager   repoManager
 		processors    []checks.ProcessorEntry
-		clonedRepos   map[string]*git.Repo
+		clonedRepos   map[repoKey]*git.Repo
 		addedAppsSet  map[string]v1alpha1.Application
 		appsSent      int32
 		appChannel    chan *v1alpha1.Application
@@ -308,6 +319,7 @@ func MockGenerator(methodName string, returns []interface{}) generator.AppsGener
 
 	return mockClient
 }
+
 func MockInitMatcherFn() MatcherFn {
 	return func(ce *CheckEvent, repo *git.Repo) error {
 		return nil

Feedback & Suggestions:

  1. Mocking Improvements: The diff introduces a more sophisticated mocking strategy by using vcsmocks.MockClient instead of a simple mockVcsClient struct. This is a good improvement as it allows for more flexible and detailed mock behavior. However, ensure that the vcsmocks.MockClient is properly initialized and that all expected methods are mocked to avoid runtime errors. 🛠️

  2. Type Consistency: The change from map[string]*git.Repo to map[repoKey]*git.Repo is a positive step towards type safety and clarity. Ensure that repoKey is well-defined and used consistently across the codebase to prevent any type-related issues. 🔑

  3. Code Readability: The removal of the mockVcsClient struct and its replacement with a more dynamic mock setup improves the readability and maintainability of the test code. This change makes it easier to understand the test setup and expected interactions. 📚

  4. Dependency Injection: The addition of ctr: container.Container{VcsClient: vcsClient} in the CheckEvent struct is a good practice for dependency injection, making the code more modular and testable. Ensure that all dependencies are correctly injected and utilized. 🔄

  5. Mock Expectations: The use of EXPECT().Username().Return("username") is a good practice to explicitly define the expected behavior of the mock. Make sure that all necessary expectations are set to avoid unexpected behavior during test execution. 🧪

Overall, the changes improve the testability and maintainability of the code. Just ensure that all mock expectations are correctly set and that the new types are consistently used throughout the codebase. 👍


😼 Mergecat review of pkg/events/worker.go

@@ -3,15 +3,18 @@ package events
 import (
 	"context"
 	"fmt"
+	"runtime/debug"
 	"sync/atomic"
 
+	"github.com/ghodss/yaml"
+	"github.com/rs/zerolog/log"
+	"github.com/zapier/kubechecks/pkg/vcs"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 
 	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	"github.com/rs/zerolog"
 	"github.com/zapier/kubechecks/pkg"
-	"github.com/zapier/kubechecks/pkg/argo_client"
 	"github.com/zapier/kubechecks/pkg/checks"
 	"github.com/zapier/kubechecks/pkg/container"
 	"github.com/zapier/kubechecks/pkg/git"
@@ -20,14 +23,15 @@ import (
 )
 
 type worker struct {
-	appChannel chan *v1alpha1.Application
-	ctr        container.Container
-	logger     zerolog.Logger
-	processors []checks.ProcessorEntry
-	vcsNote    *msg.Message
+	appChannel  chan *v1alpha1.Application
+	ctr         container.Container
+	logger      zerolog.Logger
+	processors  []checks.ProcessorEntry
+	pullRequest vcs.PullRequest
+	vcsNote     *msg.Message
 
 	done                func()
-	getRepo             func(ctx context.Context, vcsClient hasUsername, cloneURL, branchName string) (*git.Repo, error)
+	getRepo             func(ctx context.Context, cloneURL, branchName string) (*git.Repo, error)
 	queueApp, removeApp func(application v1alpha1.Application)
 }
 
@@ -54,91 +58,86 @@ func (w *worker) processApp(ctx context.Context, app v1alpha1.Application) {
 	var (
 		err error
 
-		appName    = app.Name
-		appSrc     = app.Spec.Source
-		appPath    = appSrc.Path
-		appRepoUrl = appSrc.RepoURL
+		appName = app.Name
 
-		logger = w.logger.With().
-			Str("app_name", appName).
-			Str("app_path", appPath).
-			Logger()
+		rootLogger = w.logger.With().
+				Str("app_name", appName).
+				Logger()
 	)
 
 	ctx, span := tracer.Start(ctx, "processApp", trace.WithAttributes(
 		attribute.String("app", appName),
-		attribute.String("dir", appPath),
 	))
 	defer span.End()
 
 	atomic.AddInt32(&inFlight, 1)
 	defer atomic.AddInt32(&inFlight, -1)
 
-	logger.Info().Msg("Processing app")
+	rootLogger.Info().Msg("Processing app")
 
 	// Build a new section for this app in the parent comment
 	w.vcsNote.AddNewApp(ctx, appName)
 
 	defer func() {
-		if err := recover(); err != nil {
+		if r := recover(); r != nil {
 			desc := fmt.Sprintf("panic while checking %s", appName)
-			w.logger.Error().Str("app", appName).Msgf("panic while running check")
+			w.logger.Error().Any("error", r).
+				Str("app", appName).Msgf("panic while running check")
+			println(string(debug.Stack()))
 
-			telemetry.SetError(span, fmt.Errorf("%v", err), "panic while running check")
+			telemetry.SetError(span, fmt.Errorf("%v", r), "panic while running check")
 			result := msg.Result{
 				State:   pkg.StatePanic,
 				Summary: desc,
-				Details: fmt.Sprintf(errorCommentFormat, desc, err),
+				Details: fmt.Sprintf(errorCommentFormat, desc, r),
 			}
 			w.vcsNote.AddToAppMessage(ctx, appName, result)
 		}
 	}()
 
-	repo, err := w.getRepo(ctx, w.ctr.VcsClient, appRepoUrl, appSrc.TargetRevision)
+	rootLogger.Debug().Msg("Getting manifests")
+	jsonManifests, err := w.ctr.ArgoClient.GetManifests(ctx, appName, app, w.pullRequest, w.getRepo)
 	if err != nil {
-		logger.Error().Err(err).Msg("Unable to clone repository")
-		w.vcsNote.AddToAppMessage(ctx, appName, msg.Result{
-			State:   pkg.StateError,
-			Summary: "failed to clone repo",
-			Details: fmt.Sprintf("Clone URL: `%s`\nTarget Revision: `%s`\n```\n%s\n```", appRepoUrl, appSrc.TargetRevision, err.Error()),
-		})
-		return
-	}
-	repoPath := repo.Directory
-
-	logger.Debug().Str("repo_path", repoPath).Msg("Getting manifests")
-	jsonManifests, err := w.ctr.ArgoClient.GetManifestsLocal(ctx, appName, repoPath, appPath, app)
-	if err != nil {
-		logger.Error().Err(err).Msg("Unable to get manifests")
+		rootLogger.Error().Err(err).Msg("Unable to get manifests")
 		w.vcsNote.AddToAppMessage(ctx, appName, msg.Result{
 			State:   pkg.StateError,
 			Summary: "Unable to get manifests",
-			Details: fmt.Sprintf("```\n%s\n```", cleanupGetManifestsError(err, repo.Directory)),
+			Details: fmt.Sprintf("```\n%s\n```", err),
 		})
 		return
 	}
 
 	// Argo diff logic wants unformatted manifests but everything else wants them as YAML, so we prepare both
-	yamlManifests := argo_client.ConvertJsonToYamlManifests(jsonManifests)
-	logger.Trace().Msgf("Manifests:\n%+v\n", yamlManifests)
+	yamlManifests := convertJsonToYamlManifests(jsonManifests)
+	rootLogger.Trace().Msgf("Manifests:\n%+v\n", yamlManifests)
 
 	k8sVersion, err := w.ctr.ArgoClient.GetKubernetesVersionByApplication(ctx, app)
 	if err != nil {
-		logger.Error().Err(err).Msg("Error retrieving the Kubernetes version")
+		rootLogger.Error().Err(err).Msg("Error retrieving the Kubernetes version")
 		k8sVersion = w.ctr.Config.FallbackK8sVersion
 	} else {
 		k8sVersion = fmt.Sprintf("%s.0", k8sVersion)
-		logger.Info().Msgf("Kubernetes version: %s", k8sVersion)
+		rootLogger.Info().Msgf("Kubernetes version: %s", k8sVersion)
 	}
 
-	runner := newRunner(
-		w.ctr, app, repo, appName, k8sVersion, jsonManifests, yamlManifests, logger, w.vcsNote,
-		w.queueApp, w.removeApp,
-	)
+	runner := newRunner(w.ctr, app, appName, k8sVersion, jsonManifests, yamlManifests, rootLogger, w.vcsNote, w.queueApp, w.removeApp)
 
 	for _, processor := range w.processors {
 		runner.Run(ctx, processor.Name, processor.Processor, processor.WorstState)
 	}
 
 	runner.Wait()
 }
+
+func convertJsonToYamlManifests(jsonManifests []string) []string {
+	var manifests []string
+	for _, manifest := range jsonManifests {
+		ret, err := yaml.JSONToYAML([]byte(manifest))
+		if err != nil {
+			log.Warn().Err(err).Msg("Failed to format manifest")
+			continue
+		}
+		manifests = append(manifests, fmt.Sprintf("---\n%s", string(ret)))
+	}
+	return manifests
+}

Feedback & Suggestions:

  1. Error Handling with recover: The addition of runtime/debug to print stack traces is a good move for debugging. However, using println for logging the stack trace is not ideal. Consider using the logger to capture the stack trace for consistency and better log management. 📜

  2. Logging Consistency: The change from logger to rootLogger is a good step towards consistent logging. Ensure that all log messages use rootLogger to maintain uniformity. 📝

  3. Function Signature Change: The removal of vcsClient from getRepo function signature is a significant change. Ensure that this does not break any existing functionality that relies on vcsClient. Consider adding comments to explain the rationale behind this change. 🔍

  4. Manifest Conversion: The new convertJsonToYamlManifests function is a useful addition. However, consider handling the error more gracefully rather than just logging a warning. You might want to aggregate errors and handle them after the loop, depending on the criticality of the conversion. ⚠️

  5. Dependency Management: The removal of argo_client import suggests a refactor. Ensure that all dependencies are correctly managed and that the new implementation of GetManifests is thoroughly tested. 🔄

  6. Security Considerations: Ensure that any sensitive information is not logged, especially when printing stack traces or errors. Consider sanitizing logs if necessary. 🔒


😼 Mergecat review of pkg/argo_client/manifests.go

 package argo_client
 
 import (
+	"bufio"
 	"context"
 	"fmt"
+	"io"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
 	"time"
 
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
+	"github.com/argoproj/argo-cd/v2/pkg/apiclient/project"
 	"github.com/argoproj/argo-cd/v2/pkg/apiclient/settings"
-	argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	repoapiclient "github.com/argoproj/argo-cd/v2/reposerver/apiclient"
-	"github.com/argoproj/argo-cd/v2/reposerver/repository"
-	"github.com/argoproj/argo-cd/v2/util/git"
-	"github.com/ghodss/yaml"
+	"github.com/argoproj/argo-cd/v2/util/argo"
+	"github.com/argoproj/argo-cd/v2/util/db"
+	argosettings "github.com/argoproj/argo-cd/v2/util/settings"
+	"github.com/argoproj/argo-cd/v2/util/tgzstream"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
-	"k8s.io/apimachinery/pkg/api/resource"
-
-	"github.com/zapier/kubechecks/telemetry"
+	"github.com/zapier/kubechecks/pkg"
+	"github.com/zapier/kubechecks/pkg/git"
+	"github.com/zapier/kubechecks/pkg/vcs"
 )
 
-func (argo *ArgoClient) GetManifestsLocal(ctx context.Context, name, tempRepoDir, changedAppFilePath string, app argoappv1.Application) ([]string, error) {
-	var err error
+type getRepo func(ctx context.Context, cloneURL string, branchName string) (*git.Repo, error)
 
-	ctx, span := tracer.Start(ctx, "GetManifestsLocal")
+func (a *ArgoClient) GetManifests(ctx context.Context, name string, app v1alpha1.Application, pullRequest vcs.PullRequest, getRepo getRepo) ([]string, error) {
+	ctx, span := tracer.Start(ctx, "GetManifests")
 	defer span.End()
 
-	log.Debug().Str("name", name).Msg("GetManifestsLocal")
+	log.Debug().Str("name", name).Msg("GetManifests")
 
 	start := time.Now()
 	defer func() {
 		duration := time.Since(start)
 		getManifestsDuration.WithLabelValues(name).Observe(duration.Seconds())
 	}()
 
-	clusterCloser, clusterClient := argo.GetClusterClient()
-	defer clusterCloser.Close()
+	contentRefs, refs := preprocessSources(&app, pullRequest)
 
-	settingsCloser, settingsClient := argo.GetSettingsClient()
-	defer settingsCloser.Close()
+	var manifests []string
+	for _, source := range contentRefs {
+		moreManifests, err := a.generateManifests(ctx, app, source, refs, pullRequest, getRepo)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to generate manifests")
+		}
+		manifests = append(manifests, moreManifests...)
+	}
+
+	getManifestsSuccess.WithLabelValues(name).Inc()
+	return manifests, nil
+}
+
+// preprocessSources splits the content sources from the ref sources, and transforms source refs that point at the pull
+// request's base into refs that point at the pull request's head. This is necessary to generate manifests based on what
+// the world will look like _after_ the branch gets merged in.
+func preprocessSources(app *v1alpha1.Application, pullRequest vcs.PullRequest) ([]v1alpha1.ApplicationSource, []v1alpha1.ApplicationSource) {
+	if !app.Spec.HasMultipleSources() {
+		return []v1alpha1.ApplicationSource{app.Spec.GetSource()}, nil
+	}
+
+	// collect all ref sources, map by name
+	var contentSources []v1alpha1.ApplicationSource
+	var refSources []v1alpha1.ApplicationSource
+
+	for _, source := range app.Spec.Sources {
+		if source.Ref == "" {
+			contentSources = append(contentSources, source)
+			continue
+		}
+
+		/*
+			This is to make sure that the respository server understands where to pull the values.yaml file from.
+
+			Or put differently:
+
+			| PR Repo   | PR Base     | PR Target | Ref Repo  | Ref Target |                                                                                                 |
+			| --------- | ----------- | --------- | --------- | ---------- | ----------------------------------------------------------------------------------------------- |
+			| repo1.git | new-feature | main      | repo1.git | main       | need to change main to new-feature for preview, as the base will become the target after merge. |
+			| repo1.git | new-feature | main      | repo2.git | main       | no change, ref source refers to a different repository unaffected by the pull request           |
+			| repo1.git | new-feature | main      | repo1.git | staging    | no change, ref source refers to a different branch than the pull request                        |
+		*/
+		if pkg.AreSameRepos(source.RepoURL, pullRequest.CloneURL) {
+			if source.TargetRevision == pullRequest.BaseRef {
+				source.TargetRevision = pullRequest.HeadRef
+			}
+		}
+
+		refSources = append(refSources, source)
+	}
+
+	return contentSources, refSources
+}
+
+// generateManifests generates an Application along with all of its files, and sends it to the ArgoCD
+// Repository service to be transformed into raw kubernetes manifests. This allows us to take advantage of server
+// configuration and credentials.
+func (a *ArgoClient) generateManifests(ctx context.Context, app v1alpha1.Application, source v1alpha1.ApplicationSource, refs []v1alpha1.ApplicationSource, pullRequest vcs.PullRequest, getRepo func(ctx context.Context, cloneURL string, branchName string) (*git.Repo, error)) ([]string, error) {
+	// The GenerateManifestWithFiles has some non-obvious rules due to assumptions that it makes:
+	// 1. first source must be a non-ref source
+	// 2. there must be one and only one non-ref source
+	// 3. ref sources that match the pull requests' repo and target branch need to have their target branch swapped to the head branch of the pull request
+
+	clusterCloser, clusterClient := a.GetClusterClient()
+	defer clusterCloser.Close()
 
-	log.Debug().
-		Str("clusterName", app.Spec.Destination.Name).
-		Str("clusterServer", app.Spec.Destination.Server).
-		Msg("getting cluster")
 	cluster, err := clusterClient.Get(ctx, &cluster.ClusterQuery{Name: app.Spec.Destination.Name, Server: app.Spec.Destination.Server})
 	if err != nil {
-		telemetry.SetError(span, err, "Argo Get Cluster")
-		getManifestsFailed.WithLabelValues(name).Inc()
+		getManifestsFailed.WithLabelValues(app.Name).Inc()
 		return nil, errors.Wrap(err, "failed to get cluster")
 	}
 
+	settingsCloser, settingsClient := a.GetSettingsClient()
+	defer settingsCloser.Close()
+
+	log.Info().Msg("get settings")
 	argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{})
 	if err != nil {
-		telemetry.SetError(span, err, "Argo Get Settings")
-		getManifestsFailed.WithLabelValues(name).Inc()
+		getManifestsFailed.WithLabelValues(app.Name).Inc()
 		return nil, errors.Wrap(err, "failed to get settings")
 	}
 
-	log.Debug().Str("name", name).Msg("generating diff for application...")
-	res, err := argo.generateManifests(ctx, fmt.Sprintf("%s/%s", tempRepoDir, changedAppFilePath), tempRepoDir, app, argoSettings, cluster)
+	settingsMgr := argosettings.NewSettingsManager(ctx, a.k8s, a.namespace)
+	argoDB := db.NewDB(a.namespace, settingsMgr, a.k8s)
+
+	repoTarget := source.TargetRevision
+	if pkg.AreSameRepos(source.RepoURL, pullRequest.CloneURL) && areSameTargetRef(source.TargetRevision, pullRequest.BaseRef) {
+		repoTarget = pullRequest.HeadRef
+	}
+
+	log.Info().Msg("get repo")
+	repo, err := getRepo(ctx, source.RepoURL, repoTarget)
 	if err != nil {
-		telemetry.SetError(span, err, "Generate Manifests")
-		return nil, errors.Wrap(err, "failed to generate manifests")
+		return nil, errors.Wrap(err, "failed to get repo")
 	}
 
-	if res.Manifests == nil {
-		return nil, nil
+	var packageDir string
+	if a.sendFullRepository {
+		log.Info().Msg("sending full repository")
+		packageDir = repo.Directory
+	} else {
+		log.Info().Msg("packaging app")
+		packageDir, err = packageApp(ctx, source, refs, repo, getRepo)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to package application")
+		}
+	}
+
+	log.Info().Msg("compressing files")
+	f, filesWritten, checksum, err := tgzstream.CompressFiles(packageDir, []string{"*"}, []string{".git"})
+	if err != nil {
+		return nil, fmt.Errorf("failed to compress files: %w", err)
+	}
+	log.Info().Msgf("%d files compressed", filesWritten)
+	//if filesWritten == 0 {
+	//	return nil, fmt.Errorf("no files to send")
+	//}
+
+	closer, projectClient, err := a.client.NewProjectClient()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get project client")
+	}
+	defer closer.Close()
+
+	proj, err := projectClient.Get(ctx, &project.ProjectQuery{Name: app.Spec.Project})
+	if err != nil {
+		return nil, fmt.Errorf("error getting app project: %w", err)
+	}
+
+	helmRepos, err := argoDB.ListHelmRepositories(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("error listing helm repositories: %w", err)
+	}
+	permittedHelmRepos, err := argo.GetPermittedRepos(proj, helmRepos)
+	if err != nil {
+		return nil, fmt.Errorf("error retrieving permitted repos: %w", err)
+	}
+	helmRepositoryCredentials, err := argoDB.GetAllHelmRepositoryCredentials(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("error getting helm repository credentials: %w", err)
+	}
+	helmOptions, err := settingsMgr.GetHelmSettings()
+	if err != nil {
+		return nil, fmt.Errorf("error getting helm settings: %w", err)
+	}
+	permittedHelmCredentials, err := argo.GetPermittedReposCredentials(proj, helmRepositoryCredentials)
+	if err != nil {
+		return nil, fmt.Errorf("error getting permitted repos credentials: %w", err)
+	}
+	enabledSourceTypes, err := settingsMgr.GetEnabledSourceTypes()
+	if err != nil {
+		return nil, fmt.Errorf("error getting settings enabled source types: %w", err)
+	}
+
+	refSources, err := argo.GetRefSources(context.Background(), app.Spec.Sources, app.Spec.Project, argoDB.GetRepository, []string{}, false)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get ref sources: %w", err)
+	}
+
+	app.Spec.Sources = append([]v1alpha1.ApplicationSource{source}, refs...)
+
+	q := repoapiclient.ManifestRequest{
+		Repo:               &v1alpha1.Repository{Repo: source.RepoURL},
+		Revision:           source.TargetRevision,
+		AppLabelKey:        argoSettings.AppLabelKey,
+		AppName:            app.Name,
+		Namespace:          app.Spec.Destination.Namespace,
+		ApplicationSource:  &source,
+		Repos:              permittedHelmRepos,
+		KustomizeOptions:   argoSettings.KustomizeOptions,
+		KubeVersion:        cluster.Info.ServerVersion,
+		ApiVersions:        cluster.Info.APIVersions,
+		HelmRepoCreds:      permittedHelmCredentials,
+		HelmOptions:        helmOptions,
+		TrackingMethod:     argoSettings.TrackingMethod,
+		EnabledSourceTypes: enabledSourceTypes,
+		ProjectName:        proj.Name,
+		ProjectSourceRepos: proj.Spec.SourceRepos,
+		HasMultipleSources: app.Spec.HasMultipleSources(),
+		RefSources:         refSources,
+	}
+
+	log.Info().Msg("generating manifest with files")
+	stream, err := a.repoClient.GenerateManifestWithFiles(ctx)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get manifests with files")
 	}
-	getManifestsSuccess.WithLabelValues(name).Inc()
-	return res.Manifests, nil
-}
 
-func (argo *ArgoClient) generateManifests(
-	ctx context.Context, appPath, tempRepoDir string, app argoappv1.Application, argoSettings *settings.Settings, cluster *argoappv1.Cluster,
-) (*repoapiclient.ManifestResponse, error) {
-	argo.manifestsLock.Lock()
-	defer argo.manifestsLock.Unlock()
-
-	source := app.Spec.GetSource()
-
-	return repository.GenerateManifests(
-		ctx,
-		appPath,
-		tempRepoDir,
-		source.TargetRevision,
-		&repoapiclient.ManifestRequest{
-			Repo:              &argoappv1.Repository{Repo: source.RepoURL},
-			AppLabelKey:       argoSettings.AppLabelKey,
-			AppName:           app.Name,
-			Namespace:         app.Spec.Destination.Namespace,
-			ApplicationSource: &source,
-			KustomizeOptions:  argoSettings.KustomizeOptions,
-			KubeVersion:       cluster.Info.ServerVersion,
-			ApiVersions:       cluster.Info.APIVersions,
-			TrackingMethod:    argoSettings.TrackingMethod,
+	log.Info().Msg("sending request")
+	if err := stream.Send(&repoapiclient.ManifestRequestWithFiles{
+		Part: &repoapiclient.ManifestRequestWithFiles_Request{
+			Request: &q,
 		},
-		true,
-		new(git.NoopCredsStore),
-		resource.MustParse("0"),
-		nil,
-	)
+	}); err != nil {
+		return nil, errors.Wrap(err, "failed to send request")
+	}
+
+	log.Info().Msg("sending metadata")
+	if err := stream.Send(&repoapiclient.ManifestRequestWithFiles{
+		Part: &repoapiclient.ManifestRequestWithFiles_Metadata{
+			Metadata: &repoapiclient.ManifestFileMetadata{
+				Checksum: checksum,
+			},
+		},
+	}); err != nil {
+		return nil, errors.Wrap(err, "failed to send metadata")
+	}
+
+	log.Info().Msg("sending file")
+	err = sendFile(ctx, stream, f)
+	if err != nil {
+		return nil, fmt.Errorf("failed to send manifest stream file: %w", err)
+	}
+
+	log.Info().Msg("receiving repsonse")
+	response, err := stream.CloseAndRecv()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get response")
+	}
+
+	log.Info().Msg("done!")
+	return response.Manifests, nil
 }
 
-func ConvertJsonToYamlManifests(jsonManifests []string) []string {
-	var manifests []string
-	for _, manifest := range jsonManifests {
-		ret, err := yaml.JSONToYAML([]byte(manifest))
+func copyFile(srcpath, dstpath string) error {
+	dstdir := filepath.Dir(dstpath)
+	if err := os.MkdirAll(dstdir, 0o777); err != nil {
+		return errors.Wrap(err, "failed to make directories")
+	}
+
+	r, err := os.Open(srcpath)
+	if err != nil {
+		return err
+	}
+	defer r.Close() // ignore error: file was opened read-only.
+
+	w, err := os.Create(dstpath)
+	if err != nil {
+		return err
+	}
+
+	defer func() {
+		// Report the error, if any, from Close, but do so
+		// only if there isn't already an outgoing error.
+		if c := w.Close(); err == nil {
+			err = c
+		}
+	}()
+
+	_, err = io.Copy(w, r)
+	return err
+}
+
+func packageApp(ctx context.Context, source v1alpha1.ApplicationSource, refs []v1alpha1.ApplicationSource, repo *git.Repo, getRepo getRepo) (string, error) {
+	tempDir, err := os.MkdirTemp("", "package-*")
+	if err != nil {
+		return "", errors.Wrap(err, "failed to make temp dir")
+	}
+
+	tempAppDir := filepath.Join(tempDir, source.Path)
+	appPath := filepath.Join(repo.Directory, source.Path)
+
+	// copy app files to the temp dir
+	if err = filepath.Walk(appPath, func(path string, info fs.FileInfo, err error) error {
 		if err != nil {
-			log.Warn().Err(err).Msg("Failed to format manifest")
-			continue
+			return err
+		}
+
+		if info.IsDir() {
+			return nil
+		}
+
+		relPath, err := filepath.Rel(appPath, path)
+		if err != nil {
+			return errors.Wrapf(err, "failed to calculate rel between %q and %q", appPath, path)
+		}
+		src :=

</details>

---

## Dependency Review
<details><summary>Click to read mergecats review!</summary>

No suggestions found
</details>

@djeebus djeebus mentioned this pull request Dec 16, 2024
@djeebus djeebus merged commit 839119d into main Dec 16, 2024
6 checks passed
@djeebus djeebus deleted the multi-source-apps branch December 16, 2024 18:07
@MeNsaaH MeNsaaH mentioned this pull request Dec 17, 2024
@loadstar81
Copy link

appreciate your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants