diff --git a/pkg/argo_client/manifests.go b/pkg/argo_client/manifests.go index 2d82847f..3ea9611b 100644 --- a/pkg/argo_client/manifests.go +++ b/pkg/argo_client/manifests.go @@ -42,10 +42,10 @@ func (a *ArgoClient) GetManifests(ctx context.Context, name string, app v1alpha1 getManifestsDuration.WithLabelValues(name).Observe(duration.Seconds()) }() - contents, refs := a.preprocessSources(&app, pullRequest) + contentRefs, refs := a.preprocessSources(&app, pullRequest) var manifests []string - for _, source := range contents { + for _, source := range contentRefs { moreManifests, err := a.generateManifests(ctx, app, source, refs, pullRequest, getRepo) if err != nil { return nil, errors.Wrap(err, "failed to generate manifests") @@ -57,6 +57,9 @@ func (a *ArgoClient) GetManifests(ctx context.Context, name string, app v1alpha1 return manifests, nil } +// preprocessSources splits the content sources from the ref sources, and transforms source refs that point at the pull +// request's base into refs that point at the pull request's head. This is necessary to generate manifests based on what +// the world will look like _after_ the branch gets merged in. func (a *ArgoClient) preprocessSources(app *v1alpha1.Application, pullRequest vcs.PullRequest) ([]v1alpha1.ApplicationSource, []v1alpha1.ApplicationSource) { if !app.Spec.HasMultipleSources() { return []v1alpha1.ApplicationSource{app.Spec.GetSource()}, nil @@ -72,8 +75,13 @@ func (a *ArgoClient) preprocessSources(app *v1alpha1.Application, pullRequest vc continue } - if source.TargetRevision == pullRequest.BaseRef { - source.TargetRevision = pullRequest.HeadRef + // If the source is the same as the _base_ of the pull request, + // then change the revision to the pull request's target. + // This ensures that the manifests represent the result of the request. + if pkg.AreSameRepos(source.RepoURL, pullRequest.CloneURL) { + if source.TargetRevision == pullRequest.BaseRef { + source.TargetRevision = pullRequest.HeadRef + } } refSources = append(refSources, source) @@ -82,11 +90,14 @@ func (a *ArgoClient) preprocessSources(app *v1alpha1.Application, pullRequest vc return contentSources, refSources } +// generateManifests generates an Application along with all of its files, and sends it to the ArgoCD +// Repository service to be transformed into raw kubernetes manifests. This allows us to take advantage of server +// configuration and credentials. func (a *ArgoClient) generateManifests(ctx context.Context, app v1alpha1.Application, source v1alpha1.ApplicationSource, refs []v1alpha1.ApplicationSource, pullRequest vcs.PullRequest, getRepo func(ctx context.Context, cloneURL string, branchName string) (*git.Repo, error)) ([]string, error) { // multisource apps must adhere to the following rules: // 1. first source must be a non-ref source // 2. there must be one and only one non-ref source - // 3. ref sources that match the pull requests's repo and target branch need to have their target branch swapped to the head branch of the pull request + // 3. ref sources that match the pull requests' repo and target branch need to have their target branch swapped to the head branch of the pull request clusterCloser, clusterClient := a.GetClusterClient() defer clusterCloser.Close() @@ -111,7 +122,7 @@ func (a *ArgoClient) generateManifests(ctx context.Context, app v1alpha1.Applica argoDB := db.NewDB(a.namespace, settingsMgr, a.k8s) repoTarget := source.TargetRevision - if areSameRepos(source.RepoURL, pullRequest.CloneURL) && areSameTargetRef(source.TargetRevision, pullRequest.BaseRef) { + if pkg.AreSameRepos(source.RepoURL, pullRequest.CloneURL) && areSameTargetRef(source.TargetRevision, pullRequest.BaseRef) { repoTarget = pullRequest.HeadRef } @@ -324,7 +335,7 @@ func packageApp(ctx context.Context, source v1alpha1.ApplicationSource, refs []v } refRepo := repo - if !areSameRepos(ref.RepoURL, repo.CloneURL) { + if !pkg.AreSameRepos(ref.RepoURL, repo.CloneURL) { refRepo, err = getRepo(ctx, ref.RepoURL, ref.TargetRevision) if err != nil { return "", errors.Wrapf(err, "failed to clone repo: %q", ref.RepoURL) @@ -350,6 +361,11 @@ func packageApp(ctx context.Context, source v1alpha1.ApplicationSource, refs []v continue } + if strings.Contains(valueFile, "://") { + // todo: is there anything to do here? + continue + } + relPath, err := filepath.Rel(source.Path, valueFile) if err != nil { return "", errors.Wrap(err, "failed to calculate relative path") @@ -418,22 +434,6 @@ func sendFile(ctx context.Context, sender sender, file *os.File) error { return nil } -func areSameRepos(url1, url2 string) bool { - repo1, err := pkg.Canonicalize(url1) - if err != nil { - log.Warn().Msgf("failed to canonicalize %q", url1) - return false - } - - repo2, err := pkg.Canonicalize(url2) - if err != nil { - log.Warn().Msgf("failed to canonicalize %q", url2) - return false - } - - return repo1 == repo2 -} - func areSameTargetRef(ref1, ref2 string) bool { return ref1 == ref2 } diff --git a/pkg/argo_client/manifests_test.go b/pkg/argo_client/manifests_test.go new file mode 100644 index 00000000..dfa0d0a2 --- /dev/null +++ b/pkg/argo_client/manifests_test.go @@ -0,0 +1,52 @@ +package argo_client + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAreSameTargetRef(t *testing.T) { + testcases := map[string]struct { + ref1, ref2 string + expected bool + }{ + "same": {"one", "one", true}, + "different": {"one", "two", false}, + } + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + actual := areSameTargetRef(tc.ref1, tc.ref2) + assert.Equal(t, tc.expected, actual) + }) + } +} + +func TestSplitRefFromPath(t *testing.T) { + testcases := map[string]struct { + input string + refName, path string + err error + }{ + "simple": { + "$values/charts/prometheus/values.yaml", "values", "charts/prometheus/values.yaml", nil, + }, + "too-short": { + "$values", "", "", ErrInvalidSourceRef, + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + ref, path, err := splitRefFromPath(tc.input) + if tc.err != nil { + assert.EqualError(t, err, tc.err.Error()) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, tc.refName, ref) + assert.Equal(t, tc.path, path) + }) + } +} diff --git a/pkg/repoUrl.go b/pkg/repoUrl.go index 99a3723f..6ee8c188 100644 --- a/pkg/repoUrl.go +++ b/pkg/repoUrl.go @@ -7,6 +7,7 @@ import ( "github.com/chainguard-dev/git-urls" "github.com/pkg/errors" + "github.com/rs/zerolog/log" ) type RepoURL struct { @@ -51,3 +52,19 @@ func Canonicalize(cloneURL string) (RepoURL, error) { return parsed, nil } + +func AreSameRepos(url1, url2 string) bool { + repo1, err := Canonicalize(url1) + if err != nil { + log.Warn().Msgf("failed to canonicalize %q", url1) + return false + } + + repo2, err := Canonicalize(url2) + if err != nil { + log.Warn().Msgf("failed to canonicalize %q", url2) + return false + } + + return repo1 == repo2 +} diff --git a/pkg/repoUrl_test.go b/pkg/repoUrl_test.go index a4bc786e..9ac46b6c 100644 --- a/pkg/repoUrl_test.go +++ b/pkg/repoUrl_test.go @@ -70,3 +70,23 @@ func TestNormalizeStrings(t *testing.T) { }) } } + +func TestAreSameRepos(t *testing.T) { + testcases := map[string]struct { + input1, input2 string + expected bool + }{ + "empty": {"", "", true}, + "empty1": {"", "blah", false}, + "empty2": {"blah", "", false}, + "git-to-git": {"git@github.com:zapier/kubechecks.git", "git@github.com:zapier/kubechecks.git", true}, + "no-git-suffix-to-git": {"git@github.com:zapier/kubechecks", "git@github.com:zapier/kubechecks.git", true}, + "https-to-git": {"https://github.com/zapier/kubechecks", "git@github.com:zapier/kubechecks.git", true}, + } + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + actual := AreSameRepos(tc.input1, tc.input2) + assert.Equal(t, tc.expected, actual) + }) + } +}