code review for rookie

review.patch

Index: internal/mywebapp/users/users_test.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- internal/mywebapp/users/users_test.go	(date 1575024740000)
+++ internal/mywebapp/users/users_test.go	(date 1575030918663)
@@ -1,6 +1,7 @@
 package users_test
 
 import (
+	// pls setup goimports on save in your editor
 	"bytes"
 	"encoding/json"
 	"fmt"
@@ -22,6 +23,25 @@
 
 var _ = Describe("my web app", func() {
 	var server *httptest.Server
+
+	// This may look like a boomer point of view but, since it's not just go but also devops related, I will go with it:
+	//
+	// Using a cool mux or flashy testing frameworks might seem nice (yea, readability), but using them
+	// * might add up to the onboarding length of each new employee (who didn't use them before) or to every situation where the solution on stackoverflow is written in idiomatic go and you have to translate it to your shiny tools
+	// * certainly adds up to the CI/CD pipeline length (since these will have to be fetched at every check/deploy) or complexity (think caching them)
+	// * comes with a plus of readability in a language that is already among the most readable ones
+	// * might anger rob pike
+	//
+	// I would propose a more idiomatic approach, and here are some examples:
+	// * you could use go's TestMain func, which helps with wrapping code around each actual test (BeforeEach and AfterEach below).
+	// * table driven subtests!
+	// * for the mux, the situation is even more self explanatory, since creating a few routes using net/http's default mux is a breeze.
+	//
+	// On the other hand,
+	// * readability still weighs a lot, since that's how we came to use BDD for testing here
+	// * the separation of concerns is nice (think http code separated in controllers)
+	//
+	// so let's discuss this over coffee
 	BeforeEach(func() {
 		server = httptest.NewServer(mywebapp.NewController())
 	})
Index: cmd/mywebapp/main.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- cmd/mywebapp/main.go	(date 1575024740000)
+++ cmd/mywebapp/main.go	(date 1575031480474)
@@ -8,4 +8,9 @@
 
 func main() {
 	log.Fatal(http.ListenAndServe(":8080", mywebapp.NewController()))
+	// Output should be captured in a io.Writer that can be os.stdout or not
+	// and then you can have proper cli integration testing
+	//
+	// I would much rather use cobra (self documenting app, autocomplete, ...)
+	// and viper for configs (e.g. the credentials)
 }
Index: internal/mywebapp/controllers.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- internal/mywebapp/controllers.go	(date 1575024740000)
+++ internal/mywebapp/controllers.go	(date 1575031637351)
@@ -1,11 +1,12 @@
 package mywebapp
 
+// please goimport your comments
+// import "mywebapp/internal/mywebapp/users" so this actually works
 import (
 	"context"
 	"encoding/json"
 	"fmt"
 	"log"
-	"mywebapp/internal/mywebapp/users"
 	"net/http"
 	"regexp"
 
@@ -118,6 +119,7 @@
 			}
 			return
 		}
+		// The delete test expects this to return http.StatusNoContent
 		w.WriteHeader(http.StatusOK)
 	}
 }
Index: internal/mywebapp/users/users.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- internal/mywebapp/users/users.go	(date 1575024740000)
+++ internal/mywebapp/users/users.go	(date 1575030719590)
@@ -15,6 +15,13 @@
 	Email string `json:"email"`
 }
 
+// main thing here is "never store credentials in code"
+//
+// but please also check out https://12factor.net/config
+// and all the website, since you're there :P
+//
+// simplest thing here would be to store this string in an env var,
+// but I would ideally use a combination of env vars and cli flags for this and json.Marshal the Config struct on the fly
 const creds = `{
   "type": "service_account",
   "project_id": "simprints-cloud-hiring",
@@ -39,10 +46,17 @@
 }
 
 func NewRepository(ctx context.Context) (Repository, error) {
+	// please don't shadow stuff, since, even if not the case here, you might need it later
+	// and check the comment above the creds const
 	creds, err := google.CredentialsFromJSON(ctx, []byte(creds), "https://www.googleapis.com/auth/cloud-platform")
 	if err != nil {
 		return nil, err
 	}
+	// 1. you should retrieve something like this env var from outside the constructor and pass it as a parameter
+	// 2. you will have to skip this GCLOUD_PROJECT requirement for tests, since we MUST mock the client like this:
+	// https://godoc.org/cloud.google.com/go/firestore#hdr-Google_Cloud_Firestore_Emulator
+	// This will speed up the tests a lot and also rid us of external dependencies, effectively transforming them from
+	// integration to unit tests.
 	project := os.Getenv("GCLOUD_PROJECT")
 	if project == "" {
 		return nil, fmt.Errorf("missing required environment variable GCLOUD_PROJECT")