Repository: fluxcd/notification-controller
Pull request #154 (2021-03-05 15:15:39)

Fix Azure Devops notifier issues
This change contains general fixes to the Azure DevOps notifier to fix smaller usability issues.

* Respect schema set by the provider url
* Modify the Azure DevOps notifier to allow compatibility with running behind a proxy.
* Check existing commit statuses to avoid spamming the same status when reconciling.

Signed-off-by: Philip Laine <philip.laine@xenit.se>


Patch:
diff --git a/internal/notifier/azure_devops.go b/internal/notifier/azure_devops.go
index 70dc051..68e0a69 100644
--- a/internal/notifier/azure_devops.go
+++ b/internal/notifier/azure_devops.go
@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"time"
 
 	"github.com/fluxcd/pkg/recorder"
 	"github.com/microsoft/azure-devops-go-api/azuredevops"
@@ -31,9 +32,9 @@ const genre string = "fluxcd"
 
 // AzureDevOps is a Azure DevOps notifier.
 type AzureDevOps struct {
-	Project    string
-	Repo       string
-	Connection *azuredevops.Connection
+	Project string
+	Repo    string
+	Client  git.Client
 }
 
 // NewAzureDevOps creates and returns a new AzureDevOps notifier.
@@ -55,11 +56,16 @@ func NewAzureDevOps(addr string, token string) (*AzureDevOps, error) {
 	proj := comp[1]
 	repo := comp[3]
 
-	c := azuredevops.NewPatConnection(fmt.Sprintf("%v/%v", host, org), token)
+	orgURL := fmt.Sprintf("%v/%v", host, org)
+	connection := azuredevops.NewPatConnection(orgURL, token)
+	client := connection.GetClientByUrl(orgURL)
+	gitClient := &git.ClientImpl{
+		Client: *client,
+	}
 	return &AzureDevOps{
-		Project:    proj,
-		Repo:       repo,
-		Connection: c,
+		Project: proj,
+		Repo:    repo,
+		Client:  gitClient,
 	}, nil
 }
 
@@ -83,15 +89,13 @@ func (a AzureDevOps) Post(event recorder.Event) error {
 		return err
 	}
 
-	ctx := context.Background()
-	client, err := git.NewClient(ctx, a.Connection)
-	if err != nil {
-		return err
-	}
+	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	defer cancel()
 
+	// Check if the exact status is already set
 	g := genre
 	name, desc := formatNameAndDescription(event)
-	args := git.CreateCommitStatusArgs{
+	createArgs := git.CreateCommitStatusArgs{
 		Project:      &a.Project,
 		RepositoryId: &a.Repo,
 		CommitId:     &rev,
@@ -104,11 +108,24 @@ func (a AzureDevOps) Post(event recorder.Event) error {
 			},
 		},
 	}
-	_, err = client.CreateCommitStatus(ctx, args)
+	getArgs := git.GetStatusesArgs{
+		Project:      &a.Project,
+		RepositoryId: &a.Repo,
+		CommitId:     &rev,
+	}
+	statuses, err := a.Client.GetStatuses(ctx, getArgs)
 	if err != nil {
-		return err
+		return fmt.Errorf("could not list commit statuses: %v", err)
+	}
+	if duplicateAzureDevOpsStatus(statuses, createArgs.GitCommitStatusToCreate) {
+		return nil
 	}
 
+	// Create a new status
+	_, err = a.Client.CreateCommitStatus(context.Background(), createArgs)
+	if err != nil {
+		return fmt.Errorf("could not create commit status: %v", err)
+	}
 	return nil
 }
 
@@ -122,3 +139,19 @@ func toAzureDevOpsState(severity string) (git.GitStatusState, error) {
 		return "", errors.New("can't convert to azure devops state")
 	}
 }
