add linting, clean up a bunch of code #289
Conversation
|
Temporary image available at |
# Conflicts: # pkg/events/check.go # pkg/server/hook_handler.go # pkg/vcs/github_client/client.go # pkg/vcs/github_client/client_test.go # pkg/vcs/gitlab_client/client.go
Mergecat's ReviewClick to read mergecats review!😼 Mergecat review of pkg/vcs/repo.go-// PullRequest represents an PR/MR
+// PullRequest represents an PR/MR.Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. While this change improves consistency in comment punctuation, it does not introduce any security or performance issues. However, ensure that all comments throughout the codebase follow a consistent style for clarity and professionalism. 📝 😼 Mergecat review of pkg/vcs/types.go-// Client represents a VCS client
+// Client represents a VCS client.Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. While this is a minor stylistic improvement for consistency in documentation, it doesn't impact functionality or performance. Ensure that all comments throughout the codebase follow a consistent style for clarity and professionalism. 😼 Mergecat review of pkg/vcs/client.go- // ErrInvalidType is a sentinel error for use in client implementations
+ // ErrInvalidType is a sentinel error for use in client implementations.Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. This is a minor stylistic change and does not impact functionality or performance. However, it's a good practice to maintain consistency in comment punctuation throughout the codebase. Ensure that all comments follow the same style for clarity and professionalism. 👍 😼 Mergecat review of pkg/affected_apps/multi_matcher.go@@ -4,6 +4,7 @@ import (
"context"
"github.com/pkg/errors"
+
"github.com/zapier/kubechecks/pkg/git"
)
Feedback & Suggestions: The diff introduces an unnecessary blank line between the import statements. While this does not affect functionality, it can be considered a style issue. It's generally a good practice to keep related import statements together without extra spacing unless separating standard library imports from third-party imports. Consider removing the extra line to maintain a cleaner and more consistent code style. 🧹 😼 Mergecat review of pkg/vcs/gitlab_client/project.go-// GetProjectByID gets a project by the given Project Name or ID
+// GetProjectByID gets a project by the given Project Name or ID.Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. While this is a minor stylistic improvement for consistency in documentation, there are no functional or security implications to address. If this change is part of a broader effort to standardize comment formatting, it is a positive step. However, ensure that such changes are consistent across the entire codebase to maintain uniformity. 📝 😼 Mergecat review of pkg/repoUrl.go@@ -5,7 +5,7 @@ import (
"net/url"
"strings"
- "github.com/chainguard-dev/git-urls"
+ giturls "github.com/chainguard-dev/git-urls"
)
type RepoURL struct {Feedback & Suggestions: The change in the import statement is purely stylistic and does not affect functionality. However, if the alias 😼 Mergecat review of pkg/vcs/gitlab_client/pipeline.go@@ -17,7 +17,6 @@ func (c *Client) GetPipelinesForCommit(projectName string, commitSHA string) ([]
}
return pipelines, nil
-
}
func (c *Client) GetLastPipelinesForCommit(projectName string, commitSHA string) *gitlab.PipelineInfo {Feedback & Suggestions: The diff removes an unnecessary newline, which is a minor change. While this doesn't impact functionality, maintaining consistent formatting can improve code readability. Ensure that any changes to formatting are consistent with the project's style guide. If the newline was removed to adhere to a specific style, that's great! Otherwise, consider whether such changes are necessary to avoid unnecessary diffs in version control. 📝 😼 Mergecat review of pkg/repoUrl_test.go@@ -5,9 +5,8 @@ import (
"net/url"
"testing"
- "github.com/stretchr/testify/require"
-
"github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
)
func TestNormalizeStrings(t *testing.T) {Feedback & Suggestions:
😼 Mergecat review of pkg/config/config.go@@ -40,7 +40,7 @@ type ServerConfig struct {
VcsToken string `mapstructure:"vcs-token"`
VcsType string `mapstructure:"vcs-type"`
- //github
+ // github
GithubPrivateKey string `mapstructure:"github-private-key"`
GithubAppID int64 `mapstructure:"github-app-id"`
GithubInstallationID int64 `mapstructure:"github-installation-id"`Feedback & Suggestions: The change made in the diff is purely cosmetic, correcting the comment to include a space after the 😼 Mergecat review of cmd/flags_test.go@@ -45,7 +45,6 @@ func TestStringUsages(t *testing.T) {
for testName, test := range tests {
t.Run(testName, func(t *testing.T) {
-
actual := generateUsage(test.opt, test.usage, test.name)
assert.Equal(t, test.expected, actual)
})Feedback & Suggestions: The diff only removes an unnecessary blank line, which is a minor formatting change. While this change does not introduce any issues, it also does not significantly improve the code. Consider whether this change is necessary, as it may not impact readability or functionality. If the goal is to maintain a consistent style, ensure that similar formatting is applied throughout the codebase. 🧹 😼 Mergecat review of cmd/locations.go- if err := repo.Update(ctx); err != nil {
+ if err := repo.Update(); err != nil {Feedback & Suggestions:
😼 Mergecat review of pkg/config/config_test.go@@ -23,8 +23,8 @@ func TestNew(t *testing.T) {
cfg, err := NewWithViper(v)
require.NoError(t, err)
assert.Equal(t, zerolog.InfoLevel, cfg.LogLevel)
- assert.Equal(t, true, cfg.ArgoCDInsecure)
- assert.Equal(t, true, cfg.ArgoCDPlainText)
+ assert.True(t, cfg.ArgoCDInsecure)
+ assert.True(t, cfg.ArgoCDPlainText)
assert.Equal(t, pkg.StateWarning, cfg.WorstConfTestState, "worst states can be overridden")
assert.Equal(t, time.Minute*10, cfg.RepoRefreshInterval)
}Feedback & Suggestions:
😼 Mergecat review of telemetry/helpers.go@@ -44,7 +44,7 @@ func GetTraceID(ctx context.Context) string {
return strconv.FormatUint(traceIDToUint64(tID), 10)
}
-// traceIDToUint64 converts 128bit traceId to 64 bit uint64
+// traceIDToUint64 converts 128bit traceId to 64 bit uint64.
func traceIDToUint64(b [16]byte) uint64 {
return binary.BigEndian.Uint64(b[len(b)-8:])
}Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. While this is a minor stylistic change, it is generally good practice to ensure consistency in comment punctuation throughout the codebase. If other comments in the codebase end with a period, this change aligns with that style. Otherwise, consider updating all comments for consistency. 📝 😼 Mergecat review of pkg/vcs/gitlab_client/emoji.go-// ToEmoji returns a string representation of this state for use in the request
+// ToEmoji returns a string representation of this state for use in the request.Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. This is a minor stylistic change and does not impact functionality, security, or performance. No further improvements are necessary. 👍 😼 Mergecat review of pkg/checks/hooks/check_test.go@@ -7,6 +7,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+
"github.com/zapier/kubechecks/pkg"
"github.com/zapier/kubechecks/pkg/checks"
)Feedback & Suggestions: The diff introduces an extra blank line between the import statements. While this doesn't affect functionality, it is generally unnecessary and can be removed to maintain a cleaner code style. Consistent formatting helps in maintaining readability and organization of the code. Consider removing the extra line unless it serves a specific purpose in your coding standards. 😼 Mergecat review of pkg/git/repo_test.go@@ -14,6 +14,8 @@ import (
)
func wipe(t *testing.T, path string) {
+ t.Helper()
+
err := os.RemoveAll(path)
require.NoError(t, err)
}Feedback & Suggestions:
Overall, the diff improves the test code by making it easier to debug. Keep up the good work! 🚀 😼 Mergecat review of pkg/generator/value_interpolation.go@@ -12,7 +12,6 @@ func appendTemplatedValues(values map[string]string, params map[string]interface
for key, value := range values {
result, err := replaceTemplatedString(value, params, useGoTemplate, goTemplateOptions)
-
if err != nil {
return fmt.Errorf("failed to replace templated string: %w", err)
}Feedback & Suggestions: The diff removes a blank line, which is a minor change and does not affect functionality or performance. However, removing such lines can sometimes reduce readability, especially if they were used to separate logical blocks of code. Consider whether the removal of this line impacts the readability of the code. If it does, it might be worth keeping the line for clarity. 🧐 😼 Mergecat review of pkg/vcs/github_client/emoji.go-// ToEmoji returns a string representation of this state for use in the request
+// ToEmoji returns a string representation of this state for use in the request.Feedback & Suggestions: The only change in this diff is the addition of a period at the end of the comment. This is a minor stylistic change and does not impact functionality, security, or performance. Ensure that this change aligns with the project's documentation style guide. If consistency in comment punctuation is a goal, this change is appropriate. Otherwise, it may be unnecessary. ✍️ 😼 Mergecat review of cmd/controller_cmd.go@@ -22,7 +22,7 @@ import (
"github.com/zapier/kubechecks/telemetry"
)
-// ControllerCmd represents the run command
+// ControllerCmd represents the run command.
var ControllerCmd = &cobra.Command{
Use: "controller",
Short: "Start the VCS Webhook handler.",
@@ -67,7 +67,7 @@ var ControllerCmd = &cobra.Command{
if err != nil {
log.Panic().Err(err).Msg("Failed to initialize telemetry")
}
- defer t.Shutdown()
+ defer t.Shutdown(ctx)
log.Info().Msgf("starting web server")
startWebserver(ctx, ctr, processors)Feedback & Suggestions:
Overall, these changes improve the code's robustness and maintainability. Keep up the good work! 🚀 😼 Mergecat review of pkg/msg/message.go@@ -49,7 +49,7 @@ type toEmoji interface {
// Message type that allows concurrent updates
// Has a reference to the owner/repo (ie zapier/kubechecks),
-// the PR/MR id, and the actual messsage
+// the PR/MR id, and the actual messsage.
type Message struct {
Name string
Owner string
@@ -141,7 +141,7 @@ func (m *Message) buildFooter(start time.Time, commitSHA, labelFilter string, sh
return fmt.Sprintf("<small> _Done: Pod: %s, Dur: %v, SHA: %s%s_ <small>\n", hostname, duration, pkg.GitCommit, envStr)
}
-// BuildComment iterates the map of all apps in this message, building a final comment from their current state
+// BuildComment iterates the map of all apps in this message, building a final comment from their current state.
func (m *Message) BuildComment(ctx context.Context, start time.Time, commitSHA, labelFilter string, showDebugInfo bool) string {
_, span := tracer.Start(ctx, "buildComment")
defer span.End()Feedback & Suggestions: The changes made in the diff are purely cosmetic, adding a period at the end of comments. While this improves consistency in comment style, it does not affect functionality or performance. Ensure that all comments throughout the codebase follow this style for uniformity. No further action is required. 📝 😼 Mergecat review of pkg/container/main.go@@ -5,14 +5,14 @@ import (
"io/fs"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
- client "github.com/zapier/kubechecks/pkg/kubernetes"
"github.com/zapier/kubechecks/pkg"
"github.com/zapier/kubechecks/pkg/app_watcher"
"github.com/zapier/kubechecks/pkg/appdir"
"github.com/zapier/kubechecks/pkg/argo_client"
"github.com/zapier/kubechecks/pkg/config"
"github.com/zapier/kubechecks/pkg/git"
+ client "github.com/zapier/kubechecks/pkg/kubernetes"
"github.com/zapier/kubechecks/pkg/vcs"
)
Feedback & Suggestions: The only change in this diff is the reordering of the import statement for the 😼 Mergecat review of pkg/vcs/gitlab_client/status.go@@ -82,9 +82,8 @@ func (c *Client) setCommitStatus(projectWithNS string, commitSHA string, status
return commitStatus, err
}
-// configureBackOff returns a backoff configuration to use to retry requests
+// configureBackOff returns a backoff configuration to use to retry requests.
func configureBackOff() *backoff.ExponentialBackOff {
-
// Lets setup backoff logic to retry this request for 30 seconds
expBackOff := backoff.NewExponentialBackOff()
expBackOff.MaxInterval = 10 * time.SecondFeedback & Suggestions:
Overall, the changes are minor and focused on style improvements. Keep up the attention to detail! 🛠️ 😼 Mergecat review of pkg/repo_config/loader.go@@ -1,7 +1,6 @@
package repo_config
import (
- "fmt"
"os"
"path/filepath"
@@ -73,7 +72,7 @@ func LoadRepoConfigBytes(b []byte) (*Config, error) {
cfg := &Config{}
err := yaml.Unmarshal(b, cfg)
if err != nil {
- return nil, fmt.Errorf("could not parse Project config file (.kubechecks.yaml): %v", err)
+ return nil, errors.Wrap(err, "could not parse Project config file (.kubechecks.yaml)")
}
if err := validate.Validate(cfg); err != nil {Feedback & Suggestions:
Overall, the changes improve the consistency and cleanliness of the code. Keep up the good work! 🚀 😼 Mergecat review of pkg/checks/hooks/check.go@@ -135,7 +135,7 @@ func phasesAndWaves(obj *unstructured.Unstructured) ([]hookInfo, error) {
}
for hookType := range hookTypes(obj) {
- hookInfos = append(hookInfos, hookInfo{hookType: hookType, hookWave: waveNum(syncWave)})
+ hookInfos = append(hookInfos, hookInfo{hookType: hookType, hookWave: waveNum(syncWave)}) //nolint:gosec
}
return hookInfos, nilFeedback & Suggestions:
😼 Mergecat review of pkg/affected_apps/argocd_matcher.go@@ -5,6 +5,7 @@ import (
"os"
"github.com/rs/zerolog/log"
+
"github.com/zapier/kubechecks/pkg/appdir"
"github.com/zapier/kubechecks/pkg/container"
"github.com/zapier/kubechecks/pkg/git"Feedback & Suggestions: The diff introduces an extra blank line between the import statements. While this change does not affect functionality, it is generally unnecessary and could be considered a style inconsistency. It is advisable to maintain a consistent style throughout the codebase. Consider removing the extra blank line unless it serves a specific purpose for readability or grouping related imports. 😼 Mergecat review of pkg/appdir/app_directory.go@@ -63,7 +63,7 @@ func (d *AppDirectory) ProcessApp(app v1alpha1.Application) {
//
// changeList: a slice of strings representing the paths of modified files.
// targetBranch: the branch name to compare against the target revision of the applications.
-// e.g. changeList = ["path/to/file1", "path/to/file2"]
+// e.g. changeList = ["path/to/file1", "path/to/file2"].
func (d *AppDirectory) FindAppsBasedOnChangeList(changeList []string, targetBranch string) []v1alpha1.Application {
log.Debug().Msgf("checking %d changes", len(changeList))
Feedback & Suggestions: The only change in this diff is the addition of a period at the end of a comment line. While this change improves the consistency of the comment style, it does not impact the functionality or performance of the code. If the goal is to maintain a consistent comment style throughout the codebase, ensure that all comments follow this format. However, from a code functionality perspective, there are no issues with this change. 👍 😼 Mergecat review of hacks/env-to-docs.go@@ -21,8 +21,10 @@ type option struct {
Default string
}
-var UsageEnvVar = regexp.MustCompile(` \(KUBECHECKS_[_A-Z0-9]+\)`)
-var UsageDefaultValue = regexp.MustCompile(`Defaults to \.?(.*)+\.`)
+var (
+ UsageEnvVar = regexp.MustCompile(` \(KUBECHECKS_[_A-Z0-9]+\)`)
+ UsageDefaultValue = regexp.MustCompile(`Defaults to \.?(.*)+\.`)
+)
func main() {
outputFilename := filepath.Join("docs", "usage.md")Feedback & Suggestions:
Overall, this is a positive change for code clarity and maintainability. Keep up the good work! 🚀 😼 Mergecat review of pkg/affected_apps/matcher.go@@ -5,6 +5,7 @@ import (
"path"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+
"github.com/zapier/kubechecks/pkg/git"
)
@@ -53,7 +54,7 @@ type Matcher interface {
}
// modifiedDirs filters a list of changed files down to a list
-// the unique dirs containing the changed files
+// the unique dirs containing the changed files.
func modifiedDirs(changeList []string) []string {
dirMap := map[string]bool{}
for _, file := range changeList {Feedback & Suggestions:
No security or performance issues were introduced in this diff. Keep up the good work! 👍 😼 Mergecat review of pkg/affected_apps/multi_matcher_test.go@@ -6,8 +6,9 @@ import (
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/stretchr/testify/require"
- "github.com/zapier/kubechecks/pkg/git"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ "github.com/zapier/kubechecks/pkg/git"
)
type fakeMatcher struct {Feedback & Suggestions:
😼 Mergecat review of pkg/appdir/vcstoargomap.go@@ -5,6 +5,7 @@ import (
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/rs/zerolog/log"
+
"github.com/zapier/kubechecks/pkg"
)
Feedback & Suggestions: The only change in this diff is the addition of a blank line between import statements. While this does not introduce any functional issues, it is generally unnecessary unless it is part of a specific style guide or convention being followed in the project. If there is no such guideline, consider removing the extra line to maintain a cleaner and more consistent code style. 🧹 😼 Mergecat review of pkg/server/server.go@@ -2,7 +2,6 @@ package server
import (
"context"
- "fmt"
"net/url"
"strings"
@@ -53,9 +52,9 @@ func (s *Server) Start(ctx context.Context) {
ghHooks := NewVCSHookHandler(s.ctr, s.processors)
ghHooks.AttachHandlers(hooksGroup)
- fmt.Println("Method\tPath")
+ log.Debug().Msg("Method\tPath")
for _, r := range e.Routes() {
- fmt.Printf("%s\t%s\n", r.Method, r.Path)
+ log.Warn().Msgf("%s\t%s\n", r.Method, r.Path)
}
if err := e.Start(":8080"); err != nil {Feedback & Suggestions:
😼 Mergecat review of pkg/appdir/appset_directory.go@@ -8,8 +8,9 @@ import (
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/rs/zerolog/log"
- "github.com/zapier/kubechecks/pkg/git"
"sigs.k8s.io/yaml"
+
+ "github.com/zapier/kubechecks/pkg/git"
)
type AppSetDirectory struct {
@@ -172,7 +173,7 @@ func (d *AppSetDirectory) RemoveApp(app v1alpha1.ApplicationSet) {
}
}
-// containsKindApplicationSet checks if the file contains kind: ApplicationSet
+// containsKindApplicationSet checks if the file contains kind: ApplicationSet.
func containsKindApplicationSet(path string) bool {
file, err := os.Open(path)
if err != nil {Feedback & Suggestions:
Overall, these changes are minor and mostly stylistic. Ensure that such changes align with the project's coding standards or style guide. 😼 Mergecat review of pkg/commitState.go@@ -5,10 +5,10 @@ import (
"strings"
)
-// CommitState is an enum for represnting the state of a commit for posting via CommitStatus
+// CommitState is an enum for represnting the state of a commit for posting via CommitStatus.
type CommitState uint8
-// must be in order of best to worst, in order for WorstState to work
+// must be in order of best to worst, in order for WorstState to work.
const (
StateNone CommitState = iota
StateSkip
@@ -67,6 +67,5 @@ func ParseCommitState(s string) (CommitState, error) {
return StateSkip, nil
default:
return StateNone, fmt.Errorf("unknown commit state: %s", s)
-
}
}Feedback & Suggestions:
😼 Mergecat review of pkg/kubernetes/api_client.go@@ -10,7 +10,7 @@ import (
controllerClient "sigs.k8s.io/controller-runtime/pkg/client"
)
-// ClusterTypes must match with the cmd/root.go kubernetes-type flag
+// ClusterTypes must match with the cmd/root.go kubernetes-type flag.
const (
ClusterTypeEKS = "eks"
ClusterTypeLOCAL = "local"
@@ -29,7 +29,6 @@ type NewClientInput struct {
// New creates new Kubernetes clients with the specified options.
func New(input *NewClientInput, opts ...NewClientOption) (Interface, error) {
-
var (
k8sConfig *rest.Config
err error
@@ -50,7 +49,6 @@ func New(input *NewClientInput, opts ...NewClientOption) (Interface, error) {
return nil, err
}
}
-
}
input.restConfig = k8sConfigFeedback & Suggestions:
😼 Mergecat review of pkg/aisummary/openai_client.go@@ -17,8 +17,10 @@ type OpenAiClient struct {
enabled bool
}
-var openAiClient *OpenAiClient
-var once sync.Once
+var (
+ openAiClient *OpenAiClient
+ once sync.Once
+)
func GetOpenAiClient(apiToken string) *OpenAiClient {
once.Do(func() {
@@ -35,7 +37,7 @@ func GetOpenAiClient(apiToken string) *OpenAiClient {
}
func createCompletionRequest(model, appName string, prompt string, content string, prefix string) openai.ChatCompletionRequest {
- var summarizeRequest = openai.ChatCompletionRequest{
+ summarizeRequest := openai.ChatCompletionRequest{
Model: model,
MaxTokens: 500,
Temperature: 0.4,Feedback & Suggestions:
Overall, the diff improves code readability and adheres to Go idiomatic practices. Keep up the good work! 🐱💻 😼 Mergecat review of cmd/version.go@@ -1,8 +1,7 @@
package cmd
import (
- "fmt"
-
+ "github.com/rs/zerolog/log"
"github.com/spf13/cobra"
"github.com/zapier/kubechecks/pkg"
@@ -13,7 +12,7 @@ var versionCmd = &cobra.Command{
Short: "List version information",
Long: ``,
Run: func(cmd *cobra.Command, args []string) {
- fmt.Printf("kubechecks\nVersion:%s\nSHA%s\n", pkg.GitTag, pkg.GitCommit)
+ log.Info().Msgf("kubechecks\nVersion:%s\nSHA%s\n", pkg.GitTag, pkg.GitCommit)
},
}
Feedback & Suggestions:
😼 Mergecat review of pkg/checks/rego/check_test.go@@ -18,7 +18,9 @@ import (
)
func mustWrite(t *testing.T, filePath, content string) {
- err := os.WriteFile(filePath, []byte(content), 0o666)
+ t.Helper()
+
+ err := os.WriteFile(filePath, []byte(content), 0o600)
require.NoError(t, err)
}
@@ -149,8 +151,9 @@ warn[msg] {
}
for _, tc := range testcases {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+
policiesPath, err := os.MkdirTemp("", "kubechecks-test-policies-")
require.NoError(t, err)
Feedback & Suggestions:
Overall, these changes enhance the security, readability, and performance of the test code. Keep up the good work! 🚀 😼 Mergecat review of pkg/aisummary/diff_summary.go@@ -2,8 +2,8 @@ package aisummary
import (
"context"
- "fmt"
+ "github.com/rs/zerolog/log"
"github.com/sashabaranov/go-openai"
"go.opentelemetry.io/otel"
@@ -34,12 +34,11 @@ func (c *OpenAiClient) SummarizeDiff(ctx context.Context, appName, diff string)
resp, err := c.makeCompletionRequestWithBackoff(ctx, req)
if err != nil {
telemetry.SetError(span, err, "ChatCompletionStream error")
- fmt.Printf("ChatCompletionStream error: %v\n", err)
+ log.Debug().Err(err).Msg("ChatCompletionStream")
return "", err
}
return resp.Choices[0].Message.Content, nil
-
}
return "", nil
}Feedback & Suggestions:
😼 Mergecat review of pkg/generator/interface.go@@ -23,5 +23,7 @@ type Generator interface {
GetTemplate(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) *argoprojiov1alpha1.ApplicationSetTemplate
}
-var EmptyAppSetGeneratorError = errors.New("ApplicationSet is empty")
-var NoRequeueAfter time.Duration
+var (
+ EmptyAppSetGeneratorError = errors.New("ApplicationSet is empty")
+ NoRequeueAfter time.Duration
+)Feedback & Suggestions:
😼 Mergecat review of pkg/app_watcher/app_watcher.go@@ -20,7 +20,7 @@ import (
"github.com/zapier/kubechecks/pkg/config"
)
-// ApplicationWatcher is the controller that watches ArgoCD Application resources via the Kubernetes API
+// ApplicationWatcher is the controller that watches ArgoCD Application resources via the Kubernetes API.
type ApplicationWatcher struct {
applicationClientset appclientset.Interface
appInformer cache.SharedIndexInformer
@@ -68,7 +68,7 @@ func (ctrl *ApplicationWatcher) Run(ctx context.Context, processors int) {
}
// onAdd is the function executed when the informer notifies the
-// presence of a new Application in the namespace
+// presence of a new Application in the namespace.
func (ctrl *ApplicationWatcher) onApplicationAdded(obj interface{}) {
app, ok := canProcessApp(obj)
if !ok {
@@ -99,7 +99,6 @@ func (ctrl *ApplicationWatcher) onApplicationUpdated(old, new interface{}) {
log.Info().Str("key", key).Msg("appwatcher: onApplicationUpdated")
ctrl.vcsToArgoMap.UpdateApp(old.(*appv1alpha1.Application), new.(*appv1alpha1.Application))
}
-
}
func (ctrl *ApplicationWatcher) onApplicationDeleted(obj interface{}) {Feedback & Suggestions:
Overall, these changes are minor but contribute to a cleaner and more consistent codebase. Keep up the good work! 🚀 😼 Mergecat review of pkg/appdir/app_directory_test.go@@ -107,6 +107,8 @@ func TestRemoveFromSlice(t *testing.T) {
ints := []int{1, 2, 3, 4, 5}
intsAfterRemoval := []int{1, 2, 4, 5}
intsTest := func(t *testing.T) {
+ t.Helper()
+
result := removeFromSlice(ints, 3, func(a, b int) bool { return a == b })
if !reflect.DeepEqual(result, intsAfterRemoval) {
t.Errorf("Expected %v, got %v", intsAfterRemoval, result)
@@ -117,6 +119,8 @@ func TestRemoveFromSlice(t *testing.T) {
strings := []string{"apple", "banana", "cherry", "date"}
stringsAfterRemoval := []string{"apple", "cherry", "date"}
stringsTest := func(t *testing.T) {
+ t.Helper()
+
result := removeFromSlice(strings, "banana", func(a, b string) bool { return a == b })
if !reflect.DeepEqual(result, stringsAfterRemoval) {
t.Errorf("Expected %v, got %v", stringsAfterRemoval, result)Feedback & Suggestions:
😼 Mergecat review of pkg/generator/generator_spec_processor_test.go@@ -4,15 +4,13 @@ import (
"context"
"testing"
+ argov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+ corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
- argov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-
- corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
kubefake "k8s.io/client-go/kubernetes/fake"
crtclient "sigs.k8s.io/controller-runtime/pkg/client"Feedback & Suggestions:
Overall, the changes seem to be focused on improving the organization of the import statements, which is a positive step towards cleaner code. Keep up the good work! 👍 😼 Mergecat review of pkg/argo_client/client.go@@ -7,11 +7,10 @@ import (
"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/cluster"
"github.com/argoproj/argo-cd/v2/pkg/apiclient/settings"
"github.com/rs/zerolog/log"
- "github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
-
"github.com/zapier/kubechecks/pkg/config"
)
Feedback & Suggestions: The diff removes a duplicate import of the 😼 Mergecat review of cmd/container.go@@ -6,6 +6,7 @@ import (
"github.com/pkg/errors"
"github.com/rs/zerolog/log"
+
"github.com/zapier/kubechecks/pkg/app_watcher"
"github.com/zapier/kubechecks/pkg/appdir"
"github.com/zapier/kubechecks/pkg/argo_client"
@@ -20,7 +21,7 @@ import (
func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool) (container.Container, error) {
var err error
- var ctr = container.Container{
+ ctr := container.Container{
Config: cfg,
RepoManager: git.NewRepoManager(cfg),
}
@@ -30,7 +31,7 @@ func newContainer(ctx context.Context, cfg config.ServerConfig, watchApps bool)
case "gitlab":
ctr.VcsClient, err = gitlab_client.CreateGitlabClient(cfg)
case "github":
- ctr.VcsClient, err = github_client.CreateGithubClient(cfg)
+ ctr.VcsClient, err = github_client.CreateGithubClient(ctx, cfg)
default:
err = fmt.Errorf("unknown vcs-type: %q", cfg.VcsType)
}Feedback & Suggestions:
Overall, the changes improve the code's readability and maintainability. Keep up the good work! 🚀 😼 Mergecat review of pkg/generator/matrix_test.go@@ -4,18 +4,16 @@ import (
"testing"
"time"
+ argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
-
- argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)
func TestMatrixGenerate(t *testing.T) {
-
listGenerator := &argoprojiov1alpha1.ListGenerator{
Elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "Cluster","url": "Url", "templated": "test-{{path.basenameNormalized}}"}`)}},
}
@@ -127,7 +125,6 @@ func TestMatrixGenerate(t *testing.T) {
}
func TestMatrixGenerateGoTemplate(t *testing.T) {
-
listGenerator := &argoprojiov1alpha1.ListGenerator{
Elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "Cluster","url": "Url"}`)}},
}Feedback & Suggestions:
Overall, these changes are minor but contribute to a cleaner codebase. Keep up the good work! 👍 😼 Mergecat review of pkg/affected_apps/argocd_matcher_test.go@@ -38,6 +38,6 @@ func TestFindAffectedAppsWithNilAppsDirectory(t *testing.T) {
// verify results
require.NoError(t, err)
- require.Len(t, items.Applications, 0)
- require.Len(t, items.ApplicationSets, 0)
+ require.Empty(t, items.Applications)
+ require.Empty(t, items.ApplicationSets)
}Feedback & Suggestions:
Overall, this is a positive change for code clarity and consistency. Keep up the good work! 🎉 😼 Mergecat review of pkg/checks/preupgrade/kubepug.go@@ -40,14 +40,14 @@ func checkApp(ctx context.Context, appName, targetKubernetesVersion string, mani
tempDir, err := os.MkdirTemp("/tmp", "kubechecks-kubepug")
if err != nil {
logger.Error().Err(err).Msg("could not create temp directory to write manifests for kubepug check")
- //return "", err
+ // return "", err
return msg.Result{}, err
}
defer os.RemoveAll(tempDir)
for i, manifest := range manifests {
tmpFile := fmt.Sprintf("%s/%b.yaml", tempDir, i)
- if err = os.WriteFile(tmpFile, []byte(manifest), 0666); err != nil {
+ if err = os.WriteFile(tmpFile, []byte(manifest), 0o600); err != nil {
logger.Error().Err(err).Str("path", tmpFile).Msg("failed to write file")
}
}
@@ -71,7 +71,6 @@ func checkApp(ctx context.Context, appName, targetKubernetesVersion string, mani
}
if len(result.DeprecatedAPIs) > 0 || len(result.DeletedAPIs) > 0 {
-
if len(result.DeprecatedAPIs) > 0 {
outputString = append(outputString, "\n\n**Deprecated APIs**\n")
buff := &bytes.Buffer{}
@@ -117,7 +116,6 @@ func checkApp(ctx context.Context, appName, targetKubernetesVersion string, mani
table.Render()
outputString = append(outputString, buff.String())
}
-
} else {
outputString = append(outputString, "No Deprecated or Deleted APIs found.")
}Feedback & Suggestions:
Overall, the changes made in the diff enhance the security and readability of the code. Keep up the good work! 🚀 😼 Mergecat review of pkg/kubernetes/api_eks_client.go@@ -12,10 +12,10 @@ import (
"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/service/eks"
+ "github.com/aws/aws-sdk-go-v2/service/sts"
smithyhttp "github.com/aws/smithy-go/transport/http"
+ "github.com/pkg/errors"
"k8s.io/client-go/rest"
-
- "github.com/aws/aws-sdk-go-v2/service/sts"
)
const (
@@ -143,7 +143,7 @@ func (t *TokenRefresher) refreshToken(ctx context.Context) error {
if time.Now().After(t.tokenExpiration) {
token, err := getEKSToken(ctx, t.awsConfig, t.clusterID)
if err != nil {
- return fmt.Errorf("unable to refresh EKS token, %v", err)
+ return errors.Wrap(err, "unable to refresh EKS token")
}
t.token = *token
@@ -165,7 +165,7 @@ type transportWrapper struct {
refresher *TokenRefresher
}
-// RoundTrip will perform the refrsh token before each request
+// RoundTrip will perform the refrsh token before each request.
func (t *transportWrapper) RoundTrip(req *http.Request) (*http.Response, error) {
if err := t.refresher.refreshToken(req.Context()); err != nil {
// log the error and continue with the requestFeedback & Suggestions:
😼 Mergecat review of pkg/vcs/github_client/client.go@@ -33,24 +33,22 @@ type Client struct {
username, email string
}
-// GClient is a struct that holds the services for the GitHub client
+// GClient is a struct that holds the services for the GitHub client.
type GClient struct {
PullRequests PullRequestsServices
Repositories RepositoriesServices
Issues IssuesServices
}
// CreateGithubClient creates a new GitHub client using the auth token provided. We
-// can't validate the token at this point, so if it exists we assume it works
-func CreateGithubClient(cfg config.ServerConfig) (*Client, error) {
+// can't validate the token at this point, so if it exists we assume it works.
+func CreateGithubClient(ctx context.Context, cfg config.ServerConfig) (*Client, error) {
var (
err error
googleClient *github.Client
shurcoolClient *githubv4.Client
)
- ctx := context.Background()
-
githubClient, err := createHttpClient(ctx, cfg)
if err != nil {
return nil, errors.Wrap(err, "failed to create github http client")
@@ -216,7 +214,7 @@ func (c *Client) buildRepoFromEvent(event *github.PullRequestEvent) vcs.PullRequ
return c.buildRepo(event.PullRequest)
}
-// buildRepoFromComment builds a vcs.PullRequest from a github.IssueCommentEvent
+// buildRepoFromComment builds a vcs.PullRequest from a github.IssueCommentEvent.
func (c *Client) buildRepoFromComment(context context.Context, comment *github.IssueCommentEvent) (vcs.PullRequest, error) {
owner := comment.GetRepo().GetOwner().GetLogin()
repoName := comment.GetRepo().GetName()Feedback & Suggestions:
Overall, the changes made in the diff are beneficial for code clarity and maintainability. Keep up the good work! 🎉 😼 Mergecat review of pkg/repo_config/loader_test.go@@ -101,7 +101,7 @@ func Test_loadProjectConfigFile(t *testing.T) {
return
}
- assert.Equal(t, got, tt.want, "Configs are not the same.")
+ assert.Equal(t, tt.want, got, "Configs are not the same.")
})
}
}
@@ -163,7 +163,6 @@ func TestLoadRepoConfig(t *testing.T) {
want *Config
wantErr bool
}{
-
{
name: "yaml",
args: args{
@@ -251,7 +250,7 @@ func TestLoadRepoConfig(t *testing.T) {
t.Errorf("LoadRepoConfig() error = %v, wantErr %v", err, tt.wantErr)
return
}
- assert.Equal(t, got, tt.want, "Configs are not the same.")
+ assert.Equal(t, tt.want, got, "Configs are not the same.")
})
}
}Feedback & Suggestions:
Overall, these changes improve the clarity and correctness of the test assertions. Keep up the good work! 🚀 😼 Mergecat review of pkg/appdir/walk_kustomize_files_test.go@@ -41,7 +41,8 @@ resources:
- ./overlays/dev
- /common/overlays/prod
- https://google.com/some/url
-`)},
+`),
+ },
"test/app2/kustomization.yaml": {
Data: toBytes(`
@@ -55,14 +56,16 @@ resources:
- file1.yaml
- ../overlays/base
- /common/overlays/prod
-`)},
+`),
+ },
"test/overlays/base/kustomization.yaml": {
Data: toBytes(`
resources:
- some-file1.yaml
- some-file2.yaml
- ../common
-`)},
+`),
+ },
"test/overlays/common/kustomization.yaml": {Data: toBytes("hello: world")},
"test/app/file1.yaml": {Data: toBytes("hello: world")},Feedback & Suggestions:
😼 Mergecat review of pkg/affected_apps/config_matcher.go@@ -9,10 +9,10 @@ import (
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/pkg/errors"
"github.com/rs/zerolog/log"
- "github.com/zapier/kubechecks/pkg/git"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"github.com/zapier/kubechecks/pkg/container"
+ "github.com/zapier/kubechecks/pkg/git"
"github.com/zapier/kubechecks/pkg/repo_config"
)
@@ -123,13 +123,12 @@ func (b *ConfigMatcher) applicationsForDir(dir string) []*repo_config.ArgoCdAppl
continue
}
}
-
}
return apps
}
-// appsFromApplicationSetForDir: Get the list of apps managed by an applicationset from dir
+// appsFromApplicationSetForDir: Get the list of apps managed by an applicationset from dir.
func (b *ConfigMatcher) appsFromApplicationSetForDir(ctx context.Context, dir string) ([]*repo_config.ArgocdApplicationSetConfig, []*repo_config.ArgoCdApplicationConfig, error) {
var appsets []*repo_config.ArgocdApplicationSetConfig
for _, appset := range b.cfg.ApplicationSets {
@@ -177,20 +176,22 @@ func dirMatchForApp(changeDir, appDir string) bool {
return false
}
-// Any files modified under appset subdirectories assumes the appset is modified
+// Any files modified under appset subdirectories assumes the appset is modified.
func dirMatchForAppSet(changeDir, appSetDir string) bool {
// normalize dir for matching
appSetDir = path.Clean(appSetDir)
changeDir = path.Clean(changeDir)
log.Debug().Msgf("appSetDir: %s; changeDir: %s", appSetDir, changeDir)
- if strings.HasSuffix(changeDir, appSetDir) {
+ switch {
+ case strings.HasSuffix(changeDir, appSetDir):
return true
- } else if strings.HasPrefix(changeDir, appSetDir) {
+ case strings.HasPrefix(changeDir, appSetDir):
return true
- } else if changeDir == "." && appSetDir == "/" {
+ case changeDir == "." && appSetDir == "/":
return true
}
+
return false
}Feedback & Suggestions:
Overall, these changes improve the code's readability and maintainability. Keep up the good work! 🚀 😼 Mergecat review of pkg/events/worker.go@@ -5,11 +5,11 @@ import (
"fmt"
"sync/atomic"
+ "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+ "github.com/rs/zerolog"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
- "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
- "github.com/rs/zerolog"
"github.com/zapier/kubechecks/pkg"
"github.com/zapier/kubechecks/pkg/argo_client"
"github.com/zapier/kubechecks/pkg/checks"
@@ -31,7 +31,7 @@ type worker struct {
queueApp, removeApp func(application v1alpha1.Application)
}
-// process apps
+// process apps.
func (w *worker) run(ctx context.Context) {
for app := range w.appChannel {
if app != nil {Feedback & Suggestions:
Overall, these changes are minor and mostly stylistic. Ensure that any style guide changes are documented and communicated to the team to maintain consistency. 📝 😼 Mergecat review of cmd/locations_test.go@@ -40,7 +40,6 @@ func TestMaybeCloneGitUrl_NonGitUrl(t *testing.T) {
}
for _, tc := range testcases {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
fc := &fakeCloner{result: nil, err: nil}
actual, err := maybeCloneGitUrl(ctx, fc, time.Duration(0), tc.input, testUsername)
@@ -52,13 +51,13 @@ func TestMaybeCloneGitUrl_NonGitUrl(t *testing.T) {
}
}
-const testRoot = "/tmp/path"
-const testUsername = "username"
+const (
+ testRoot = "/tmp/path"
+ testUsername = "username"
+)
func TestMaybeCloneGitUrl_HappyPath(t *testing.T) {
- var (
- ctx = context.TODO()
- )
+ ctx := context.TODO()
type expected struct {
path, cloneUrl, branch string
@@ -134,7 +133,6 @@ func TestMaybeCloneGitUrl_HappyPath(t *testing.T) {
}
for _, tc := range testcases {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
fc := &fakeCloner{result: &git.Repo{Directory: testRoot}, err: nil}
actual, err := maybeCloneGitUrl(ctx, fc, time.Duration(0), tc.input, testUsername)
@@ -147,9 +145,7 @@ func TestMaybeCloneGitUrl_HappyPath(t *testing.T) {
}
func TestMaybeCloneGitUrl_URLError(t *testing.T) {
- var (
- ctx = context.TODO()
- )
+ ctx := context.TODO()
testcases := []struct {
name, input, expected string
@@ -162,7 +158,6 @@ func TestMaybeCloneGitUrl_URLError(t *testing.T) {
}
for _, tc := range testcases {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
fc := &fakeCloner{result: &git.Repo{Directory: testRoot}, err: nil}
result, err := maybeCloneGitUrl(ctx, fc, time.Duration(0), tc.input, testUsername)
@@ -186,7 +181,6 @@ func TestMaybeCloneGitUrl_CloneError(t *testing.T) {
}
for _, tc := range testcases {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
ctx := context.Background()
ctx, cancel := context.WithCancel(ctx)Feedback & Suggestions:
Overall, these changes improve the code's readability and maintainability without affecting its functionality. Great job! 🎉 😼 Mergecat review of pkg/generator/generator_spec_processor.go@@ -4,16 +4,12 @@ import (
"fmt"
"reflect"
- "github.com/jeremywohl/flatten"
-
"github.com/argoproj/argo-cd/v2/applicationset/utils"
-
- "k8s.io/apimachinery/pkg/labels"
-
argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-
"github.com/imdario/mergo"
+ "github.com/jeremywohl/flatten"
"github.com/rs/zerolog/log"
+ "k8s.io/apimachinery/pkg/labels"
)
const (
@@ -25,7 +21,7 @@ type TransformResult struct {
Template argoprojiov1alpha1.ApplicationSetTemplate
}
-// Transform a spec generator to list of paramSets and a template
+// Transform a spec generator to list of paramSets and a template.
func Transform(requestedGenerator argoprojiov1alpha1.ApplicationSetGenerator, allGenerators map[string]Generator, baseTemplate argoprojiov1alpha1.ApplicationSetTemplate, appSet *argoprojiov1alpha1.ApplicationSet, genParams map[string]interface{}) ([]TransformResult, error) {
// This is a custom version of the `LabelSelectorAsSelector` that is in k8s.io/apimachinery. This has been copied
// verbatim from that package, with the difference that we do not have any restrictions on label values. This is done
@@ -100,7 +96,7 @@ func GetRelevantGenerators(requestedGenerator *argoprojiov1alpha1.ApplicationSet
var res []Generator
v := reflect.Indirect(reflect.ValueOf(requestedGenerator))
- for i := 0; i < v.NumField(); i++ {
+ for i := range v.NumField() {
field := v.Field(i)
if !field.CanInterface() {
continueFeedback & Suggestions:
😼 Mergecat review of pkg/vcs/github_client/message.go@@ -35,7 +35,6 @@ func (c *Client) PostMessage(ctx context.Context, pr vcs.PullRequest, message st
pr.CheckID,
&github.IssueComment{Body: &message},
)
-
if err != nil {
telemetry.SetError(span, err, "Create Pull Request comment")
return nil, errors.Wrap(err, "could not post message to PR")
@@ -63,7 +62,6 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, msg string)
int64(m.NoteID),
&github.IssueComment{Body: &msg},
)
-
if err != nil {
telemetry.SetError(span, err, "Update Pull Request comment")
log.Error().Err(err).Msgf("could not update message to PR, msg: %s, response: %+v", msg, resp)
@@ -78,7 +76,7 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, msg string)
// Pull all comments for the specified PR, and delete any comments that already exist from the bot
// This is different from updating an existing message, as this will delete comments from previous runs of the bot
-// Whereas updates occur mid-execution
+// Whereas updates occur mid-execution.
func (c *Client) pruneOldComments(ctx context.Context, pr vcs.PullRequest, comments []*github.IssueComment) error {
_, span := tracer.Start(ctx, "pruneOldComments")
defer span.End()
@@ -127,7 +125,6 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, pr vcs.PullRequest, c
}
return nil
-
}
func (c *Client) TidyOutdatedComments(ctx context.Context, pr vcs.PullRequest) error {Feedback & Suggestions:
Overall, the changes are minor and focused on code style. Keep up the good work in maintaining a clean and readable codebase! 👍 😼 Mergecat review of pkg/server/hook_handler.go@@ -7,6 +7,7 @@ import (
"strings"
"github.com/labstack/echo/v4"
+ "github.com/pkg/errors"
"github.com/rs/zerolog/log"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
@@ -32,6 +33,7 @@ func NewVCSHookHandler(ctr container.Container, processors []checks.ProcessorEnt
processors: processors,
}
}
+
func (h *VCSHookHandler) AttachHandlers(grp *echo.Group) {
projectHookPath := fmt.Sprintf("/%s/project", h.ctr.VcsClient.GetName())
grp.POST(projectHookPath, h.groupHandler)
@@ -49,15 +51,14 @@ func (h *VCSHookHandler) groupHandler(c echo.Context) error {
pr, err := h.ctr.VcsClient.ParseHook(ctx, c.Request(), payload)
if err != nil {
- switch err {
- case vcs.ErrInvalidType:
+ if errors.Is(err, vcs.ErrInvalidType) {
log.Debug().Msg("Ignoring event, not a supported request")
return c.String(http.StatusOK, "Skipped")
- default:
- // TODO: do something ELSE with the error
- log.Error().Err(err).Msg("Failed to create a repository locally")
- return echo.ErrBadRequest
}
+
+ // TODO: do something ELSE with the error
+ log.Error().Err(err).Msg("Failed to create a repository locally")
+ return echo.ErrBadRequest
}
// We now have a generic repo with all the info we need to start processing an event. Hand off to the event processor
@@ -76,8 +77,7 @@ func (h *VCSHookHandler) processCheckEvent(ctx context.Context, pullRequest vcs.
ProcessCheckEvent(ctx, pullRequest, h.ctr, h.processors)
}
-type RepoDirectory struct {
-}
+type RepoDirectory struct{}
func ProcessCheckEvent(ctx context.Context, pr vcs.PullRequest, ctr container.Container, processors []checks.ProcessorEntry) {
ctx, span := tracer.Start(ctx, "processCheckEvent",Feedback & Suggestions:
Overall, the changes improve the code's error handling and readability. Addressing the 😼 Mergecat review of cmd/root.go@@ -11,7 +11,7 @@ import (
"github.com/spf13/viper"
)
-// RootCmd represents the base command when called without any subcommands
+// RootCmd represents the base command when called without any subcommands.
var RootCmd = &cobra.Command{
Use: "kubechecks",
Short: "Argo Git Hooks",
@@ -142,15 +142,14 @@ func setupLogOutput() {
// set logrus log level to overwrite the logs exporting from argo-cd package
logrusLevel := logrus.ErrorLevel
if viper.GetBool("persist_log_level") {
- if log.Debug().Enabled() {
+ if log.Debug().Enabled() { //nolint: zerologlint
logrusLevel = logrus.DebugLevel
}
- if log.Trace().Enabled() {
+ if log.Trace().Enabled() { //nolint: zerologlint
logrusLevel = logrus.TraceLevel
}
}
logrus.StandardLogger().Level = logrusLevel
log.Info().Str("log_level", logrus.StandardLogger().Level.String()).Msg("setting logrus log level")
-
}Feedback & Suggestions:
Overall, the changes are minor but contribute to a cleaner and more consistent codebase. Ensure that the use of linter directives is justified and documented. 😼 Mergecat review of pkg/vcs/gitlab_client/client.go@@ -19,8 +19,6 @@ import (
"github.com/zapier/kubechecks/pkg/vcs"
)
-const GitlabTokenHeader = "X-Gitlab-Token"
-
type Client struct {
c *GLClient
cfg config.ServerConfig
@@ -90,10 +88,10 @@ func (c *Client) GetName() string {
return "gitlab"
}
-// VerifyHook returns an err if the webhook isn't valid
+// VerifyHook returns an err if the webhook isn't valid.
func (c *Client) VerifyHook(r *http.Request, secret string) ([]byte, error) {
// If we have a secret, and the secret doesn't match, return an error
- if secret != "" && secret != r.Header.Get(GitlabTokenHeader) {
+ if secret != "" && secret != r.Header.Get("X-Gitlab-Token") {
return nil, fmt.Errorf("invalid secret")
}
@@ -104,7 +102,7 @@ func (c *Client) VerifyHook(r *http.Request, secret string) ([]byte, error) {
var nilPr vcs.PullRequest
-// ParseHook parses and validates a webhook event; return an err if this isn't valid
+// ParseHook parses and validates a webhook event; return an err if this isn't valid.
func (c *Client) ParseHook(ctx context.Context, r *http.Request, request []byte) (vcs.PullRequest, error) {
eventRequest, err := gitlab.ParseHook(gitlab.HookEventType(r), request)
if err != nil {
@@ -195,14 +193,13 @@ func (c *Client) CreateHook(ctx context.Context, repoName, webhookUrl, webhookSe
return errors.Wrap(err, "failed to parse repo name")
}
- _, glStatus, err := c.c.Projects.AddProjectHook(pid, &gitlab.AddProjectHookOptions{
+ _, _, err = c.c.Projects.AddProjectHook(pid, &gitlab.AddProjectHookOptions{
URL: pkg.Pointer(webhookUrl),
MergeRequestsEvents: pkg.Pointer(true),
NoteEvents: pkg.Pointer(true),
Token: pkg.Pointer(webhookSecret),
})
-
- if err != nil && glStatus.StatusCode < http.StatusOK || glStatus.StatusCode >= http.StatusMultipleChoices {
+ if err != nil {
return errors.Wrap(err, "failed to create project webhook")
}
Feedback & Suggestions:
By addressing these points, the code will be more robust and maintainable. 🛠️ 😼 Mergecat review of pkg/affected_apps/config_matcher_test.go@@ -39,7 +39,6 @@ func Test_dirMatchForApp(t *testing.T) {
}
func TestConfigMatcher_triggeredApps(t *testing.T) {
-
type args struct {
modifiedFiles []string
}
@@ -95,8 +94,6 @@ func TestConfigMatcher_triggeredApps(t *testing.T) {
}
for _, tt := range tests {
- tt := tt
-
t.Run(tt.name, func(t *testing.T) {
mockArgoClient := newMockArgoClient()
@@ -116,8 +113,7 @@ func newMockArgoClient() argoClient {
return new(mockArgoClient)
}
-type mockArgoClient struct {
-}
+type mockArgoClient struct{}
func (m mockArgoClient) GetApplications(ctx context.Context) (*v1alpha1.ApplicationList, error) {
return new(v1alpha1.ApplicationList), nil
@@ -130,10 +126,11 @@ func (m mockArgoClient) GetApplicationsByAppset(ctx context.Context, appsetName
var _ argoClient = new(mockArgoClient)
func testLoadConfig(t *testing.T, configDir string) *repo_config.Config {
+ t.Helper()
+
cfg, err := repo_config.LoadRepoConfig(configDir)
- if err != nil {
- t.Errorf("could not load test config from dir (%s): %v", configDir, err)
- }
+ assert.NoErrorf(t, err, "could not load test config from dir (%s)", configDir)
+
return cfg
}
Feedback & Suggestions:
Overall, these changes improve the readability, maintainability, and robustness of the test code. Keep up the good work! 🚀 😼 Mergecat review of pkg/argo_client/applications.go@@ -2,17 +2,15 @@ package argo_client
import (
"context"
- "fmt"
"strings"
- "github.com/argoproj/argo-cd/v2/pkg/apiclient/applicationset"
- "github.com/rs/zerolog/log"
- "go.opentelemetry.io/otel"
-
"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/cluster"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/pkg/errors"
+ "github.com/rs/zerolog/log"
+ "go.opentelemetry.io/otel"
"github.com/zapier/kubechecks/telemetry"
)
@@ -34,7 +32,7 @@ func (argo *ArgoClient) GetApplicationByName(ctx context.Context, name string) (
resp, err := appClient.Get(ctx, &application.ApplicationQuery{Name: &name})
if err != nil {
telemetry.SetError(span, err, "Argo Get Application error")
- return nil, fmt.Errorf("failed to retrieve the application: %v", err)
+ return nil, errors.Wrap(err, "failed to retrieve the application")
}
return resp, nil
@@ -65,7 +63,7 @@ func (argo *ArgoClient) GetKubernetesVersionByApplication(ctx context.Context, a
clusterResponse, err := clusterClient.Get(ctx, clusterRequest)
if err != nil {
telemetry.SetError(span, err, "Argo Get Cluster error")
- return "", fmt.Errorf("failed to retrieve the destination Kubernetes cluster: %v", err)
+ return "", errors.Wrap(err, "failed to retrieve the destination Kubernetes cluster")
}
// Get Kubernetes version
@@ -95,7 +93,7 @@ func (argo *ArgoClient) GetApplicationsByLabels(ctx context.Context, labels stri
resp, err := appClient.List(ctx, &application.ApplicationQuery{Selector: &labels})
if err != nil {
telemetry.SetError(span, err, "Argo List Application error")
- return nil, fmt.Errorf("failed to retrieve applications from labels: %v", err)
+ return nil, errors.Wrap(err, "failed to retrieve applications from labels")
}
return resp, nilFeedback & Suggestions:
Overall, the changes improve the code's readability and error handling consistency. Keep up the good work! 👍 😼 Mergecat review of pkg/vcs/gitlab_client/message.go@@ -47,7 +47,6 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, projectName string, m
// loop through notes and collapse any that are from the current user
for _, note := range notes {
-
// Do not try to hide the note if
// note user is not the gitlabTokenUser
// note is an internal system note such as notes on commit messages
@@ -74,7 +73,6 @@ func (c *Client) hideOutdatedMessages(ctx context.Context, projectName string, m
_, _, err := c.c.Notes.UpdateMergeRequestNote(projectName, mergeRequestID, note.ID, &gitlab.UpdateMergeRequestNoteOptions{
Body: &newBody,
})
-
if err != nil {
telemetry.SetError(span, err, "Hide Existing Merge Request Check Note")
return fmt.Errorf("could not hide note %d for merge request: %w", note.ID, err)
@@ -95,7 +93,6 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri
n, _, err := c.c.Notes.UpdateMergeRequestNote(m.Name, m.CheckID, m.NoteID, &gitlab.UpdateMergeRequestNoteOptions{
Body: pkg.Pointer(message),
})
-
if err != nil {
log.Error().Err(err).Msg("could not update message to MR")
return err
@@ -106,7 +103,7 @@ func (c *Client) UpdateMessage(ctx context.Context, m *msg.Message, message stri
return nil
}
-// Iterate over all comments for the Merge Request, deleting any from the authenticated user
+// Iterate over all comments for the Merge Request, deleting any from the authenticated user.
func (c *Client) pruneOldComments(ctx context.Context, projectName string, mrID int, notes []*gitlab.Note) error {
_, span := tracer.Start(ctx, "pruneOldComments")
defer span.End()
@@ -144,7 +141,6 @@ func (c *Client) TidyOutdatedComments(ctx context.Context, pr vcs.PullRequest) e
Page: nextPage,
},
})
-
if err != nil {
telemetry.SetError(span, err, "Tidy Outdated Comments")
return fmt.Errorf("could not fetch notes for merge request: %w", err)
@@ -160,7 +156,6 @@ func (c *Client) TidyOutdatedComments(ctx context.Context, pr vcs.PullRequest) e
return c.pruneOldComments(ctx, pr.FullName, pr.CheckID, allNotes)
}
return c.hideOutdatedMessages(ctx, pr.FullName, pr.CheckID, allNotes)
-
}
type NotesServices interface {Feedback & Suggestions:
Overall, the changes are minor and focused on code style improvements. Keep up the good work! 👍 😼 Mergecat review of pkg/app_watcher/appset_watcher.go@@ -11,14 +11,15 @@ 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/config"
"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"
)
-// ApplicationSetWatcher is the controller that watches ArgoCD Application resources via the Kubernetes API
+// ApplicationSetWatcher is the controller that watches ArgoCD Application resources via the Kubernetes API.
type ApplicationSetWatcher struct {
applicationClientset appclientset.Interface
appInformer cache.SharedIndexInformer
@@ -81,7 +82,7 @@ func (ctrl *ApplicationSetWatcher) newApplicationSetInformerAndLister(refreshTim
}
// onAdd is the function executed when the informer notifies the
-// presence of a new Application in the namespace
+// presence of a new Application in the namespace.
func (ctrl *ApplicationSetWatcher) onApplicationSetAdded(obj interface{}) {
appSet, ok := canProcessAppSet(obj)
if !ok {
@@ -112,7 +113,6 @@ func (ctrl *ApplicationSetWatcher) onApplicationSetUpdated(old, new interface{})
log.Info().Str("key", key).Msg("appsetwatcher: onApplicationSetUpdated")
ctrl.vcsToArgoMap.UpdateAppSet(old.(*appv1alpha1.ApplicationSet), new.(*appv1alpha1.ApplicationSet))
}
-
}
func (ctrl *ApplicationSetWatcher) onApplicationSetDeleted(obj interface{}) {
@@ -128,6 +128,7 @@ func (ctrl *ApplicationSetWatcher) onApplicationSetDeleted(obj interface{}) {
log.Info().Str("key", key).Msg("appsetwatcher: onApplicationSetDeleted")
ctrl.vcsToArgoMap.DeleteAppSet(app)
}
+
func canProcessAppSet(obj interface{}) (*appv1alpha1.ApplicationSet, bool) {
app, ok := obj.(*appv1alpha1.ApplicationSet)
if !ok {Feedback & Suggestions:
Overall, these changes are minor and stylistic, focusing on improving code readability and organization. Keep up the good work! 👍 😼 Mergecat review of pkg/checks/kubeconform/validate.go@@ -20,7 +20,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, tempRepoPath string) []string {
cfg := ctr.Config
locations := []string{
@@ -87,12 +87,12 @@ func argoCdAppValidate(ctx context.Context, ctr container.Container, appName, ta
KubernetesVersion: targetKubernetesVersion,
Strict: true,
IgnoreMissingSchemas: false,
- Debug: log.Debug().Enabled(),
+ Debug: log.Debug().Enabled(), //nolint: zerologlint
}
var (
outputString []string
- schemaLocations = getSchemaLocations(ctx, ctr, tempRepoPath)
+ schemaLocations = getSchemaLocations(ctr, tempRepoPath)
)
log.Debug().Msgf("cache location: %s", vOpts.Cache)
@@ -101,7 +101,7 @@ func argoCdAppValidate(ctx context.Context, ctr container.Container, appName, ta
v, err := validator.New(schemaLocations, vOpts)
if err != nil {
- return msg.Result{}, fmt.Errorf("could not create kubeconform validator: %v", err)
+ return msg.Result{}, errors.Wrap(err, "could not create kubeconform validator")
}
result := v.Validate("-", io.NopCloser(strings.NewReader(strings.Join(appManifests, "\n"))))
var invalid, failedValidation bool
@@ -127,11 +127,12 @@ func argoCdAppValidate(ctx context.Context, ctr container.Container, appName, ta
}
var cr msg.Result
- if invalid {
+ switch {
+ case invalid:
cr.State = pkg.StateWarning
- } else if failedValidation {
+ case failedValidation:
cr.State = pkg.StateFailure
- } else {
+ default:
cr.State = pkg.StateSuccess
}
Feedback & Suggestions:
Overall, the changes improve the code's readability and maintainability, but be cautious about the implications of removing the context parameter. 🛠️ 😼 Mergecat review of pkg/appdir/appset_directory_test.go@@ -7,13 +7,13 @@ import (
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/stretchr/testify/assert"
- "github.com/zapier/kubechecks/pkg/git"
v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+ "github.com/zapier/kubechecks/pkg/git"
)
func TestAppSetDirectory_ProcessApp(t *testing.T) {
-
type args struct {
app v1alpha1.ApplicationSet
}
@@ -69,7 +69,6 @@ func TestAppSetDirectory_ProcessApp(t *testing.T) {
}
func TestAppSetDirectory_FindAppsBasedOnChangeList(t *testing.T) {
-
tests := []struct {
name string
changeList []string
@@ -196,12 +195,12 @@ spec:
for fileName, content := range tt.mockFiles {
absPath := filepath.Join(tempDir, fileName)
cleanUpDirs = append(cleanUpDirs, absPath)
- err := os.MkdirAll(filepath.Dir(absPath), 0755)
+ err := os.MkdirAll(filepath.Dir(absPath), 0o755)
if err != nil {
fatalErr = err
break
}
- err = os.WriteFile(absPath, []byte(content), 0644)
+ err = os.WriteFile(absPath, []byte(content), 0o600)
if err != nil {
fatalErr = err
break
@@ -218,8 +217,10 @@ spec:
}
}
-// cleanUpTmpFiles removes the temporary directories created for the test
+// cleanUpTmpFiles removes the temporary directories created for the test.
func cleanUpTmpFiles(t *testing.T, cleanUpDirs []string) {
+ t.Helper()
+
for _, dir := range cleanUpDirs {
if err := os.RemoveAll(filepath.Dir(dir)); err != nil {
t.Fatalf("failed to remove tmp folder %s", err)Feedback & Suggestions:
Overall, these changes improve the code's readability and security. Ensure that the file permission changes are intentional and align with your security requirements. 😼 Mergecat review of pkg/generator/list_test.go@@ -3,11 +3,11 @@ package generator
import (
"testing"
+ argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
- argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)
func TestGenerateListParams(t *testing.T) {
@@ -25,8 +25,7 @@ func TestGenerateListParams(t *testing.T) {
}
for _, testCase := range testCases {
-
- var listGenerator = NewListGenerator()
+ listGenerator := NewListGenerator()
applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
@@ -38,11 +37,11 @@ func TestGenerateListParams(t *testing.T) {
got, err := listGenerator.GenerateParams(&argoprojiov1alpha1.ApplicationSetGenerator{
List: &argoprojiov1alpha1.ListGenerator{
Elements: testCase.elements,
- }}, &applicationSetInfo)
+ },
+ }, &applicationSetInfo)
- assert.NoError(t, err)
+ require.NoError(t, err)
assert.ElementsMatch(t, testCase.expected, got)
-
}
}
@@ -61,8 +60,7 @@ func TestGenerateListParamsGoTemplate(t *testing.T) {
}
for _, testCase := range testCases {
-
- var listGenerator = NewListGenerator()
+ listGenerator := NewListGenerator()
applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
@@ -76,9 +74,10 @@ func TestGenerateListParamsGoTemplate(t *testing.T) {
got, err := listGenerator.GenerateParams(&argoprojiov1alpha1.ApplicationSetGenerator{
List: &argoprojiov1alpha1.ListGenerator{
Elements: testCase.elements,
- }}, &applicationSetInfo)
+ },
+ }, &applicationSetInfo)
- assert.NoError(t, err)
+ require.NoError(t, err)
assert.ElementsMatch(t, testCase.expected, got)
}
}Feedback & Suggestions:
Overall, these changes improve the code's readability and reliability. Great job! 🎉 😼 Mergecat review of pkg/checks/rego/check.go@@ -48,9 +48,11 @@ func NewChecker(cfg config.ServerConfig) (*Checker, error) {
return &c, nil
}
-var ErrResourceMustHaveKind = errors.New("resource does not have kind")
-var ErrResourceMustHaveMetadata = errors.New("resource does not have metadata")
-var ErrResourceMustHaveName = errors.New("resource does not have name")
+var (
+ ErrResourceMustHaveKind = errors.New("resource does not have kind")
+ ErrResourceMustHaveMetadata = errors.New("resource does not have metadata")
+ ErrResourceMustHaveName = errors.New("resource does not have name")
+)
func getFilenameFromRawManifest(manifest string) (string, error) {
resource := make(map[string]interface{})
@@ -105,7 +107,7 @@ func dumpFiles(manifests []string) (string, error) {
Int("size", len(manifestBytes)).
Msg("dumping manifest")
- if err = os.WriteFile(fullPath, manifestBytes, 0o666); err != nil {
+ if err = os.WriteFile(fullPath, manifestBytes, 0o600); err != nil {
return result, errors.Wrapf(err, "failed to write %s", filename)
}
}
@@ -177,11 +179,12 @@ func (c *Checker) Check(ctx context.Context, request checks.Request) (msg.Result
}
var cr msg.Result
- if failures {
+ switch {
+ case failures:
cr.State = pkg.StateFailure
- } else if warnings {
+ case warnings:
cr.State = pkg.StateWarning
- } else {
+ default:
cr.State = pkg.StateSuccess
}
Feedback & Suggestions:
Overall, these changes enhance the code's security, readability, and maintainability. Great job! 🎉 😼 Mergecat review of pkg/generator/merge_test.go@@ -5,10 +5,10 @@ import (
"fmt"
"testing"
+ argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-
- argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)
func getNestedListGenerator(json string) *argoprojiov1alpha1.ApplicationSetNestedGenerator {
@@ -49,6 +49,7 @@ func listOfMapsToSet(maps []map[string]interface{}) (map[string]bool, error) {
}
func TestMergeGenerate(t *testing.T) {
+ t.Parallel()
testCases := []struct {
name string
@@ -156,7 +157,7 @@ func TestMergeGenerate(t *testing.T) {
appSet := &argoprojiov1alpha1.ApplicationSet{}
- var mergeGenerator = NewMergeGenerator(
+ mergeGenerator := NewMergeGenerator(
map[string]Generator{
"List": &ListGenerator{},
"Matrix": &MatrixGenerator{
@@ -184,19 +185,18 @@ func TestMergeGenerate(t *testing.T) {
assert.EqualError(t, err, testCaseCopy.expectedErr.Error())
} else {
expectedSet, err := listOfMapsToSet(testCaseCopy.expected)
- assert.NoError(t, err)
+ require.NoError(t, err)
actualSet, err := listOfMapsToSet(got)
- assert.NoError(t, err)
-
- assert.NoError(t, err)
+ require.NoError(t, err)
assert.Equal(t, expectedSet, actualSet)
}
})
}
}
func toAPIExtensionsJSON(t *testing.T, g interface{}) *apiextensionsv1.JSON {
+ t.Helper()
resVal, err := json.Marshal(g)
if err != nil {
@@ -210,6 +210,8 @@ func toAPIExtensionsJSON(t *testing.T, g interface{}) *apiextensionsv1.JSON {
}
func TestParamSetsAreUniqueByMergeKeys(t *testing.T) {
+ t.Parallel()
+
testCases := []struct {
name string
mergeKeys []string
@@ -341,11 +343,9 @@ func TestParamSetsAreUniqueByMergeKeys(t *testing.T) {
if testCaseCopy.expectedErr != nil {
assert.EqualError(t, err, testCaseCopy.expectedErr.Error())
} else {
- assert.NoError(t, err)
+ require.NoError(t, err)
assert.Equal(t, testCaseCopy.expected, got)
}
-
})
-
}
}Feedback & Suggestions:
Overall, these changes enhance the test code's performance, readability, and reliability. Great job! 🎉 😼 Mergecat review of pkg/generator/merge.go@@ -5,11 +5,10 @@ import (
"fmt"
"time"
- "github.com/imdario/mergo"
-
"github.com/argoproj/argo-cd/v2/applicationset/utils"
argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-
+ "github.com/imdario/mergo"
+ "github.com/pkg/errors"
"github.com/rs/zerolog/log"
)
@@ -77,7 +76,6 @@ func (m *MergeGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Appl
for mergeKeyValue, baseParamSet := range baseParamSetsByMergeKey {
if overrideParamSet, exists := paramSetsByMergeKey[mergeKeyValue]; exists {
-
if appSet.Spec.GoTemplate {
if err := mergo.Merge(&baseParamSet, overrideParamSet, mergo.WithOverride); err != nil {
return nil, fmt.Errorf("error merging base param set with override param set: %w", err)
@@ -95,7 +93,7 @@ func (m *MergeGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Appl
}
mergedParamSets := make([]map[string]interface{}, len(baseParamSetsByMergeKey))
- var i = 0
+ i := 0
for _, mergedParamSet := range baseParamSetsByMergeKey {
mergedParamSets[i] = mergedParamSet
i += 1
@@ -177,9 +175,8 @@ func (m *MergeGenerator) getParams(appSetBaseGenerator argoprojiov1alpha1.Applic
argoprojiov1alpha1.ApplicationSetTemplate{},
appSet,
map[string]interface{}{})
-
if err != nil {
- return nil, fmt.Errorf("child generator returned an error on parameter generation: %v", err)
+ return nil, errors.Wrap(err, "child generator returned an error on parameter generation")
}
if len(t) == 0 {
@@ -227,7 +224,6 @@ func (m *MergeGenerator) GetRequeueAfter(appSetGenerator *argoprojiov1alpha1.App
} else {
return NoRequeueAfter
}
-
}
func getMergeGenerator(r argoprojiov1alpha1.ApplicationSetNestedGenerator) (*argoprojiov1alpha1.MergeGenerator, error) {Feedback & Suggestions:
Overall, the changes improve the code's readability and error handling. Keep up the good work! 🚀 😼 Mergecat review of pkg/appdir/vcstoargomap_test.go@@ -26,8 +26,8 @@ func TestAddApp(t *testing.T) {
v2a.AddApp(app1)
appDir := v2a.GetAppsInRepo("https://github.com/argoproj/argo-cd.git")
- assert.Equal(t, appDir.Count(), 1)
- assert.Equal(t, len(appDir.appDirs["test-app-1"]), 1)
+ assert.Equal(t, 1, appDir.Count())
+ assert.Len(t, appDir.appDirs["test-app-1"], 1)
// Assertions to verify the behavior here.
app2 := &v1alpha1.Application{
@@ -41,8 +41,8 @@ func TestAddApp(t *testing.T) {
}
v2a.AddApp(app2)
- assert.Equal(t, appDir.Count(), 2)
- assert.Equal(t, len(appDir.appDirs["test-app-2"]), 1)
+ assert.Equal(t, 2, appDir.Count())
+ assert.Len(t, appDir.appDirs["test-app-2"], 1)
}
func TestDeleteApp(t *testing.T) {
@@ -73,13 +73,13 @@ func TestDeleteApp(t *testing.T) {
v2a.AddApp(app2)
appDir := v2a.GetAppsInRepo("https://github.com/argoproj/argo-cd.git")
- assert.Equal(t, appDir.Count(), 2)
- assert.Equal(t, len(appDir.appDirs["test-app-1"]), 1)
- assert.Equal(t, len(appDir.appDirs["test-app-2"]), 1)
+ assert.Equal(t, 2, appDir.Count())
+ assert.Len(t, appDir.appDirs["test-app-1"], 1)
+ assert.Len(t, appDir.appDirs["test-app-2"], 1)
v2a.DeleteApp(app2)
- assert.Equal(t, appDir.Count(), 1)
- assert.Equal(t, len(appDir.appDirs["test-app-2"]), 0)
+ assert.Equal(t, 1, appDir.Count())
+ assert.Empty(t, appDir.appDirs["test-app-2"])
}
func TestVcsToArgoMap_AddAppSet(t *testing.T) {
@@ -137,7 +137,7 @@ func TestVcsToArgoMap_AddAppSet(t *testing.T) {
appSetDirByRepo: tt.fields.appSetDirByRepo,
}
v2a.AddAppSet(tt.args.app)
- assert.Equal(t, tt.expectedCount, len(v2a.appSetDirByRepo))
+ assert.Len(t, v2a.appSetDirByRepo, tt.expectedCount)
})
}
}
@@ -178,11 +178,11 @@ func TestVcsToArgoMap_DeleteAppSet(t *testing.T) {
v2a.AddAppSet(app2)
appDir := v2a.GetAppSetsInRepo("https://github.com/argoproj/argo-cd.git")
- assert.Equal(t, appDir.Count(), 2)
- assert.Equal(t, len(appDir.appSetDirs["test-app-1"]), 1)
- assert.Equal(t, len(appDir.appSetDirs["test-app-2"]), 1)
+ assert.Equal(t, 2, appDir.Count())
+ assert.Len(t, appDir.appSetDirs["test-app-1"], 1)
+ assert.Len(t, appDir.appSetDirs["test-app-2"], 1)
v2a.DeleteAppSet(app2)
- assert.Equal(t, appDir.Count(), 1)
- assert.Equal(t, len(appDir.appSetDirs["test-app-2"]), 0)
+ assert.Equal(t, 1, appDir.Count())
+ assert.Empty(t, appDir.appSetDirs["test-app-2"])Feedback & Suggestions:
Overall, these changes enhance the clarity and maintainability of the test code. Great job! 🎉 😼 Mergecat review of pkg/app_watcher/appset_watcher_test.go@@ -9,12 +9,15 @@ import (
appclientsetfake "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
"github.com/zapier/kubechecks/pkg/appdir"
"github.com/zapier/kubechecks/pkg/config"
- metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
func initTestObjectsForAppSets(t *testing.T) *ApplicationSetWatcher {
+ t.Helper()
+
cfg, err := config.New()
// Handle the error appropriately, e.g., log it or fail the test
require.NoError(t, err, "failed to create config")
@@ -70,7 +73,7 @@ func TestApplicationSetWatcher_OnApplicationAdded(t *testing.T) {
time.Sleep(time.Second * 1)
- assert.Equal(t, 2, len(appWatcher.vcsToArgoMap.GetAppSetMap()))
+ assert.Len(t, appWatcher.vcsToArgoMap.GetAppSetMap(), 2)
_, err := appWatcher.applicationClientset.ArgoprojV1alpha1().ApplicationSets("default").Create(ctx, &v1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{Name: "test-app-3", Namespace: "default"},
@@ -90,7 +93,7 @@ func TestApplicationSetWatcher_OnApplicationAdded(t *testing.T) {
}
time.Sleep(time.Second * 1)
- assert.Equal(t, 3, len(appWatcher.vcsToArgoMap.GetAppSetMap()))
+ assert.Len(t, appWatcher.vcsToArgoMap.GetAppSetMap(), 3)
}
func TestApplicationSetWatcher_OnApplicationUpdated(t *testing.T) {
@@ -103,7 +106,7 @@ func TestApplicationSetWatcher_OnApplicationUpdated(t *testing.T) {
time.Sleep(time.Second * 1)
- assert.Equal(t, len(ctrl.vcsToArgoMap.GetAppSetMap()), 2)
+ assert.Len(t, ctrl.vcsToArgoMap.GetAppSetMap(), 2)
oldAppDirectory := ctrl.vcsToArgoMap.GetAppSetsInRepo("https://gitlab.com/test/repo.git")
newAppDirectory := ctrl.vcsToArgoMap.GetAppSetsInRepo("https://gitlab.com/test/repo-3.git")
@@ -129,8 +132,8 @@ func TestApplicationSetWatcher_OnApplicationUpdated(t *testing.T) {
time.Sleep(time.Second * 1)
oldAppDirectory = ctrl.vcsToArgoMap.GetAppSetsInRepo("https://gitlab.com/test/repo.git")
newAppDirectory = ctrl.vcsToArgoMap.GetAppSetsInRepo("https://gitlab.com/test/repo-3.git")
- assert.Equal(t, oldAppDirectory.Count(), 0)
- assert.Equal(t, newAppDirectory.Count(), 1)
+ assert.Equal(t, 0, oldAppDirectory.Count())
+ assert.Equal(t, 1, newAppDirectory.Count())
}
func TestApplicationSetWatcher_OnApplicationDEleted(t *testing.T) {
@@ -143,7 +146,7 @@ func TestApplicationSetWatcher_OnApplicationDEleted(t *testing.T) {
time.Sleep(time.Second * 1)
- assert.Equal(t, 2, len(ctrl.vcsToArgoMap.GetAppSetMap()))
+ assert.Len(t, ctrl.vcsToArgoMap.GetAppSetMap(), 2)
appDirectory := ctrl.vcsToArgoMap.GetAppSetsInRepo("https://gitlab.com/test/repo.git")
assert.Equal(t, 1, appDirectory.Count())
@@ -257,7 +260,6 @@ func Test_CanProcessAppSet(t *testing.T) {
}
for _, tc := range tests {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
app, canProcess := canProcessAppSet(tc.resource)
Feedback & Suggestions:
Overall, these changes improve the readability and maintainability of the test code. Keep up the good work! 🚀 😼 Mergecat review of pkg/checks/kubeconform/validate_test.go package kubeconform
import (
- "context"
- "fmt"
"os"
"strings"
"testing"
fixtures "github.com/go-git/go-git-fixtures/v4"
"github.com/spf13/viper"
-
"github.com/stretchr/testify/assert"
"github.com/zapier/kubechecks/pkg/container"
)
func TestDefaultGetSchemaLocations(t *testing.T) {
- ctx := context.TODO()
ctr := container.Container{}
- schemaLocations := getSchemaLocations(ctx, ctr, "/some/other/path")
+ schemaLocations := getSchemaLocations(ctr, "/some/other/path")
// 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") == "" {
@@ -35,11 +30,10 @@ func TestGetRemoteSchemaLocations(t *testing.T) {
basic := fixtures.Basic()
fixture := basic.One()
- fmt.Println(fixture.URL)
// 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, "/some/other/path")
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])
+ assert.Truef(t, hasTmpDirPrefix, "invalid schemas location. Schema location should have prefix /tmp/schemas but has %s", schemaLocations[0])
}Feedback & Suggestions:
Overall, the changes simplify the code and improve readability. Just ensure that the removal of context and logging aligns with the intended functionality. 🚀 😼 Mergecat review of pkg/repo_config/config.go@@ -1,9 +1,8 @@
package repo_config
import (
- "fmt"
-
"github.com/creasty/defaults"
+ "github.com/pkg/errors"
)
type Config struct {
@@ -12,19 +11,19 @@ type Config struct {
}
type ArgoCdApplicationConfig struct {
- Name string `yaml:"name" validate:"empty=false"`
- Cluster string `yaml:"cluster" validate:"empty=false"`
- Path string `yaml:"path" validate:"empty=false"`
+ Name string `validate:"empty=false" yaml:"name"`
+ Cluster string `validate:"empty=false" yaml:"cluster"`
+ Path string `validate:"empty=false" yaml:"path"`
AdditionalPaths []string `yaml:"additionalPaths"`
- EnableConfTest bool `yaml:"enableConfTest" default:"true"`
- EnableKubeConform bool `yaml:"enableKubeConform" default:"true"`
- EnableKubePug bool `yaml:"enableKubePug" default:"true"`
+ EnableConfTest bool `default:"true" yaml:"enableConfTest"`
+ EnableKubeConform bool `default:"true" yaml:"enableKubeConform"`
+ EnableKubePug bool `default:"true" yaml:"enableKubePug"`
}
func (s *ArgoCdApplicationConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
err := defaults.Set(s)
if err != nil {
- return fmt.Errorf("failed to set defaults for project config: %v", err)
+ return errors.Wrap(err, "failed to set defaults for project config")
}
type plain ArgoCdApplicationConfig
@@ -36,17 +35,17 @@ func (s *ArgoCdApplicationConfig) UnmarshalYAML(unmarshal func(interface{}) erro
}
type ArgocdApplicationSetConfig struct {
- Name string `yaml:"name" validate:"empty=false"`
- Paths []string `yaml:"paths" validate:"empty=false"`
- EnableConfTest bool `yaml:"enableConfTest" default:"true"`
- EnableKubeConform bool `yaml:"enableKubeConform" default:"true"`
- EnableKubePug bool `yaml:"enableKubePug" default:"true"`
+ Name string `validate:"empty=false" yaml:"name"`
+ Paths []string `validate:"empty=false" yaml:"paths"`
+ EnableConfTest bool `default:"true" yaml:"enableConfTest"`
+ EnableKubeConform bool `default:"true" yaml:"enableKubeConform"`
+ EnableKubePug bool `default:"true" yaml:"enableKubePug"`
}
func (s *ArgocdApplicationSetConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
err := defaults.Set(s)
if err != nil {
- return fmt.Errorf("failed to set defaults for project config: %v", err)
+ return errors.Wrap(err, "failed to set defaults for project config")
}
type plain ArgocdApplicationSetConfigFeedback & Suggestions:
Overall, these changes enhance the code's readability and error handling. Keep up the good work! 👍 😼 Mergecat review of pkg/vcs/gitlab_client/backoff.go@@ -1,17 +1,16 @@
package gitlab_client
import (
- "fmt"
+ "net/http"
"time"
"github.com/cenkalti/backoff/v4"
- "github.com/rs/zerolog/log"
+ "github.com/pkg/errors"
"github.com/xanzy/go-gitlab"
)
-// getBackOff returns a backoff pointer to use to retry requests
+// getBackOff returns a backoff pointer to use to retry requests.
func getBackOff() *backoff.ExponentialBackOff {
-
// Lets setup backoff logic to retry this request for 1 minute
bOff := backoff.NewExponentialBackOff()
bOff.InitialInterval = 60 * time.Second
@@ -22,23 +21,17 @@ func getBackOff() *backoff.ExponentialBackOff {
return bOff
}
+var ErrRateLimited = errors.New("rate limited")
+
func checkReturnForBackoff(resp *gitlab.Response, err error) error {
// if the error is nil lets check it out
- if resp != nil {
- if resp.StatusCode == 429 {
- log.Warn().Msg("being rate limited doing backoff")
- return fmt.Errorf("%s", "Rate Limited")
- }
+ if resp != nil && resp.StatusCode == http.StatusTooManyRequests {
+ return ErrRateLimited
}
+
if err != nil {
- // Lets check the error and see if we need to trigger backoff
- switch err.(type) {
- default:
- // If it is not one of the above errors lets skip the backoff logic
- return &backoff.PermanentError{Err: err}
- }
+ return &backoff.PermanentError{Err: err}
}
- // Return nil as the error passed in must have been nil as it passed the switch statement
return nil
}Feedback & Suggestions:
😼 Mergecat review of pkg/generator/matrix.go@@ -4,11 +4,10 @@ import (
"fmt"
"time"
- "github.com/imdario/mergo"
-
"github.com/argoproj/argo-cd/v2/applicationset/utils"
argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-
+ "github.com/imdario/mergo"
+ "github.com/pkg/errors"
"github.com/rs/zerolog/log"
)
@@ -33,7 +32,6 @@ func NewMatrixGenerator(supportedGenerators map[string]Generator) Generator {
}
func (m *MatrixGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, appSet *argoprojiov1alpha1.ApplicationSet) ([]map[string]interface{}, error) {
-
if appSetGenerator.Matrix == nil {
return nil, EmptyAppSetGeneratorError
}
@@ -58,7 +56,6 @@ func (m *MatrixGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.App
return nil, fmt.Errorf("failed to get params for second generator in the matrix generator: %w", err)
}
for _, b := range g1 {
-
if appSet.Spec.GoTemplate {
tmp := map[string]interface{}{}
if err := mergo.Merge(&tmp, b, mergo.WithOverride); err != nil {
@@ -120,13 +117,12 @@ func (m *MatrixGenerator) getParams(appSetBaseGenerator argoprojiov1alpha1.Appli
argoprojiov1alpha1.ApplicationSetTemplate{},
appSet,
params)
-
if err != nil {
- return nil, fmt.Errorf("child generator returned an error on parameter generation: %v", err)
+ return nil, errors.Wrap(err, "child generator returned an error on parameter generation")
}
if len(t) == 0 {
- return nil, fmt.Errorf("child generator generated no parameters")
+ return nil, ErrNoParameters
}
if len(t) > 1 {
@@ -136,6 +132,8 @@ func (m *MatrixGenerator) getParams(appSetBaseGenerator argoprojiov1alpha1.Appli
return t[0].Params, nil
}
+var ErrNoParameters = errors.New("child generator generated no parameters")
+
const maxDuration time.Duration = 1<<63 - 1
func (m *MatrixGenerator) GetRequeueAfter(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) time.Duration {
@@ -172,7 +170,6 @@ func (m *MatrixGenerator) GetRequeueAfter(appSetGenerator *argoprojiov1alpha1.Ap
} else {
return NoRequeueAfter
}
-
}
func getMatrixGenerator(r argoprojiov1alpha1.ApplicationSetNestedGenerator) (*argoprojiov1alpha1.MatrixGenerator, error) {Feedback & Suggestions:
Overall, these changes improve the code's readability and error handling. Keep up the good work! 🚀 😼 Mergecat review of pkg/app_watcher/app_watcher_test.go@@ -9,13 +9,15 @@ import (
appclientsetfake "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
- "github.com/zapier/kubechecks/pkg/config"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"github.com/zapier/kubechecks/pkg/appdir"
+ "github.com/zapier/kubechecks/pkg/config"
)
func initTestObjects(t *testing.T) *ApplicationWatcher {
+ t.Helper()
+
cfg, err := config.New()
// Handle the error appropriately, e.g., log it or fail the test
require.NoError(t, err, "failed to create config")
@@ -57,7 +59,7 @@ func TestApplicationAdded(t *testing.T) {
time.Sleep(time.Second * 1)
- assert.Equal(t, len(appWatcher.vcsToArgoMap.GetMap()), 2)
+ assert.Len(t, appWatcher.vcsToArgoMap.GetMap(), 2)
_, err := appWatcher.applicationClientset.ArgoprojV1alpha1().Applications("default").Create(ctx, &v1alpha1.Application{
ObjectMeta: metav1.ObjectMeta{Name: "test-app-3", Namespace: "default"},
@@ -70,7 +72,7 @@ func TestApplicationAdded(t *testing.T) {
}
time.Sleep(time.Second * 1)
- assert.Equal(t, len(appWatcher.vcsToArgoMap.GetMap()), 3)
+ assert.Len(t, appWatcher.vcsToArgoMap.GetMap(), 3)
}
func TestApplicationUpdated(t *testing.T) {
@@ -83,12 +85,12 @@ func TestApplicationUpdated(t *testing.T) {
time.Sleep(time.Second * 1)
- assert.Equal(t, len(ctrl.vcsToArgoMap.GetMap()), 2)
+ assert.Len(t, ctrl.vcsToArgoMap.GetMap(), 2)
oldAppDirectory := ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git")
newAppDirectory := ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo-3.git")
- assert.Equal(t, oldAppDirectory.Count(), 1)
- assert.Equal(t, newAppDirectory.Count(), 0)
+ assert.Equal(t, 1, oldAppDirectory.Count())
+ assert.Equal(t, 0, newAppDirectory.Count())
//
_, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications("default").Update(ctx, &v1alpha1.Application{
ObjectMeta: metav1.ObjectMeta{Name: "test-app-1", Namespace: "default"},
@@ -102,8 +104,8 @@ func TestApplicationUpdated(t *testing.T) {
time.Sleep(time.Second * 1)
oldAppDirectory = ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git")
newAppDirectory = ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo-3.git")
- assert.Equal(t, oldAppDirectory.Count(), 0)
- assert.Equal(t, newAppDirectory.Count(), 1)
+ assert.Equal(t, 0, oldAppDirectory.Count())
+ assert.Equal(t, 1, newAppDirectory.Count())
}
func TestApplicationDeleted(t *testing.T) {
@@ -116,10 +118,10 @@ func TestApplicationDeleted(t *testing.T) {
time.Sleep(time.Second * 1)
- assert.Equal(t, len(ctrl.vcsToArgoMap.GetMap()), 2)
+ assert.Len(t, ctrl.vcsToArgoMap.GetMap(), 2)
appDirectory := ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git")
- assert.Equal(t, appDirectory.Count(), 1)
+ assert.Equal(t, 1, appDirectory.Count())
//
err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications("default").Delete(ctx, "test-app-1", metav1.DeleteOptions{})
if err != nil {
@@ -128,7 +130,7 @@ func TestApplicationDeleted(t *testing.T) {
time.Sleep(time.Second * 1)
appDirectory = ctrl.vcsToArgoMap.GetAppsInRepo("https://gitlab.com/test/repo.git")
- assert.Equal(t, appDirectory.Count(), 0)
+ assert.Equal(t, 0, appDirectory.Count())
}
// TestIsGitRepo will test various URLs against the isGitRepo function.
@@ -236,7 +238,6 @@ func TestCanProcessApp(t *testing.T) {
}
for _, tc := range tests {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
app, canProcess := canProcessApp(tc.resource)
Feedback & Suggestions:
Overall, these changes improve the readability and maintainability of the test code. Great job! 🎉 😼 Mergecat review of pkg/generator/list.go@@ -5,15 +5,14 @@ import (
"fmt"
"time"
- "sigs.k8s.io/yaml"
-
argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+ "github.com/pkg/errors"
+ "sigs.k8s.io/yaml"
)
var _ Generator = (*ListGenerator)(nil)
-type ListGenerator struct {
-}
+type ListGenerator struct{}
func NewListGenerator() Generator {
g := &ListGenerator{}
@@ -28,6 +27,8 @@ func (g *ListGenerator) GetTemplate(appSetGenerator *argoprojiov1alpha1.Applicat
return &appSetGenerator.List.Template
}
+var ErrInvalidValuesType = errors.New("invalid type")
+
func (g *ListGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, appSet *argoprojiov1alpha1.ApplicationSet) ([]map[string]interface{}, error) {
if appSetGenerator == nil {
return nil, EmptyAppSetGeneratorError
@@ -44,7 +45,7 @@ func (g *ListGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Appli
var element map[string]interface{}
err := json.Unmarshal(tmpItem.Raw, &element)
if err != nil {
- return nil, fmt.Errorf("error unmarshling list element %v", err)
+ return nil, errors.Wrap(err, "error unmarshling list element")
}
if appSet.Spec.GoTemplate {
@@ -54,19 +55,19 @@ func (g *ListGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Appli
if key == "values" {
values, ok := (value).(map[string]interface{})
if !ok {
- return nil, fmt.Errorf("error parsing values map")
+ return nil, ErrInvalidValuesType
}
for k, v := range values {
value, ok := v.(string)
if !ok {
- return nil, fmt.Errorf("error parsing value as string %v", err)
+ return nil, errors.Wrap(err, "error parsing value as string")
}
params[fmt.Sprintf("values.%s", k)] = value
}
} else {
v, ok := value.(string)
if !ok {
- return nil, fmt.Errorf("error parsing value as string %v", err)
+ return nil, errors.Wrap(err, "error parsing value as string")
}
params[key] = v
}
@@ -77,11 +78,10 @@ func (g *ListGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Appli
// Append elements from ElementsYaml to the response
if len(appSetGenerator.List.ElementsYaml) > 0 {
-
var yamlElements []map[string]interface{}
err := yaml.Unmarshal([]byte(appSetGenerator.List.ElementsYaml), &yamlElements)
if err != nil {
- return nil, fmt.Errorf("error unmarshling decoded ElementsYaml %v", err)
+ return nil, errors.Wrap(err, "error unmarshling decoded ElementsYaml")
}
res = append(res, yamlElements...)
}Feedback & Suggestions:
Overall, the changes made in the diff improve error handling and code organization. Keep up the good work! 👍 😼 Mergecat review of pkg/events/check.go@@ -13,17 +13,17 @@ import (
"github.com/pkg/errors"
"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
- "github.com/zapier/kubechecks/pkg/generator"
- "github.com/zapier/kubechecks/pkg/repo_config"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
"github.com/zapier/kubechecks/pkg"
"github.com/zapier/kubechecks/pkg/affected_apps"
"github.com/zapier/kubechecks/pkg/checks"
"github.com/zapier/kubechecks/pkg/container"
+ "github.com/zapier/kubechecks/pkg/generator"
"github.com/zapier/kubechecks/pkg/git"
"github.com/zapier/kubechecks/pkg/msg"
+ "github.com/zapier/kubechecks/pkg/repo_config"
"github.com/zapier/kubechecks/pkg/vcs"
"github.com/zapier/kubechecks/telemetry"
)
@@ -77,7 +77,6 @@ func GenerateMatcher(ce *CheckEvent, repo *git.Repo) error {
}
func NewCheckEvent(pullRequest vcs.PullRequest, ctr container.Container, repoManager repoManager, processors []checks.ProcessorEntry) *CheckEvent {
-
ce := &CheckEvent{
addedAppsSet: make(map[string]v1alpha1.Application),
appChannel: make(chan *v1alpha1.Application, ctr.Config.MaxQueueSize),
@@ -258,7 +257,7 @@ func (ce *CheckEvent) Process(ctx context.Context) error {
ce.logger.Error().Err(err).Msg("Failed to tidy outdated comments")
}
- if len(ce.affectedItems.Applications) <= 0 && len(ce.affectedItems.ApplicationSets) <= 0 {
+ if len(ce.affectedItems.Applications) == 0 && len(ce.affectedItems.ApplicationSets) == 0 {
ce.logger.Info().Msg("No affected apps or appsets, skipping")
if _, err := ce.ctr.VcsClient.PostMessage(ctx, ce.pullRequest, "No changes"); err != nil {
return errors.Wrap(err, "failed to post changes")
@@ -273,7 +272,6 @@ 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,
@@ -359,7 +357,7 @@ func (ce *CheckEvent) queueApp(app v1alpha1.Application) {
}
// CommitStatus sets the commit status on the MR
-// To set the PR/MR status
+// To set the PR/MR status.
func (ce *CheckEvent) CommitStatus(ctx context.Context, status pkg.CommitState) {
_, span := tracer.Start(ctx, "CommitStatus")
defer span.End()
@@ -380,7 +378,7 @@ Check kubechecks application logs for more information.
`
)
-// createNote creates a generic Note struct that we can write into across all worker threads
+// createNote creates a generic Note struct that we can write into across all worker threads.
func (ce *CheckEvent) createNote(ctx context.Context) (*msg.Message, error) {
ctx, span := otel.Tracer("check").Start(ctx, "createNote")
defer span.End()Feedback & Suggestions:
Overall, these changes are minor improvements focused on code style and readability. Ensure that any changes align with the project's coding standards. 😼 Mergecat review of pkg/events/check_test.go@@ -11,6 +11,8 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
affectedappsmocks "github.com/zapier/kubechecks/mocks/affected_apps/mocks"
generatorsmocks "github.com/zapier/kubechecks/mocks/generator/mocks"
"github.com/zapier/kubechecks/pkg/affected_apps"
@@ -21,7 +23,6 @@ import (
"github.com/zapier/kubechecks/pkg/git"
"github.com/zapier/kubechecks/pkg/msg"
"github.com/zapier/kubechecks/pkg/vcs"
- metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
// TestCleanupGetManifestsError tests the cleanupGetManifestsError function.
@@ -153,7 +154,6 @@ func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) {
matcher affected_apps.Matcher
}
type args struct {
- ctx context.Context
repo *git.Repo
targetBranch string
initMatcherFn MatcherFn
@@ -195,7 +195,6 @@ func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) {
}),
},
args: args{
- ctx: context.Background(),
repo: &git.Repo{Directory: "/tmp"},
targetBranch: "HEAD",
initMatcherFn: MockInitMatcherFn(),
@@ -225,7 +224,6 @@ func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) {
}),
},
args: args{
- ctx: context.Background(),
repo: &git.Repo{Directory: "/tmp"},
targetBranch: "HEAD",
initMatcherFn: MockInitMatcherFn(),
@@ -262,7 +260,6 @@ func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) {
}),
},
args: args{
- ctx: context.Background(),
repo: &git.Repo{Directory: "/tmp"},
targetBranch: "HEAD",
initMatcherFn: MockInitMatcherFn(),
@@ -289,8 +286,7 @@ func TestCheckEvent_GenerateListOfAffectedApps(t *testing.T) {
generator: tt.fields.generator,
matcher: tt.fields.matcher,
}
- tt.wantErr(t, ce.GenerateListOfAffectedApps(tt.args.ctx, tt.args.repo, tt.args.targetBranch, tt.args.initMatcherFn), fmt.Sprintf("GenerateListOfAffectedApps(%v, %v, %v, %v)", tt.args.ctx, tt.args.repo, tt.args.targetBranch, tt.args.initMatcherFn))
-
+ tt.wantErr(t, ce.GenerateListOfAffectedApps(context.Background(), tt.args.repo, tt.args.targetBranch, tt.args.initMatcherFn), fmt.Sprintf("GenerateListOfAffectedApps(%v, %v, %v, %v)", context.Background(), tt.args.repo, tt.args.targetBranch, tt.args.initMatcherFn))
})
}
}
@@ -308,6 +304,7 @@ func MockGenerator(methodName string, returns []interface{}) generator.AppsGener
return mockClient
}
+
func MockInitMatcherFn() MatcherFn {
return func(ce *CheckEvent, repo *git.Repo) error {
return nilFeedback & Suggestions:
Overall, the changes seem minor and mostly related to code organization. Ensure that these changes align with the overall codebase style and future maintainability. 🛠️ 😼 Mergecat review of telemetry/telemetry.go@@ -23,17 +23,16 @@ import (
const DefaultMetricInterval = 2
type BaseTelemetry struct {
- c context.Context
traceProvider *sdktrace.TracerProvider
metric *metric.MeterProvider
}
-func (bt *BaseTelemetry) Shutdown() {
+func (bt *BaseTelemetry) Shutdown(ctx context.Context) {
if bt.traceProvider != nil {
- _ = bt.traceProvider.Shutdown(bt.c)
+ _ = bt.traceProvider.Shutdown(ctx)
}
if bt.metric != nil {
- _ = bt.metric.Shutdown(bt.c)
+ _ = bt.metric.Shutdown(ctx)
}
}
@@ -54,9 +53,8 @@ func (ot *OperatorTelemetry) StartMetricCollectors() error {
func Init(ctx context.Context, serviceName, gitTag, gitCommit string, otelEnabled bool, otelHost, otelPort string) (*OperatorTelemetry, error) {
log.Info().Msg("Initializing telemetry")
bt := &OperatorTelemetry{
- BaseTelemetry: &BaseTelemetry{
- c: ctx,
- }}
+ BaseTelemetry: &BaseTelemetry{},
+ }
res, err := resource.New(ctx,
resource.WithAttributes(
@@ -69,7 +67,7 @@ func Init(ctx context.Context, serviceName, gitTag, gitCommit string, otelEnable
return nil, fmt.Errorf("failed to create resource: %w", err)
}
- err = bt.initProviders(res, otelEnabled, otelHost, otelPort)
+ err = bt.initProviders(ctx, res, otelEnabled, otelHost, otelPort)
if err != nil {
return bt, err
}
@@ -80,7 +78,7 @@ func Init(ctx context.Context, serviceName, gitTag, gitCommit string, otelEnable
return bt, nil
}
-func createGRPCConn(ctx context.Context, enabled bool, otelHost string, otelPort string) (*grpc.ClientConn, error) {
+func createGRPCConn(enabled bool, otelHost string, otelPort string) (*grpc.ClientConn, error) {
if !enabled {
log.Info().Msg("otel disabled")
return nil, nil
@@ -102,44 +100,44 @@ func createGRPCConn(ctx context.Context, enabled bool, otelHost string, otelPort
return conn, err
}
-func (bt *BaseTelemetry) initProviders(res *resource.Resource, enabled bool, otelHost string, otelPort string) error {
- conn, err := createGRPCConn(bt.c, enabled, otelHost, otelPort)
+func (bt *BaseTelemetry) initProviders(ctx context.Context, res *resource.Resource, enabled bool, otelHost, otelPort string) error {
+ conn, err := createGRPCConn(enabled, otelHost, otelPort)
if err != nil {
return err
}
- err = bt.initTrace(conn, res)
+ err = bt.initTrace(ctx, conn, res)
if err != nil {
return err
}
- err = bt.initMetric(conn, res)
+ err = bt.initMetric(ctx, conn, res)
if err != nil {
return err
}
return nil
}
-func (bt *BaseTelemetry) initTrace(conn *grpc.ClientConn, res *resource.Resource) error {
+func (bt *BaseTelemetry) initTrace(ctx context.Context, conn *grpc.ClientConn, res *resource.Resource) error {
if conn == nil {
return nil
}
- err := bt.initOTLPTrace(conn, res)
+ err := bt.initOTLPTrace(ctx, conn, res)
if err != nil {
log.Error().Err(err).Msg("unable to init tracer")
}
log.Debug().Msg("tracer initialized")
return nil
}
-func (bt *BaseTelemetry) initMetric(conn *grpc.ClientConn, res *resource.Resource) error {
+func (bt *BaseTelemetry) initMetric(ctx context.Context, conn *grpc.ClientConn, res *resource.Resource) error {
if conn == nil {
return nil
}
- err := bt.initOTLPMetric(conn, res)
+ err := bt.initOTLPMetric(ctx, conn, res)
if err != nil {
log.Error().Err(err).Msg("unable to init metrics")
return err
@@ -148,9 +146,9 @@ func (bt *BaseTelemetry) initMetric(conn *grpc.ClientConn, res *resource.Resourc
return nil
}
-func (bt *BaseTelemetry) initOTLPTrace(conn *grpc.ClientConn, res *resource.Resource) error {
+func (bt *BaseTelemetry) initOTLPTrace(ctx context.Context, conn *grpc.ClientConn, res *resource.Resource) error {
// tracer grpc client
- traceExporter, err := otlptracegrpc.New(bt.c,
+ traceExporter, err := otlptracegrpc.New(ctx,
otlptracegrpc.WithInsecure(),
otlptracegrpc.WithGRPCConn(conn),
)
@@ -171,9 +169,9 @@ func (bt *BaseTelemetry) initOTLPTrace(conn *grpc.ClientConn, res *resource.Reso
return nil
}
-func (bt *BaseTelemetry) initOTLPMetric(conn *grpc.ClientConn, res *resource.Resource) error {
+func (bt *BaseTelemetry) initOTLPMetric(ctx context.Context, conn *grpc.ClientConn, res *resource.Resource) error {
mClient, err := otlpmetricgrpc.New(
- bt.c,
+ ctx,
otlpmetricgrpc.WithInsecure(),
otlpmetricgrpc.WithGRPCConn(conn),
)Feedback & Suggestions:
Overall, the changes improve the flexibility and maintainability of the code. Just ensure that all necessary updates are made to accommodate the new context handling approach. 👍 😼 Mergecat review of pkg/generator/applicationsets.go@@ -7,40 +7,39 @@ import (
"github.com/argoproj/argo-cd/v2/applicationset/utils"
argov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
- "github.com/zapier/kubechecks/pkg/container"
+ "github.com/rs/zerolog/log"
"k8s.io/apimachinery/pkg/util/strategicpatch"
"k8s.io/client-go/kubernetes"
"sigs.k8s.io/controller-runtime/pkg/client"
+
+ "github.com/zapier/kubechecks/pkg/container"
)
func New() AppsGenerator {
return &gen{}
}
-type gen struct {
-}
+type gen struct{}
type AppsGenerator interface {
GenerateApplicationSetApps(ctx context.Context, appset argov1alpha1.ApplicationSet, ctr *container.Container) ([]argov1alpha1.Application, error)
}
func (c *gen) GenerateApplicationSetApps(ctx context.Context, appset argov1alpha1.ApplicationSet, ctr *container.Container) ([]argov1alpha1.Application, error) {
-
appSetGenerators := getGenerators(ctx, *ctr.KubeClientSet.ControllerClient(), ctr.KubeClientSet.ClientSet(), ctr.Config.ArgoCDNamespace)
apps, appsetReason, err := generateApplications(appset, appSetGenerators)
if err != nil {
- fmt.Printf("error generating applications: %v, appset reason: %v", err, appsetReason)
+ log.Error().Err(err).Msgf("error generating applications, appset reason: %v", appsetReason)
return nil, fmt.Errorf("error generating applications: %w", err)
}
return apps, nil
}
// GetGenerators returns the generators that will be used to generate applications for the ApplicationSet
//
-// only support List and Clusters generators
+// only support List and Clusters generators.
func getGenerators(ctx context.Context, c client.Client, k8sClient kubernetes.Interface, namespace string) map[string]Generator {
-
terminalGenerators := map[string]Generator{
"List": NewListGenerator(),
"Clusters": NewClusterGenerator(c, ctx, k8sClient, namespace),
@@ -62,7 +61,7 @@ func getGenerators(ctx context.Context, c client.Client, k8sClient kubernetes.In
return topLevelGenerators
}
-// generateApplications generates applications from the ApplicationSet
+// generateApplications generates applications from the ApplicationSet.
func generateApplications(applicationSetInfo argov1alpha1.ApplicationSet, g map[string]Generator) (
[]argov1alpha1.Application, argov1alpha1.ApplicationSetReasonType, error,
) {
@@ -87,7 +86,7 @@ func generateApplications(applicationSetInfo argov1alpha1.ApplicationSet, g map[
for _, p := range a.Params {
app, err := renderer.RenderTemplateParams(tmplApplication, applicationSetInfo.Spec.SyncPolicy, p, applicationSetInfo.Spec.GoTemplate, applicationSetInfo.Spec.GoTemplateOptions)
if err != nil {
- //logCtx.WithError(err).WithField("params", a.Params).WithField("generator", requestedGenerator).
+ // logCtx.WithError(err).WithField("params", a.Params).WithField("generator", requestedGenerator).
// Error("error generating application from params")
if firstError == nil {
@@ -117,8 +116,8 @@ func generateApplications(applicationSetInfo argov1alpha1.ApplicationSet, g map[
}
}
- //logCtx.WithField("generator", requestedGenerator).Infof("generated %d applications", len(res))
- //logCtx.WithField("generator", requestedGenerator).Debugf("apps from generator: %+v", res)
+ // logCtx.WithField("generator", requestedGenerator).Infof("generated %d applications", len(res))
+ // logCtx.WithField("generator", requestedGenerator).Debugf("apps from generator: %+v", res)
}
return res, applicationSetReason, firstError
@@ -147,14 +146,12 @@ func getTempApplication(applicationSetTemplate argov1alpha1.ApplicationSetTempla
}
func applyTemplatePatch(app *argov1alpha1.Application, templatePatch string) (*argov1alpha1.Application, error) {
-
appString, err := json.Marshal(app)
if err != nil {
return nil, fmt.Errorf("error while marhsalling Application %w", err)
}
convertedTemplatePatch, err := utils.ConvertYAMLToJSON(templatePatch)
-
if err != nil {
return nil, fmt.Errorf("error while converting template to json %q: %w", convertedTemplatePatch, err)
}
@@ -164,7 +161,6 @@ func applyTemplatePatch(app *argov1alpha1.Application, templatePatch string) (*a
}
data, err := strategicpatch.StrategicMergePatch(appString, []byte(convertedTemplatePatch), argov1alpha1.Application{})
-
if err != nil {
return nil, fmt.Errorf("error while applying templatePatch template to json %q: %w", convertedTemplatePatch, err)
}Feedback & Suggestions:
Overall, the changes improve the code's readability and maintainability. Great job! 🎉 😼 Mergecat review of pkg/checks/diff/diff.go@@ -22,6 +22,7 @@ import (
"github.com/argoproj/gitops-engine/pkg/utils/kube"
"github.com/ghodss/yaml"
"github.com/go-logr/zerologr"
+ "github.com/pkg/errors"
"github.com/pmezard/go-difflib/difflib"
"github.com/rs/zerolog/log"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -72,27 +73,27 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) {
argoSettings, err := getArgoSettings(ctx, request)
if err != nil {
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to get Argo settings")
}
resources, err := getResources(ctx, request)
if err != nil {
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to get resources")
}
liveObjs, err := cmdutil.LiveObjects(resources)
if err != nil {
telemetry.SetError(span, err, "Get Argo Live Objects")
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to get live objects")
}
groupedObjs, err := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace)
if err != nil {
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to group objects by key")
}
if items, err = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.Name); err != nil {
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to group objects for diff")
}
var diffBuffer strings.Builder
@@ -107,13 +108,13 @@ func Check(ctx context.Context, request checks.Request) (msg.Result, error) {
diffRes, err := generateDiff(ctx, request, argoSettings, item)
if err != nil {
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to generate diff")
}
if diffRes.Modified || item.target == nil || item.live == nil {
err := addResourceDiffToMessage(ctx, &diffBuffer, resourceId, item, diffRes)
if err != nil {
- return msg.Result{}, err
+ return msg.Result{}, errors.Wrap(err, "failed to add diff to message")
}
processResources(item, diffRes, request, &added, &modified, &removed)
@@ -305,7 +306,7 @@ func groupObjsByKey(localObs []*unstructured.Unstructured, liveObjs []*unstructu
func groupObjsForDiff(resources []*argoappv1.ResourceDiff, objs map[kube.ResourceKey]*unstructured.Unstructured, items []objKeyLiveTarget, argoSettings *settings.Settings, appName string) ([]objKeyLiveTarget, error) {
resourceTracking := argo.NewResourceTracking()
for _, res := range resources {
- var live = &unstructured.Unstructured{}
+ live := &unstructured.Unstructured{}
if err := json.Unmarshal([]byte(res.NormalizedLiveState), &live); err != nil {
return nil, err
}
@@ -351,7 +352,7 @@ func (p *resourceInfoProvider) IsNamespaced(gk schema.GroupKind) (bool, error) {
}
// PrintDiff prints a diff between two unstructured objects to stdout using an external diff utility
-// Honors the diff utility set in the KUBECTL_EXTERNAL_DIFF environment variable
+// Honors the diff utility set in the KUBECTL_EXTERNAL_DIFF environment variable.
func PrintDiff(w io.Writer, live *unstructured.Unstructured, target *unstructured.Unstructured) error {
var err error
targetData := []byte("")
@@ -370,15 +371,15 @@ func PrintDiff(w io.Writer, live *unstructured.Unstructured, target *unstructure
}
}
- diff := difflib.UnifiedDiff{
+ theDiff := difflib.UnifiedDiff{
A: difflib.SplitLines(string(liveData)),
B: difflib.SplitLines(string(targetData)),
// FromFile: "Original",
// ToFile: "Current",
Context: 2,
}
- return difflib.WriteUnifiedDiff(w, diff)
- //return difflib.GetUnifiedDiffString(diff)
+ return difflib.WriteUnifiedDiff(w, theDiff)
+ // return difflib.GetUnifiedDiffString(diff)
// return dmp.DiffPrettyText(diff), nil
}Feedback & Suggestions:
Overall, the changes improve error handling and code clarity. Keep up the good work! 👍 😼 Mergecat review of pkg/generator/cluster.go@@ -5,21 +5,18 @@ import (
"fmt"
"time"
- "github.com/rs/zerolog/log"
-
+ "github.com/argoproj/argo-cd/v2/applicationset/utils"
+ argoappsetv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/argoproj/argo-cd/v2/util/settings"
-
+ "github.com/rs/zerolog/log"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"sigs.k8s.io/controller-runtime/pkg/client"
-
- "github.com/argoproj/argo-cd/v2/applicationset/utils"
- argoappsetv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)
const (
- ArgoCDSecretTypeLabel = "argocd.argoproj.io/secret-type"
+ ArgoCDSecretTypeLabel = "argocd.argoproj.io/secret-type" //nolint:gosec
ArgoCDSecretTypeCluster = "cluster"
)
@@ -28,7 +25,7 @@ var _ Generator = (*ClusterGenerator)(nil)
// ClusterGenerator generates Applications for some or all clusters registered with ArgoCD.
type ClusterGenerator struct {
client.Client
- ctx context.Context
+ ctx context.Context //nolint:containedctx
clientset kubernetes.Interface
// namespace is the Argo CD namespace
namespace string
@@ -38,7 +35,6 @@ type ClusterGenerator struct {
var render = &utils.Render{}
func NewClusterGenerator(c client.Client, ctx context.Context, clientset kubernetes.Interface, namespace string) Generator {
-
settingsManager := settings.NewSettingsManager(ctx, clientset, namespace)
g := &ClusterGenerator{
@@ -52,7 +48,7 @@ func NewClusterGenerator(c client.Client, ctx context.Context, clientset kuberne
}
// GetRequeueAfter never requeue the cluster generator because the `clusterSecretEventHandler` will requeue the appsets
-// when the cluster secrets change
+// when the cluster secrets change.
func (g *ClusterGenerator) GetRequeueAfter(_ *argoappsetv1alpha1.ApplicationSetGenerator) time.Duration {
return NoRequeueAfter
}
@@ -62,7 +58,6 @@ func (g *ClusterGenerator) GetTemplate(appSetGenerator *argoappsetv1alpha1.Appli
}
func (g *ClusterGenerator) GenerateParams(appSetGenerator *argoappsetv1alpha1.ApplicationSetGenerator, appSet *argoappsetv1alpha1.ApplicationSet) ([]map[string]interface{}, error) {
-
if appSetGenerator == nil {
return nil, EmptyAppSetGeneratorError
}
@@ -95,12 +90,10 @@ func (g *ClusterGenerator) GenerateParams(appSetGenerator *argoappsetv1alpha1.Ap
var secretsFound []corev1.Secret
for _, cluster := range clustersFromArgoCD.Items {
-
// If there is a secret for this cluster, then it's a non-local cluster, so it will be
// handled by the next step.
if secretForCluster, exists := clusterSecrets[cluster.Name]; exists {
secretsFound = append(secretsFound, secretForCluster)
-
} else if !ignoreLocalClusters {
// If there is no secret for the cluster, it's the local cluster, so handle it here.
params := map[string]interface{}{}
@@ -185,5 +178,4 @@ func (g *ClusterGenerator) getSecretsByClusterName(appSetGenerator *argoappsetv1
}
return res, nil
-
}Feedback & Suggestions:
Overall, the changes seem to focus on code cleanliness and linting adjustments. Ensure that any linting suppressions are well-justified and documented. 📜 😼 Mergecat review of pkg/vcs/gitlab_client/client_test.go@@ -10,6 +10,7 @@ import (
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/xanzy/go-gitlab"
+
gitlabMocks "github.com/zapier/kubechecks/mocks/gitlab_client/mocks"
"github.com/zapier/kubechecks/pkg/config"
"github.com/zapier/kubechecks/pkg/vcs"
@@ -57,7 +58,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email string
}
type args struct {
- ctx context.Context
repoName string
webhookUrl string
}
@@ -90,7 +90,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email: "",
},
args: args{
- ctx: context.TODO(),
repoName: "test",
webhookUrl: "https://dummywebhooks.local",
},
@@ -116,7 +115,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email: "",
},
args: args{
- ctx: context.TODO(),
repoName: "test",
webhookUrl: "https://dummywebhooks.local",
},
@@ -126,22 +124,24 @@ func TestClient_GetHookByUrl(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ ctx := context.Background()
+
c := &Client{
c: tt.fields.c,
cfg: tt.fields.cfg,
username: tt.fields.username,
email: tt.fields.email,
}
- got, err := c.GetHookByUrl(tt.args.ctx, tt.args.repoName, tt.args.webhookUrl)
- if !tt.wantErr(t, err, fmt.Sprintf("GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.repoName, tt.args.webhookUrl)) {
+ got, err := c.GetHookByUrl(ctx, tt.args.repoName, tt.args.webhookUrl)
+ if !tt.wantErr(t, err, fmt.Sprintf("GetHookByUrl(%v, %v)", tt.args.repoName, tt.args.webhookUrl)) {
return
}
- assert.Equalf(t, tt.want, got, "GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.repoName, tt.args.webhookUrl)
+ assert.Equalf(t, tt.want, got, "GetHookByUrl(%v, %v)", tt.args.repoName, tt.args.webhookUrl)
})
}
}
-// MockGitLabProjects is a generic function to mock Gitlab MergeRequest client methods
+// MockGitLabProjects is a generic function to mock Gitlab MergeRequest client methods.
func MockGitLabProjects(methodName string, returns []interface{}) *GLClient {
mockClient := new(gitlabMocks.MockProjectsServices)
mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...)
@@ -159,7 +159,6 @@ func TestClient_CreateHook(t *testing.T) {
email string
}
type args struct {
- ctx context.Context
repoName string
webhookUrl string
webhookSecret string
@@ -188,7 +187,6 @@ func TestClient_CreateHook(t *testing.T) {
email: "",
},
args: args{
- ctx: context.TODO(),
repoName: "main",
webhookUrl: "https://dummywebhooks.local",
webhookSecret: "",
@@ -209,7 +207,6 @@ func TestClient_CreateHook(t *testing.T) {
email: "",
},
args: args{
- ctx: context.TODO(),
repoName: "main",
webhookUrl: "https://dummywebhooks.local",
webhookSecret: "",
@@ -225,7 +222,7 @@ func TestClient_CreateHook(t *testing.T) {
username: tt.fields.username,
email: tt.fields.email,
}
- tt.wantErr(t, c.CreateHook(tt.args.ctx, tt.args.repoName, tt.args.webhookUrl, tt.args.webhookSecret), fmt.Sprintf("CreateHook(%v, %v, %v, %v)", tt.args.ctx, tt.args.repoName, tt.args.webhookUrl, tt.args.webhookSecret))
+ tt.wantErr(t, c.CreateHook(context.Background(), tt.args.repoName, tt.args.webhookUrl, tt.args.webhookSecret), fmt.Sprintf("CreateHook(%v, %v, %v)", tt.args.repoName, tt.args.webhookUrl, tt.args.webhookSecret))
})
}
}Feedback & Suggestions:
Overall, the changes simplify the test cases but consider the trade-offs in flexibility for context management. Keep up the good work! 👍 😼 Mergecat review of pkg/git/repo.go@@ -68,14 +68,14 @@ func (r *Repo) Clone(ctx context.Context) error {
args = append(args, "--branch", r.BranchName)
}
- cmd := r.execCommand("git", args...)
+ cmd := r.execGit(args...)
out, err := cmd.CombinedOutput()
if err != nil {
log.Error().Err(err).Msgf("unable to clone repository, %s", out)
return err
}
- if log.Trace().Enabled() {
+ if log.Trace().Enabled() { //nolint: zerologlint
if err = filepath.WalkDir(r.Directory, printFile); err != nil {
log.Warn().Err(err).Msg("failed to walk directory")
}
@@ -89,13 +89,13 @@ func printFile(s string, d fs.DirEntry, err error) error {
return err
}
if !d.IsDir() {
- println(s)
+ log.Debug().Msg(s)
}
return nil
}
func (r *Repo) GetRemoteHead() (string, error) {
- cmd := r.execCommand("git", "symbolic-ref", "refs/remotes/origin/HEAD", "--short")
+ cmd := r.execGit("symbolic-ref", "refs/remotes/origin/HEAD", "--short")
out, err := cmd.CombinedOutput()
if err != nil {
return "", errors.Wrap(err, "failed to determine which branch HEAD points to")
@@ -118,7 +118,7 @@ func (r *Repo) MergeIntoTarget(ctx context.Context, sha string) error {
))
defer span.End()
- cmd := r.execCommand("git", "merge", sha)
+ cmd := r.execGit("merge", sha)
out, err := cmd.CombinedOutput()
if err != nil {
telemetry.SetError(span, err, "merge commit into branch")
@@ -129,18 +129,18 @@ func (r *Repo) MergeIntoTarget(ctx context.Context, sha string) error {
return nil
}
-func (r *Repo) Update(ctx context.Context) error {
- cmd := r.execCommand("git", "pull")
+func (r *Repo) Update() error {
+ cmd := r.execGit("pull")
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stdout
return cmd.Run()
}
-func (r *Repo) execCommand(name string, args ...string) *exec.Cmd {
+func (r *Repo) execGit(args ...string) *exec.Cmd {
argsToLog := r.censorVcsToken(args)
log.Debug().Strs("args", argsToLog).Msg("building command")
- cmd := exec.Command(name, args...)
+ cmd := exec.Command("git", args...)
if r.Directory != "" {
cmd.Dir = r.Directory
}
@@ -176,7 +176,7 @@ func censorVcsToken(cfg config.ServerConfig, args []string) []string {
return argsToLog
}
-// SetCredentials ensures Git auth is set up for cloning
+// SetCredentials ensures Git auth is set up for cloning.
func SetCredentials(cfg config.ServerConfig, vcsClient vcs.Client) error {
email := vcsClient.Email()
username := vcsClient.Username()
@@ -200,9 +200,7 @@ func SetCredentials(cfg config.ServerConfig, vcsClient vcs.Client) error {
homedir, err := os.UserHomeDir()
if err != nil {
- if err != nil {
- return errors.Wrap(err, "unable to get home directory")
- }
+ return errors.Wrap(err, "unable to get home directory")
}
outfile, err := os.Create(fmt.Sprintf("%s/.git-credentials", homedir))
if err != nil {
@@ -250,14 +248,14 @@ func getCloneUrl(user string, cfg config.ServerConfig) (string, error) {
return fmt.Sprintf("%s://%s:%s@%s", scheme, user, vcsToken, hostname), nil
}
-// GetListOfChangedFiles returns a list of files that have changed between the current branch and the target branch
+// GetListOfChangedFiles returns a list of files that have changed between the current branch and the target branch.
func (r *Repo) GetListOfChangedFiles(ctx context.Context) ([]string, error) {
_, span := tracer.Start(ctx, "RepoGetListOfChangedFiles")
defer span.End()
var fileList []string
- cmd := r.execCommand("git", "diff", "--name-only", fmt.Sprintf("%s/%s", "origin", r.BranchName))
+ cmd := r.execGit("diff", "--name-only", fmt.Sprintf("%s/%s", "origin", r.BranchName))
pipe, _ := cmd.StdoutPipe()
var wg sync.WaitGroup
scanner := bufio.NewScanner(pipe)Feedback & Suggestions:
Overall, these changes improve the clarity, consistency, and maintainability of the code. Keep up the good work! 👍 😼 Mergecat review of pkg/vcs/github_client/client_test.go@@ -11,12 +11,13 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
+
githubMocks "github.com/zapier/kubechecks/mocks/github_client/mocks"
"github.com/zapier/kubechecks/pkg/config"
"github.com/zapier/kubechecks/pkg/vcs"
)
-// MockGitHubMethod is a generic function to mock GitHub client methods
+// MockGitHubMethod is a generic function to mock GitHub client methods.
func MockGitHubMethod(methodName string, returns []interface{}) *GClient {
mockClient := new(githubMocks.MockRepositoriesServices)
mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...)
@@ -26,7 +27,7 @@ func MockGitHubMethod(methodName string, returns []interface{}) *GClient {
}
}
-// MockGitHubPullRequestMethod is a generic function to mock GitHub client methods
+// MockGitHubPullRequestMethod is a generic function to mock GitHub client methods.
func MockGitHubPullRequestMethod(methodName string, returns []interface{}) *GClient {
mockClient := new(githubMocks.MockPullRequestsServices)
mockClient.On(methodName, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(returns...)
@@ -62,7 +63,6 @@ func TestParseRepo(t *testing.T) {
}
for _, tc := range testcases {
- tc := tc
t.Run(tc.name, func(t *testing.T) {
owner, repo := parseRepo(tc.input)
assert.Equal(t, tc.expectedOwner, owner)
@@ -80,7 +80,6 @@ func TestClient_CreateHook(t *testing.T) {
email string
}
type args struct {
- ctx context.Context
ownerAndRepoName string
webhookUrl string
webhookSecret string
@@ -99,7 +98,8 @@ func TestClient_CreateHook(t *testing.T) {
[]interface{}{
&github.Hook{},
&github.Response{Response: &http.Response{StatusCode: http.StatusOK}},
- nil}),
+ nil,
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -108,7 +108,6 @@ func TestClient_CreateHook(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
webhookSecret: "dummy-webhook-secret",
@@ -123,7 +122,8 @@ func TestClient_CreateHook(t *testing.T) {
[]interface{}{
nil,
&github.Response{Response: &http.Response{StatusCode: http.StatusBadRequest}},
- fmt.Errorf("mock bad request")}),
+ fmt.Errorf("mock bad request"),
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -132,7 +132,6 @@ func TestClient_CreateHook(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
webhookSecret: "dummy-webhook-secret",
@@ -147,7 +146,8 @@ func TestClient_CreateHook(t *testing.T) {
[]interface{}{
nil,
nil,
- fmt.Errorf("mock network error")}),
+ fmt.Errorf("mock network error"),
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -156,7 +156,6 @@ func TestClient_CreateHook(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
webhookSecret: "dummy-webhook-secret",
@@ -166,14 +165,15 @@ func TestClient_CreateHook(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ ctx := context.Background()
c := &Client{
shurcoolClient: tt.fields.shurcoolClient,
googleClient: tt.fields.googleClient,
cfg: tt.fields.cfg,
username: tt.fields.username,
email: tt.fields.email,
}
- tt.wantErr(t, c.CreateHook(tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl, tt.args.webhookSecret), fmt.Sprintf("CreateHook(%v, %v, %v, %v)", tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl, tt.args.webhookSecret))
+ tt.wantErr(t, c.CreateHook(ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl, tt.args.webhookSecret), fmt.Sprintf("CreateHook(%v, %v, %v, %v)", ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl, tt.args.webhookSecret))
})
}
}
@@ -187,7 +187,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email string
}
type args struct {
- ctx context.Context
ownerAndRepoName string
webhookUrl string
}
@@ -216,7 +215,8 @@ func TestClient_GetHookByUrl(t *testing.T) {
},
},
&github.Response{Response: &http.Response{StatusCode: http.StatusOK}},
- nil}),
+ nil,
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -225,7 +225,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
},
@@ -253,7 +252,8 @@ func TestClient_GetHookByUrl(t *testing.T) {
},
},
&github.Response{Response: &http.Response{StatusCode: http.StatusOK}},
- nil}),
+ nil,
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -262,7 +262,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
},
@@ -277,7 +276,8 @@ func TestClient_GetHookByUrl(t *testing.T) {
[]interface{}{
nil,
&github.Response{Response: &http.Response{StatusCode: http.StatusOK}},
- nil}),
+ nil,
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -286,7 +286,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
},
@@ -301,7 +300,8 @@ func TestClient_GetHookByUrl(t *testing.T) {
[]interface{}{
nil,
&github.Response{Response: &http.Response{StatusCode: http.StatusBadRequest}},
- fmt.Errorf("mock bad request")}),
+ fmt.Errorf("mock bad request"),
+ }),
cfg: config.ServerConfig{
VcsToken: "ghp_helloworld",
VcsType: "github",
@@ -310,7 +310,6 @@ func TestClient_GetHookByUrl(t *testing.T) {
email: "[email protected]",
},
args: args{
- ctx: context.Background(),
ownerAndRepoName: "https://dummy-bot:********@github.com/dummy-bot-zapier/test-repo.git",
webhookUrl: "https://dummywebhooks.local",
},
@@ -320,18 +319,19 @@ func TestClient_GetHookByUrl(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ ctx := context.Background()
c := &Client{
shurcoolClient: tt.fields.shurcoolClient,
googleClient: tt.fields.googleClient,
cfg: tt.fields.cfg,
username: tt.fields.username,
email: tt.fields.email,
}
- got, err := c.GetHookByUrl(tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)
- if !tt.wantErr(t, err, fmt.Sprintf("GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)) {
+ got, err := c.GetHookByUrl(ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)
+ if !tt.wantErr(t, err, fmt.Sprintf("GetHookByUrl(%v, %v, %v)", ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)) {
return
}
- assert.Equalf(t, tt.want, got, "GetHookByUrl(%v, %v, %v)", tt.args.ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)
+ assert.Equalf(t, tt.want, got, "GetHookByUrl(%v, %v, %v)", ctx, tt.args.ownerAndRepoName, tt.args.webhookUrl)
})
}
}
@@ -345,7 +345,6 @@ func TestClient_buildRepoFromComment_HappyPath(t *testing.T) {
email string
}
type args struct {
- context context.Context
comment *github.IssueCommentEvent
}
tests := []struct {
@@ -392,14 +391,14 @@ func TestClient_buildRepoFromComment_HappyPath(t *testing.T) {
},
},
&github.Response{Response: &http.Response{StatusCode: http.StatusOK}},
- nil},
+ nil,
+ },
),
cfg: config.ServerConfig{},
username: "unittestuser",
email: "[email protected]",
},
args: args{
- context: context.TODO(),
comment: &github.IssueCommentEvent{
Issue: &github.Issue{
URL: github.String("https://github.com/zapier/kubechecks/pull/250"),
@@ -444,7 +443,7 @@ func TestClient_buildRepoFromComment_HappyPath(t *testing.T) {
username: tt.fields.username,
email: tt.fields.email,
}
- actual, err := c.buildRepoFromComment(tt.args.context, tt.args.comment)
+ actual, err := c.buildRepoFromComment(context.TODO(), tt.args.comment)
require.NoError(t, err)
assert.Equal(t, tt.want.Name, actual.Name)
assert.Equal(t, tt.want.Labels, actual.Labels)Feedback & Suggestions:
Overall, these changes improve the code's readability and maintainability. Keep up the good work! 👍 😼 Mergecat review of pkg/generator/cluster_test.go@@ -5,18 +5,16 @@ import (
"fmt"
"testing"
+ "github.com/argoproj/argo-cd/v2/applicationset/utils"
+ argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
+ kubefake "k8s.io/client-go/kubernetes/fake"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
-
- kubefake "k8s.io/client-go/kubernetes/fake"
-
- "github.com/argoproj/argo-cd/v2/applicationset/utils"
- argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-
- "github.com/stretchr/testify/assert"
)
type possiblyErroringFakeCtrlRuntimeClient struct {
@@ -104,11 +102,15 @@ func TestGenerateParams(t *testing.T) {
"aaa": "{{ server }}",
"no-op": "{{ this-does-not-exist }}",
}, expected: []map[string]interface{}{
- {"values.lol1": "lol", "values.lol2": "{{values.lol1}}{{values.lol1}}", "values.lol3": "{{values.lol2}}{{values.lol2}}{{values.lol2}}", "values.foo": "bar", "values.bar": "production", "values.no-op": "{{ this-does-not-exist }}", "values.bat": "production", "values.aaa": "https://production-01.example.com", "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production"},
+ {
+ "values.lol1": "lol", "values.lol2": "{{values.lol1}}{{values.lol1}}", "values.lol3": "{{values.lol2}}{{values.lol2}}{{values.lol2}}", "values.foo": "bar", "values.bar": "production", "values.no-op": "{{ this-does-not-exist }}", "values.bat": "production", "values.aaa": "https://production-01.example.com", "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production",
+ },
- {"values.lol1": "lol", "values.lol2": "{{values.lol1}}{{values.lol1}}", "values.lol3": "{{values.lol2}}{{values.lol2}}{{values.lol2}}", "values.foo": "bar", "values.bar": "staging", "values.no-op": "{{ this-does-not-exist }}", "values.bat": "staging", "values.aaa": "https://staging-01.example.com", "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging"},
+ {
+ "values.lol1": "lol", "values.lol2": "{{values.lol1}}{{values.lol1}}", "values.lol3": "{{values.lol2}}{{values.lol2}}{{values.lol2}}", "values.foo": "bar", "values.bar": "staging", "values.no-op": "{{ this-does-not-exist }}", "values.bat": "staging", "values.aaa": "https://staging-01.example.com", "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging",
+ },
{"values.lol1": "lol", "values.lol2": "{{values.lol1}}{{values.lol1}}", "values.lol3": "{{values.lol2}}{{values.lol2}}{{values.lol2}}", "values.foo": "bar", "values.bar": "{{ metadata.annotations.foo.argoproj.io }}", "values.no-op": "{{ this-does-not-exist }}", "values.bat": "{{ metadata.labels.environment }}", "values.aaa": "https://kubernetes.default.svc", "nameNormalized": "in-cluster", "name": "in-cluster", "server": "https://kubernetes.default.svc"},
},
@@ -124,11 +126,15 @@ func TestGenerateParams(t *testing.T) {
},
values: nil,
expected: []map[string]interface{}{
- {"name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production"},
+ {
+ "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production",
+ },
- {"name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging"},
+ {
+ "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging",
+ },
},
clientError: false,
expectedError: nil,
@@ -144,8 +150,10 @@ func TestGenerateParams(t *testing.T) {
"foo": "bar",
},
expected: []map[string]interface{}{
- {"values.foo": "bar", "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production"},
+ {
+ "values.foo": "bar", "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production",
+ },
},
clientError: false,
expectedError: nil,
@@ -168,10 +176,14 @@ func TestGenerateParams(t *testing.T) {
"foo": "bar",
},
expected: []map[string]interface{}{
- {"values.foo": "bar", "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging"},
- {"values.foo": "bar", "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production"},
+ {
+ "values.foo": "bar", "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging",
+ },
+ {
+ "values.foo": "bar", "name": "production_01/west", "nameNormalized": "production-01-west", "server": "https://production-01.example.com", "metadata.labels.environment": "production", "metadata.labels.org": "bar",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "production",
+ },
},
clientError: false,
expectedError: nil,
@@ -197,8 +209,10 @@ func TestGenerateParams(t *testing.T) {
"name": "baz",
},
expected: []map[string]interface{}{
- {"values.name": "baz", "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
- "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging"},
+ {
+ "values.name": "baz", "name": "staging-01", "nameNormalized": "staging-01", "server": "https://staging-01.example.com", "metadata.labels.environment": "staging", "metadata.labels.org": "foo",
+ "metadata.labels.argocd.argoproj.io/secret-type": "cluster", "metadata.annotations.foo.argoproj.io": "staging",
+ },
},
clientError: false,
expectedError: nil,
@@ -220,9 +234,7 @@ func TestGenerateParams(t *testing.T) {
}
for _, testCase := range testCases {
-
t.Run(testCase.name, func(t *testing.T) {
-
appClientset := kubefake.NewSimpleClientset(runtimeClusters...)
fakeClient := fake.NewClientBuilder().WithObjects(clusters...).Build()
@@ -231,7 +243,7 @@ func TestGenerateParams(t *testing.T) {
testCase.clientError,
}
- var clusterGenerator = NewClusterGenerator(cl, context.Background(), appClientset, "namespace")
+ clusterGenerator := NewClusterGenerator(cl, context.Background(), appClientset, "namespace")
applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
@@ -250,10 +262,9 @@ func TestGenerateParams(t *testing.T) {
if testCase.expectedError != nil {
assert.EqualError(t, err, testCase.expectedError.Error())
} else {
- assert.NoError(t, err)
+ require.NoError(t, err)
assert.ElementsMatch(t, testCase.expected, got)
}
-
})
}
}
@@ -594,9 +605,7 @@ func TestGenerateParamsGoTemplate(t *testing.T) {
}
for _, testCase := range testCases {
-
t.Run(testCase.name, func(t *testing.T) {
-
appClientset := kubefake.NewSimpleClientset(runtimeClusters...)
fakeClient := fake.NewClientBuilder().WithObjects(clusters...).Build()
@@ -605,7 +614,7 @@ func TestGenerateParamsGoTemplate(t *testing.T) {
testCase.clientError,
}
- var clusterGenerator = NewClusterGenerator(cl, context.Background(), appClientset, "namespace")
+ clusterGenerator := NewClusterGenerator(cl, context.Background(), appClientset, "namespace")
applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
@@ -626,10 +635,9 @@ func TestGenerateParamsGoTemplate(t *testing.T) {
if testCase.expectedError != nil {
assert.EqualError(t, err, testCase.expectedError.Error())
} else {
- assert.NoError(t, err)
+ require.NoError(t, err)
assert.ElementsMatch(t, testCase.expected, got)
}
-
})
}
}Feedback & Suggestions:
Overall, these changes enhance the readability and reliability of the test code. Great job! 🎉 Dependency ReviewClick to read mergecats review!No suggestions found |
No description provided.