Review feedback

Author: jsha

sa/vtschema/boulder_sa/vschema.json

@@ -11,7 +11,7 @@
     "certificateStatus": { "column_vindexes": [ { "column": "serial", "name": "xxhash" } ] },
     "certificates": { "column_vindexes": [ { "column": "serial", "name": "xxhash" } ] },
     "crlShards": { "column_vindexes": [ { "column": "issuerID", "name": "xxhash" } ] },
-    "crls": { "column_vindexes": [ { "column": "serials", "name": "xxhash" } ] },
+    "crls": { "column_vindexes": [ { "column": "serial", "name": "xxhash" } ] },
     "fqdnSets": { "column_vindexes": [ { "column": "id", "name": "xxhash" } ] },
     "incidents": { "column_vindexes": [ { "column": "id", "name": "xxhash" } ] },
     "issuedNames": { "column_vindexes": [ { "column": "id", "name": "xxhash" } ] },

sa/vtschema/boulder_sa_next/vschema.json

@@ -11,7 +11,7 @@
     "certificateStatus": { "column_vindexes": [ { "column": "serial", "name": "xxhash" } ] },
     "certificates": { "column_vindexes": [ { "column": "serial", "name": "xxhash" } ] },
     "crlShards": { "column_vindexes": [ { "column": "issuerID", "name": "xxhash" } ] },
-    "crls": { "column_vindexes": [ { "column": "serials", "name": "xxhash" } ] },
+    "crls": { "column_vindexes": [ { "column": "serial", "name": "xxhash" } ] },
     "fqdnSets": { "column_vindexes": [ { "column": "id", "name": "xxhash" } ] },
     "incidents": { "column_vindexes": [ { "column": "id", "name": "xxhash" } ] },
     "issuedNames": { "column_vindexes": [ { "column": "id", "name": "xxhash" } ] },

test/db.go

@@ -23,18 +23,15 @@ type CleanUpDB interface {
 }
 
 // ResetBoulderTestDatabase returns a cleanup function which deletes all rows in
-// all tables of the 'boulder_sa' database. Omits the 'gorp_migrations'
-// table as this is used by sql-migrate (https://github.com/rubenv/sql-migrate)
-// to track migrations. If it encounters an error it fails the tests.
+// all tables of the 'boulder_sa' database.
+// If it encounters an error it fails the tests.
 func ResetBoulderTestDatabase(t testing.TB) func() {
 	return resetTestDatabase(t, context.Background(), vars.DBConnSAFullPerms)
 }
 
 // ResetIncidentsTestDatabase returns a cleanup function which deletes all rows
-// in all tables of the 'incidents_sa' database. Omits the
-// 'gorp_migrations' table as this is used by sql-migrate
-// (https://github.com/rubenv/sql-migrate) to track migrations. If it encounters
-// an error it fails the tests.
+// in all tables of the 'incidents_sa' database.
+// If it encounters an error it fails the tests.
 func ResetIncidentsTestDatabase(t testing.TB) func() {
 	return resetTestDatabase(t, context.Background(), vars.DBConnIncidentsFullPerms)
 }
@@ -86,10 +83,9 @@ func deleteEverythingInAllTables(ctx context.Context, db CleanUpDB) error {
 }
 
 // allTableNamesInDB returns the names of the tables available to the passed
-// CleanUpDB. Omits the 'gorp_migrations' table as this is used by sql-migrate
-// (https://github.com/rubenv/sql-migrate) to track migrations.
+// CleanUpDB.
 func allTableNamesInDB(ctx context.Context, db CleanUpDB) ([]string, error) {
-	r, err := db.QueryContext(ctx, "select table_name from information_schema.tables t where t.table_schema = DATABASE() and t.table_name != 'gorp_migrations';")
+	r, err := db.QueryContext(ctx, "select table_name from information_schema.tables t where t.table_schema = DATABASE();")
 	if err != nil {
 		return nil, err
 	}

test/integration/revocation_test.go

@@ -383,14 +383,16 @@ func TestRevocation(t *testing.T) {
 	type revocationCheck func(t *testing.T, allCRLs map[string][]*x509.RevocationList)
 	var revocationChecks []revocationCheck
 	var rcMu sync.Mutex
-	var wg sync.WaitGroup
 
 	for _, kind := range []certKind{precert, finalcert} {
 		for _, reason := range []revocation.Reason{revocation.Unspecified, revocation.KeyCompromise, revocation.Superseded} {
+			// We only schedule four work items at a time because vttestserver only supports 4 concurrent transactions.
+			// If there are more than that, they will hang, timeout, and error.
+			// Specifically, vttestserver runs vtcomboserver with --queryserver-config-transaction-cap 4 and has no
+			// way (as of v23.0) to override it.
+			var wg sync.WaitGroup
 			for _, method := range []authMethod{byAccount, byAuth, byKey, byAdmin} {
-				wg.Add(1)
-				go func() {
-					defer wg.Done()
+				wg.Go(func() {
 					cert := issueAndRevoke(testCase{
 						method: method,
 						reason: reason,
@@ -424,7 +426,7 @@ func TestRevocation(t *testing.T) {
 					rcMu.Lock()
 					revocationChecks = append(revocationChecks, check)
 					rcMu.Unlock()
-				}()
+				})
 			}
 			wg.Wait()
 		}

test/vars/vars.go

@@ -10,11 +10,11 @@ func dsn(user, database string) string {
 	if addr == "" {
 		addr = "unset DB_ADDR"
 	}
-	config_variant := ""
+	configVariant := ""
 	if os.Getenv("BOULDER_CONFIG_DIR") == "test/config-next" && database != "information_schema" {
-		config_variant = "_next"
+		configVariant = "_next"
 	}
-	return fmt.Sprintf("%s@tcp(%s)/%s%s", user, addr, database, config_variant)
+	return fmt.Sprintf("%s@tcp(%s)/%s%s", user, addr, database, configVariant)
 }
 
 var (

test/vtcomboserver/Dockerfile

@@ -28,7 +28,6 @@ RUN go build -trimpath -o /out/vtcombo ./go/cmd/vtcombo
 RUN go build -trimpath -o /out/vtexplain ./go/cmd/vtexplain
 RUN go build -trimpath -o /out/mysqlctl ./go/cmd/mysqlctl
 
-
 FROM ${MYSQL_BASE_IMAGE} AS runtime
 
 # Vitess expects to run as a non-root user