+
+// duplicateStatus return true if the latest status
+// with a matching context has the same state and description
+func duplicateAzureDevOpsStatus(statuses *[]git.GitStatus, status *git.GitStatus) bool {
+	for _, s := range *statuses {
+		if *s.Context.Name == *status.Context.Name && *s.Context.Genre == *status.Context.Genre {
+			if *s.State == *status.State && *s.Description == *status.Description {
+				return true
+			}
+
+			return false
+		}
+	}
+
+	return false
+}
diff --git a/internal/notifier/github.go b/internal/notifier/github.go
index 5806b6b..3c961fa 100644
--- a/internal/notifier/github.go
+++ b/internal/notifier/github.go
@@ -96,7 +96,7 @@ func (g *GitHub) Post(event recorder.Event) error {
 	if err != nil {
 		return fmt.Errorf("could not list commit statuses: %v", err)
 	}
-	if duplicateStatus(statuses, status) {
+	if duplicateGithubStatus(statuses, status) {
 		return nil
 	}
 
@@ -121,7 +121,7 @@ func toGitHubState(severity string) (string, error) {
 
 // duplicateStatus return true if the latest status
 // with a matching context has the same state and description
-func duplicateStatus(statuses []*github.RepoStatus, status *github.RepoStatus) bool {
+func duplicateGithubStatus(statuses []*github.RepoStatus, status *github.RepoStatus) bool {
 	for _, s := range statuses {
 		if *s.Context == *status.Context {
 			if *s.State == *status.State && *s.Description == *status.Description {
diff --git a/internal/notifier/util.go b/internal/notifier/util.go
index c6c60dc..d7d7d69 100644
--- a/internal/notifier/util.go
+++ b/internal/notifier/util.go
@@ -32,9 +32,14 @@ func parseGitAddress(s string) (string, string, error) {
 		return "", "", nil
 	}
 
+	scheme := u.Scheme
+	if u.Scheme == "ssh" {
+		scheme = "https"
+	}
+
 	id := strings.TrimLeft(u.Path, "/")
 	id = strings.TrimSuffix(id, ".git")
-	host := fmt.Sprintf("https://%s", u.Host)
+	host := fmt.Sprintf("%s://%s", scheme, u.Host)
 	return host, id, nil
 }
 


Test patch:
diff --git a/internal/notifier/azure_devops_test.go b/internal/notifier/azure_devops_test.go
index 53d0496..5a72890 100644
--- a/internal/notifier/azure_devops_test.go
+++ b/internal/notifier/azure_devops_test.go
@@ -19,6 +19,7 @@ package notifier
 import (
 	"testing"
 
+	"github.com/microsoft/azure-devops-go-api/azuredevops/git"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -38,3 +39,35 @@ func TestNewAzureDevOpsMissingToken(t *testing.T) {
 	_, err := NewAzureDevOps("https://dev.azure.com/foo/bar/baz", "")
 	assert.NotNil(t, err)
 }
+
+func TestDuplicateAzureDevOpsStatus(t *testing.T) {
+	assert := assert.New(t)
+
+	var tests = []struct {
+		ss  *[]git.GitStatus
+		s   *git.GitStatus
+		dup bool
+	}{
+		{&[]git.GitStatus{*azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar")}, azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar"), true},
+		{&[]git.GitStatus{*azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar")}, azStatus(git.GitStatusStateValues.Failed, "foo", "bar"), false},
+		{&[]git.GitStatus{*azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar")}, azStatus(git.GitStatusStateValues.Succeeded, "baz", "bar"), false},
+		{&[]git.GitStatus{*azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar")}, azStatus(git.GitStatusStateValues.Succeeded, "foo", "baz"), false},
+		{&[]git.GitStatus{*azStatus(git.GitStatusStateValues.Succeeded, "baz", "bar"), *azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar")}, azStatus(git.GitStatusStateValues.Succeeded, "foo", "bar"), true},
+	}
+
+	for _, test := range tests {
+		assert.Equal(test.dup, duplicateAzureDevOpsStatus(test.ss, test.s))
+	}
+}
+
+func azStatus(state git.GitStatusState, context string, description string) *git.GitStatus {
+	genre := "fluxcd"
+	return &git.GitStatus{
+		Context: &git.GitStatusContext{
+			Name:  &context,
+			Genre: &genre,
+		},
+		Description: &description,
+		State:       &state,
+	}
+}
diff --git a/internal/notifier/github_test.go b/internal/notifier/github_test.go
index eec1ce0..3a11a46 100644
--- a/internal/notifier/github_test.go
+++ b/internal/notifier/github_test.go
@@ -40,7 +40,7 @@ func TestNewGitHubEmptyToken(t *testing.T) {
 	assert.NotNil(t, err)
 }
 
-func TestDuplicateState(t *testing.T) {
+func TestDuplicateGithubStatus(t *testing.T) {
 	assert := assert.New(t)
 
 	var tests = []struct {
@@ -56,7 +56,7 @@ func TestDuplicateState(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		assert.Equal(test.dup, duplicateStatus(test.ss, test.s))
+		assert.Equal(test.dup, duplicateGithubStatus(test.ss, test.s))
 	}
 }