Ensures that Helm charts are correctly resolved before sync. Fixes ar…

…goproj#2758 (argoproj#2760)

server/application/application.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/Masterminds/semver"
 	jsonpatch "github.com/evanphx/json-patch"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
@@ -38,6 +39,7 @@ import (
 	"github.com/argoproj/argo-cd/util/db"
 	"github.com/argoproj/argo-cd/util/diff"
 	"github.com/argoproj/argo-cd/util/git"
+	"github.com/argoproj/argo-cd/util/helm"
 	"github.com/argoproj/argo-cd/util/kube"
 	"github.com/argoproj/argo-cd/util/lua"
 	"github.com/argoproj/argo-cd/util/rbac"
@@ -944,14 +946,9 @@ func (s *Server) Sync(ctx context.Context, syncReq *application.ApplicationSyncR
 			return nil, status.Errorf(codes.FailedPrecondition, "Cannot sync to %s: auto-sync currently set to %s", syncReq.Revision, a.Spec.Source.TargetRevision)
 		}
 	}
-
-	revision := a.Spec.Source.TargetRevision
-	displayRevision := revision
-	if !a.Spec.Source.IsHelm() {
-		revision, displayRevision, err = s.resolveRevision(ctx, a, syncReq)
-		if err != nil {
-			return nil, status.Errorf(codes.FailedPrecondition, err.Error())
-		}
+	revision, displayRevision, err := s.resolveRevision(ctx, a, syncReq)
+	if err != nil {
+		return nil, status.Errorf(codes.FailedPrecondition, err.Error())
 	}
 	op := appv1.Operation{
 		Sync: &appv1.SyncOperation{
@@ -1030,24 +1027,49 @@ func (s *Server) resolveRevision(ctx context.Context, app *appv1.Application, sy
 	if ambiguousRevision == "" {
 		ambiguousRevision = app.Spec.Source.TargetRevision
 	}
-	if git.IsCommitSHA(ambiguousRevision) {
-		// If it's already a commit SHA, then no need to look it up
-		return ambiguousRevision, ambiguousRevision, nil
-	}
-	repo, err := s.db.GetRepository(ctx, app.Spec.Source.RepoURL)
-	if err != nil {
-		return "", "", err
-	}
-	gitClient, err := git.NewClient(repo.Repo, repo.GetGitCreds(), repo.IsInsecure(), repo.IsLFSEnabled())
-	if err != nil {
-		return "", "", err
-	}
-	commitSHA, err := gitClient.LsRemote(ambiguousRevision)
-	if err != nil {
-		return "", "", err
+	var revision string
+	if app.Spec.Source.IsHelm() {
+		repo, err := s.db.GetRepository(ctx, app.Spec.Source.RepoURL)
+		if err != nil {
+			return "", "", err
+		}
+		client := helm.NewClient(repo.Repo, repo.GetHelmCreds())
+		index, err := client.GetIndex()
+		if err != nil {
+			return "", "", err
+		}
+		entries, err := index.GetEntries(app.Spec.Source.Chart)
+		if err != nil {
+			return "", "", err
+		}
+		constraints, err := semver.NewConstraint(ambiguousRevision)
+		if err != nil {
+			return "", "", err
+		}
+		version, err := entries.MaxVersion(constraints)
+		if err != nil {
+			return "", "", err
+		}
+		return version.String(), fmt.Sprintf("%v (%v)", ambiguousRevision, version.String()), nil
+	} else {
+		if git.IsCommitSHA(ambiguousRevision) {
+			// If it's already a commit SHA, then no need to look it up
+			return ambiguousRevision, ambiguousRevision, nil
+		}
+		repo, err := s.db.GetRepository(ctx, app.Spec.Source.RepoURL)
+		if err != nil {
+			return "", "", err
+		}
+		gitClient, err := git.NewClient(repo.Repo, repo.GetGitCreds(), repo.IsInsecure(), repo.IsLFSEnabled())
+		if err != nil {
+			return "", "", err
+		}
+		revision, err = gitClient.LsRemote(ambiguousRevision)
+		if err != nil {
+			return "", "", err
+		}
+		return revision, fmt.Sprintf("%s (%s)", ambiguousRevision, revision), nil
 	}
-	displayRevision := fmt.Sprintf("%s (%s)", ambiguousRevision, commitSHA)
-	return commitSHA, displayRevision, nil
 }
 
 func (s *Server) TerminateOperation(ctx context.Context, termOpReq *application.OperationTerminateRequest) (*application.OperationTerminateResponse, error) {

server/application/application_test.go

@@ -317,6 +317,28 @@ func TestSyncAndTerminate(t *testing.T) {
 	assert.Equal(t, appsv1.OperationTerminating, app.Status.OperationState.Phase)
 }
 
+func TestSyncHelm(t *testing.T) {
+	ctx := context.Background()
+	appServer := newTestAppServer()
+	testApp := newTestApp()
+	testApp.Spec.Source.RepoURL = "https://argoproj.github.io/argo-helm"
+	testApp.Spec.Source.Path = ""
+	testApp.Spec.Source.Chart = "argo-cd"
+	testApp.Spec.Source.TargetRevision = "0.7.*"
+
+	app, err := appServer.Create(ctx, &application.ApplicationCreateRequest{Application: *testApp})
+	assert.NoError(t, err)
+
+	app, err = appServer.Sync(ctx, &application.ApplicationSyncRequest{Name: &app.Name})
+	assert.NoError(t, err)
+	assert.NotNil(t, app)
+	assert.NotNil(t, app.Operation)
+
+	events, err := appServer.kubeclientset.CoreV1().Events(appServer.ns).List(metav1.ListOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, "Unknown user initiated sync to 0.7.* (0.7.2)", events.Items[1].Message)
+}
+
 func TestRollbackApp(t *testing.T) {
 	testApp := newTestApp()
 	testApp.Status.History = []appsv1.RevisionHistory{{

util/helm/client.go

@@ -164,7 +164,7 @@ func (c *nativeHelmChart) ExtractChart(chart string, version *semver.Version) (s
 func (c *nativeHelmChart) GetIndex() (*Index, error) {
 	cachedIndex, found := indexCache.Get(c.repoURL)
 	if found {
-		log.WithFields(log.Fields{"url": c.repoURL}).Debug("Index cache hit")
+		log.WithFields(log.Fields{"url": c.repoURL}).Debug("index cache hit")
 		i := cachedIndex.(Index)
 		return &i, nil
 	}
@@ -198,13 +198,13 @@ func (c *nativeHelmChart) GetIndex() (*Index, error) {
 	defer func() { _ = resp.Body.Close() }()
 
 	if resp.StatusCode != 200 {
-		return nil, errors.New("failed to get Index: " + resp.Status)
+		return nil, errors.New("failed to get index: " + resp.Status)
 	}
 
 	index := &Index{}
 	err = yaml.NewDecoder(resp.Body).Decode(index)
 
-	log.WithFields(log.Fields{"seconds": time.Since(start).Seconds()}).Info("took to get Index")
+	log.WithFields(log.Fields{"seconds": time.Since(start).Seconds()}).Info("took to get index")
 
 	indexCache.Set(c.repoURL, *index, cache.DefaultExpiration)