Merge pull request #27 from m-lab/sandbox-soltesz-increase-coverage

Improve test coverage and add standard flag support

cmd/github_receiver/main.go

@@ -18,32 +18,35 @@
 package main
 
 import (
+	"context"
+	"flag"
 	"fmt"
-	"io/ioutil"
-	"log"
 	"net/http"
 	"os"
 
+	"github.com/m-lab/go/httpx"
+	"github.com/m-lab/go/rtx"
+
 	"github.com/m-lab/alertmanager-github-receiver/alerts"
 	"github.com/m-lab/alertmanager-github-receiver/issues"
 	"github.com/m-lab/alertmanager-github-receiver/issues/local"
-	_ "github.com/m-lab/go/prometheusx"
+	"github.com/m-lab/go/flagx"
+	"github.com/m-lab/go/prometheusx"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
-	flag "github.com/spf13/pflag"
 )
 
 var (
 	authtoken       = flag.String("authtoken", "", "Oauth2 token for access to github API.")
-	authtokenFile   = flag.String("authtokenFile", "", "Oauth2 token file for access to github API. When provided it takes precedence over authtoken.")
+	authtokenFile   = flagx.FileBytes{}
 	githubOrg       = flag.String("org", "", "The github user or organization name where all repos are found.")
 	githubRepo      = flag.String("repo", "", "The default repository for creating issues when alerts do not include a repo label.")
 	enableAutoClose = flag.Bool("enable-auto-close", false, "Once an alert stops firing, automatically close open issues.")
 	enableInMemory  = flag.Bool("enable-inmemory", false, "Perform all operations in memory, without using github API.")
-	receiverPort    = flag.String("port", "9393", "The port for accepting alertmanager webhook messages.")
+	receiverAddr    = flag.String("webhook.listen-address", ":9393", "Listen on address for new alertmanager webhook messages.")
 	alertLabel      = flag.String("alertlabel", "alert:boom:", "The default label applied to all alerts. Also used to search the repo to discover exisitng alerts.")
-	extraLabels     = flag.StringArray("label", nil, "Extra labels to add to issues at creation time.")
+	extraLabels     = flagx.StringArray{}
 )
 
 // Metrics.
@@ -57,50 +60,65 @@ var (
 	)
 )
 
+var (
+	ctx, cancelCtx = context.WithCancel(context.Background())
+	osExit         = os.Exit
+)
+
 const (
 	usage = `
-Usage of %s:
+NAME
+  github_receiver receives Alertmanager webhook notifications and creates
+  corresponding issues on Github.
 
-Github receiver requires a github --authtoken or --authtokenFile, target github --owner and
---repo names.
+DESCRIPTION
+  The github_receiver authenticates all actions using the given --authtoken
+  or the value read from --authtokenFile. As well, the given --org and --repo
+  names are used as the default destination for new issues.
 
+EXAMPLE
+  github_receiver -org <name> -repo <repo> -authtoken <token>
 `
 )
 
 func init() {
+	flag.Var(&extraLabels, "label", "Extra labels to add to issues at creation time.")
+	flag.Var(&authtokenFile, "authtokenFile", "Oauth2 token file for access to github API. When provided it takes precedence over authtoken.")
 	flag.Usage = func() {
-		fmt.Fprintf(os.Stderr, usage, os.Args[0])
+		fmt.Fprintf(flag.CommandLine.Output(), usage)
 		flag.PrintDefaults()
 	}
 }
 
