From 620d9560386a67d76c686497dcafe3a2758fad5c Mon Sep 17 00:00:00 2001
From: Alex Collins <alexec@users.noreply.github.com>
Date: Mon, 18 Nov 2019 10:08:52 -0800
Subject: [PATCH] Returns a clearer error on invalid Helm version. Closes #2665
 and #2736 (#2666)

---
 reposerver/repository/repository.go           |  2 +-
 reposerver/repository/repository_test.go      | 13 ++++++++
 .../__snapshots__/revision.test.tsx.snap      | 33 +++++++++++++++++++
 .../app/shared/components/revision.test.tsx   | 33 +++++++++++++++++++
 ui/src/app/shared/components/revision.tsx     |  7 +++-
 5 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100644 ui/src/app/shared/components/__snapshots__/revision.test.tsx.snap
 create mode 100644 ui/src/app/shared/components/revision.test.tsx

diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go
index 453bf567d2..882da3c76b 100644
--- a/reposerver/repository/repository.go
+++ b/reposerver/repository/repository.go
@@ -777,7 +777,7 @@ func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revisi
 	helmClient := s.newHelmClient(repo.Repo, repo.GetHelmCreds())
 	constraints, err := semver.NewConstraint(revision)
 	if err != nil {
-		return nil, "", err
+		return nil, "", fmt.Errorf("invalid revision '%s': %v", revision, err)
 	}
 	index, err := helmClient.GetIndex()
 	if err != nil {
diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go
index d42a18c4c0..a70245a2c8 100644
--- a/reposerver/repository/repository_test.go
+++ b/reposerver/repository/repository_test.go
@@ -446,3 +446,16 @@ func Test_newEnv(t *testing.T) {
 		},
 	}, "my-revision"))
 }
+
+func TestService_newHelmClientResolveRevision(t *testing.T) {
+	service := newService(".")
+
+	t.Run("EmptyRevision", func(t *testing.T) {
+		_, _, err := service.newHelmClientResolveRevision(&argoappv1.Repository{}, "", "")
+		assert.EqualError(t, err, "invalid revision '': improper constraint: ")
+	})
+	t.Run("InvalidRevision", func(t *testing.T) {
+		_, _, err := service.newHelmClientResolveRevision(&argoappv1.Repository{}, "???", "")
+		assert.EqualError(t, err, "invalid revision '???': improper constraint: ???")
+	})
+}
diff --git a/ui/src/app/shared/components/__snapshots__/revision.test.tsx.snap b/ui/src/app/shared/components/__snapshots__/revision.test.tsx.snap
new file mode 100644
index 0000000000..63ce38b34e
--- /dev/null
+++ b/ui/src/app/shared/components/__snapshots__/revision.test.tsx.snap
@@ -0,0 +1,33 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Revision.Branch.Children 1`] = `
+<a
+  href="http://github.com/my-org/my-repo/commit/long-branch-name"
+>
+  foo
+</a>
+`;
+
+exports[`Revision.Branch.NoChildren 1`] = `
+<a
+  href="http://github.com/my-org/my-repo/commit/long-branch-name"
+>
+  long-branch-name
+</a>
+`;
+
+exports[`Revision.SHA1.Children 1`] = `
+<a
+  href="http://github.com/my-org/my-repo/commit/24eb0b24099b2e9afff72558724e88125eaa0176"
+>
+  foo
+</a>
+`;
+
+exports[`Revision.SHA1.NoChildren 1`] = `
+<a
+  href="http://github.com/my-org/my-repo/commit/24eb0b24099b2e9afff72558724e88125eaa0176"
+>
+  24eb0b2
+</a>
+`;
diff --git a/ui/src/app/shared/components/revision.test.tsx b/ui/src/app/shared/components/revision.test.tsx
new file mode 100644
index 0000000000..b2ea79c977
--- /dev/null
+++ b/ui/src/app/shared/components/revision.test.tsx
@@ -0,0 +1,33 @@
+import * as renderer from 'react-test-renderer';
+import * as React from 'react';
+import {isSHA, Revision} from "./revision";
+
+test('Revision.SHA1.Children', () => {
+    const tree = renderer.create(<Revision repoUrl='http://github.com/my-org/my-repo' revision='24eb0b24099b2e9afff72558724e88125eaa0176'>foo</Revision>).toJSON();
+
+    expect(tree).toMatchSnapshot()
+});
+
+test('Revision.SHA1.NoChildren', () => {
+    const tree = renderer.create(<Revision repoUrl='http://github.com/my-org/my-repo' revision='24eb0b24099b2e9afff72558724e88125eaa0176'/>).toJSON();
+
+    expect(tree).toMatchSnapshot()
+});
+
+test('Revision.Branch.Children', () => {
+    const tree = renderer.create(<Revision repoUrl='http://github.com/my-org/my-repo' revision='long-branch-name'>foo</Revision>).toJSON();
+
+    expect(tree).toMatchSnapshot()
+});
+
+
+test('Revision.Branch.NoChildren', () => {
+    const tree = renderer.create(<Revision repoUrl='http://github.com/my-org/my-repo' revision='long-branch-name'/>).toJSON();
+
+    expect(tree).toMatchSnapshot()
+});
+
+test('isSHA1', () => {
+    expect(isSHA('24eb0b24099b2e9afff72558724e88125eaa0176')).toBe(true);
+    expect(isSHA('master')).toBe(false);
+});
\ No newline at end of file
diff --git a/ui/src/app/shared/components/revision.tsx b/ui/src/app/shared/components/revision.tsx
index 8b80ceac11..06d488ee2e 100644
--- a/ui/src/app/shared/components/revision.tsx
+++ b/ui/src/app/shared/components/revision.tsx
@@ -4,6 +4,11 @@ import {revisionUrl} from './urls';
 export const Revision = ({repoUrl, revision, children}: {repoUrl: string; revision: string; children?: React.ReactNode}) => {
     revision = revision || '';
     const url = revisionUrl(repoUrl, revision);
-    const content = children || revision.substr(0, 7);
+    const content = children || (isSHA(revision) ? revision.substr(0, 7) : revision);
     return url !== null ? <a href={url}>{content}</a> : <span>{content}</span>;
 };
+
+export const isSHA = (revision: string) => {
+    // https://stackoverflow.com/questions/468370/a-regex-to-match-a-sha1
+    return revision.match(/[0-9a-f]{5,40}/) !== null;
+};