-func serveReceiverHandler(client alerts.ReceiverClient) {
-	receiverHandler := &alerts.ReceiverHandler{
+func mustServeWebhookReceiver(client alerts.ReceiverClient) *http.Server {
+	receiver := &alerts.ReceiverHandler{
 		Client:      client,
 		DefaultRepo: *githubRepo,
 		AutoClose:   *enableAutoClose,
-		ExtraLabels: *extraLabels,
+		ExtraLabels: extraLabels,
+	}
+	mux := http.NewServeMux()
+	mux.Handle("/", &issues.ListHandler{ListClient: client})
+	mux.Handle("/v1/receiver", promhttp.InstrumentHandlerDuration(receiverDuration, receiver))
+	srv := &http.Server{
+		Addr:    *receiverAddr,
+		Handler: mux,
 	}
-	http.Handle("/", &issues.ListHandler{ListClient: client})
-	http.Handle("/v1/receiver", promhttp.InstrumentHandlerDuration(receiverDuration, receiverHandler))
-	http.Handle("/metrics", promhttp.Handler())
-	log.Fatal(http.ListenAndServe(":"+*receiverPort, nil))
+	rtx.Must(httpx.ListenAndServeAsync(srv), "Failed to start webhook receiver server")
+	return srv
 }
 
 func main() {
 	flag.Parse()
-	if (*authtoken == "" && *authtokenFile == "") || *githubOrg == "" || *githubRepo == "" {
+	if (*authtoken == "" && len(authtokenFile) == 0) || *githubOrg == "" || *githubRepo == "" {
 		flag.Usage()
-		os.Exit(1)
+		osExit(1)
+		return
 	}
 
 	var token string
-	if *authtokenFile != "" {
-		d, err := ioutil.ReadFile(*authtokenFile)
-		if err != nil {
-			log.Fatal(err)
-		}
-		token = string(d)
+	if len(authtokenFile) != 0 {
+		token = string(authtokenFile)
 	} else {
 		token = *authtoken
 	}
@@ -111,5 +129,10 @@ func main() {
 	} else {
 		client = issues.NewClient(*githubOrg, token, *alertLabel)
 	}
-	serveReceiverHandler(client)
+	promSrv := prometheusx.MustServeMetrics()
+	defer promSrv.Close()
+
+	srv := mustServeWebhookReceiver(client)
+	defer srv.Close()
+	<-ctx.Done()
 }

cmd/github_receiver/main_test.go

@@ -1,12 +1,64 @@
 package main
 
 import (
+	"flag"
+	"io/ioutil"
+	"sync"
 	"testing"
 
+	"github.com/m-lab/go/prometheusx"
 	"github.com/m-lab/go/prometheusx/promtest"
 )
 
 func TestMetrics(t *testing.T) {
 	receiverDuration.WithLabelValues("x")
 	promtest.LintMetrics(t)
 }
+
+func Test_main(t *testing.T) {
+	tests := []struct {
+		name      string
+		authfile  string
+		authtoken string
+		repo      string
+		inmemory  bool
+	}{
+		{
+			name:      "okay-default",
+			repo:      "fake-repo",
+			authtoken: "token",
+			inmemory:  false,
+		},
+		{
+			name:     "okay-inmemory",
+			authfile: "fake-token",
+			repo:     "fake-repo",
+			inmemory: true,
+		},
+		{
+			name: "missing-flags-usage",
+		},
+	}
+	flag.CommandLine.SetOutput(ioutil.Discard)
+	osExit = func(int) {}
+	for _, tt := range tests {
+		*authtoken = tt.authtoken
+		authtokenFile = []byte(tt.authfile)
+		*githubOrg = "fake-org"
+		*githubRepo = tt.repo
+		*enableInMemory = tt.inmemory
+		// Guarantee no port conflicts between tests of main.
+		*prometheusx.ListenAddress = ":0"
+		*receiverAddr = ":0"
+		t.Run(tt.name, func(t *testing.T) {
+			wg := sync.WaitGroup{}
+			wg.Add(1)
+			go func() {
+				main()
+				wg.Done()
+			}()
+			cancelCtx()
+			wg.Wait()
+		})
+	}
+}

issues/handler.go

@@ -16,6 +16,7 @@
 package issues
 
 import (
+	"bytes"
 	"fmt"
 	"html/template"
 	"net/http"
@@ -33,7 +34,7 @@ const (
 {{end}}
 </table>
 <br/>
-Receiver metrics: <a href="/metrics">/metrics</a>
+Receiver metrics: <a href="/metrics" onclick="javascript:event.target.port=9990">/metrics</a>
 </body></html>`
 )
 
@@ -54,7 +55,8 @@ type ListHandler struct {
 // ServeHTTP lists open issues from github for view in a browser.
 func (lh *ListHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	if req.URL.Path != "/" || req.Method != http.MethodGet {
-		http.NotFound(rw, req)
+		rw.WriteHeader(http.StatusMethodNotAllowed)
+		fmt.Fprintf(rw, "Wrong method\n")
 		return
 	}
 	issues, err := lh.ListOpenIssues()
@@ -63,10 +65,12 @@ func (lh *ListHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 		fmt.Fprintf(rw, "%s\n", err)
 		return
 	}
-	err = listTemplate.Execute(rw, &issues)
+	var buf bytes.Buffer
+	err = listTemplate.Execute(&buf, &issues)
 	if err != nil {
 		rw.WriteHeader(http.StatusInternalServerError)
 		fmt.Fprintf(rw, "%s\n", err)
 		return
 	}
+	rw.Write(buf.Bytes())
 }

issues/handler_test.go

@@ -12,66 +12,98 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //////////////////////////////////////////////////////////////////////////////
-package issues_test
+package issues
 
 import (
-	"io/ioutil"
+	"fmt"
+	"html/template"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
 	"github.com/google/go-github/github"
-
-	"github.com/m-lab/alertmanager-github-receiver/issues"
 )
 
 type fakeClient struct {
 	issues []*github.Issue
+	err    error
 }
 
 func (f *fakeClient) ListOpenIssues() ([]*github.Issue, error) {
-	return f.issues, nil
+	return f.issues, f.err
 }
-
-func TestListHandler(t *testing.T) {
-	expected := `
-<html><body>
-<h2>Open Issues</h2>
-<table>
-
-  <tr><td><a href=http://foo.bar>issue1 title</a></td></tr>
-
-</table>
-<br/>
-Receiver metrics: <a href="/metrics">/metrics</a>
-</body></html>`
-	f := &fakeClient{
-		[]*github.Issue{
-			&github.Issue{
-				HTMLURL: github.String("http://foo.bar"),
-				Title:   github.String("issue1 title"),
+func TestListHandler_ServeHTTP(t *testing.T) {
+	tests := []struct {
+		name           string
+		listClient     ListClient
+		method         string
+		expectedStatus int
+		wantErr        bool
+		template       *template.Template
+	}{
+		{
+			name:           "okay",
+			method:         http.MethodGet,
+			expectedStatus: http.StatusOK,
+			listClient: &fakeClient{
+				issues: []*github.Issue{
+					&github.Issue{
+						HTMLURL: github.String("http://foo.bar"),
+						Title:   github.String("issue1 title"),
+					},
+				},
+				err: nil,
 			},
+			template: listTemplate,
+		},
+		{
+			name:           "issues-error",
+			method:         http.MethodGet,
+			expectedStatus: http.StatusInternalServerError,
+			listClient: &fakeClient{
+				issues: nil,
+				err:    fmt.Errorf("Fake error"),
+			},
+			template: listTemplate,
+		},
+		{
+			name:           "bad-method",
+			method:         http.MethodPost,
+			expectedStatus: http.StatusMethodNotAllowed,
+		},
+		{
+			name:           "bad-template",
+			method:         http.MethodGet,
+			expectedStatus: http.StatusInternalServerError,
+			listClient: &fakeClient{
+				issues: []*github.Issue{
+					&github.Issue{
+						HTMLURL: github.String("http://foo.bar"),
+						Title:   github.String("issue1 title"),
+					},
+				},
+				err: nil,
+			},
+			template: template.Must(template.New("list").Parse(`{{range .}}{{.KeyDoesNotExist}}{{end}}`)),
 		},
 	}
-	// Create a response recorder.
-	rw := httptest.NewRecorder()
-	// Create a synthetic request object for ServeHTTP.
-	req, err := http.NewRequest("GET", "/", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Run the list handler.
-	handler := issues.ListHandler{ListClient: f}
-	handler.ServeHTTP(rw, req)
-	resp := rw.Result()
-
-	// Check the results.
-	body, _ := ioutil.ReadAll(resp.Body)
-	if resp.StatusCode != http.StatusOK {
-		t.Errorf("ListHandler got %d; want %d", resp.StatusCode, http.StatusOK)
-	}
-	if expected != string(body) {
-		t.Errorf("ListHandler got %q; want %q", string(body), expected)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Create a response recorder.
+			rw := httptest.NewRecorder()
+			// Create a synthetic request object for ServeHTTP.
+			req, err := http.NewRequest(tt.method, "/", nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			lh := &ListHandler{
+				ListClient: tt.listClient,
+			}
+			listTemplate = tt.template
+			lh.ServeHTTP(rw, req)
+			if rw.Code != tt.expectedStatus {
+				t.Errorf("ListHandler wrong status; want %d, got %d", tt.expectedStatus, rw.Code)
+			}
+		})
 	}
 }

issues/issues.go

@@ -212,7 +212,7 @@ func getOrgAndRepoFromIssue(issue *github.Issue) (string, string, error) {
 	}
 	fields := strings.Split(u.Path, "/")
 	if len(fields) != 4 {
-		return "", "", fmt.Errorf("Issue has invalid RepositoryURL value")
+		return "", "", fmt.Errorf("Issue has invalid RepositoryURL path values")
 	}
 	return fields[2], fields[3], nil