Fix docker local tests to be compatible with newer versions of pg image #3047

tylercloke · 2020-03-06T00:45:24Z

🔩 Description: What code changed, and why?

Recently, the docker image's default security config was changed because it defaulted to super insecure settings:

We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness.

👟 How to Build and Test the Change

Get on the latest docker image:

docker image ls
docker image rm -f <imageid>

In teams, authz, and authn services:

make setup_docker_pg
make test_with_db
make kill_docker_pg

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

bcmdarroch · 2020-03-06T00:52:05Z

components/authn-service/tokens/integration/token_integration_test.go

@@ -61,14 +85,9 @@ func TestChefClientAuthn(t *testing.T) {
 		t.Fatalf("parse test upstream URL %+v: %v", upstream, err)
 	}

-	pgURLGiven := false
-	pgCfg := pg.Config{
-		PGURL:          constants.TestPgURL,


is this what was passing the postgres user? 🤔

blakestier · 2020-03-06T22:48:40Z

components/authn-service/tokens/integration/token_integration_test.go

 			t.Fatalf("opening connector: %s", err)
 		} else {
 			t.Logf("opening database: %s", err)
-			t.Logf(testconstants.SkipPGMessageFmt, pgCfg.PGURL)
+			t.Logf("failed to open test database with PG_URL: %q", pgCfg.PGURL)


These subsequent logs seem confusing-- if we know we're going to log a failure, why log the attempt?

I think we are just logging the error and the URL it failed with?

Could be more clear though. Open to whatever.

We hit this line if the PG_URL wasn't provided, right? Can we log that and indicate that we are skipping pg tests (something more akin to the old message)?

Basically I would be led astray by the word "failed"

blakestier · 2020-03-06T22:56:01Z

components/authn-service/Makefile

@@ -50,11 +50,11 @@ test:

 test_with_db:
 	@docker ps | grep authn-postgres || (echo "Docker postgres not up. Run 'make setup_docker_pg' and try this command again."; exit 1)
-	@PG_URL="postgresql://[email protected]:5432/authn_test?sslmode=disable&timezone=UTC" go test -cover -count=1 -parallel=1 -p 1 $(packages)
+	@PGTZ=UTC PG_URL="postgresql://[email protected]:5432/authn_test?sslmode=disable&password=docker" go test -cover -count=1 -parallel=1 -p 1 $(packages)


any idea what the timezone parameter was doing for us?

Oh I see you've moved it above 🤔

blakestier

Would like a message explaining the skipping/jogging my old brain about how to change the skipping

bcmdarroch · 2020-03-09T17:38:09Z

components/authn-service/tokens/pg/testconstants/testconstants.go

-//
-// Yes those lines are long, but if we linebreak, the output ends up starting
-// with TAB characters, which makes it harder to copy-paste the commands.
-const SkipPGMessageFmt = `


Like @blakestier was saying, this is a much more actionable error message. Do we need to remove it?

don't think that's what we want people to do? this made sense when we weren't automating with the makefile.

bcmdarroch

👍 👍 thanks for digging into this!

msorens

Works for me--after rebasing to latest master. (The recent golang bump to 1.14 caused a problem, but rebasing fixed it for me.)

$ make test_with_db
. . .
go: inconsistent vendoring in /Users/msorens/code/go/src/github.com/chef/automate:
	golang.org/x/[email protected]: is explicitly required in go.mod, but vendor/modules.txt indicates golang.org/x/[email protected]

Recently, the docker image's default security config was changed because it defaulted to super insecure settings: docker-library/postgres#580 We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness. Signed-off-by: Tyler Cloke <[email protected]>

tylercloke force-pushed the tc/fix-docker-local-tests branch from 2000450 to 36bf561 Compare March 6, 2020 00:45

bcmdarroch reviewed Mar 6, 2020

View reviewed changes

susanev added the auth-team anything that needs to be on the auth team board label Mar 6, 2020

susanev assigned tylercloke Mar 6, 2020

blakestier reviewed Mar 6, 2020

View reviewed changes

blakestier approved these changes Mar 6, 2020

View reviewed changes

bcmdarroch reviewed Mar 9, 2020

View reviewed changes

bcmdarroch approved these changes Mar 9, 2020

View reviewed changes

tylercloke force-pushed the tc/fix-docker-local-tests branch from 36bf561 to cf091f4 Compare March 9, 2020 20:59

msorens approved these changes Mar 11, 2020

View reviewed changes

tylercloke force-pushed the tc/fix-docker-local-tests branch from cf091f4 to 2027968 Compare March 11, 2020 17:54

tylercloke merged commit 4bc727e into master Mar 12, 2020

tylercloke deleted the tc/fix-docker-local-tests branch March 12, 2020 17:39

tylercloke mentioned this pull request Mar 19, 2020

Fix authz tests locally #3155

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix docker local tests to be compatible with newer versions of pg image #3047

Fix docker local tests to be compatible with newer versions of pg image #3047

Uh oh!

tylercloke commented Mar 6, 2020 •

edited

Loading

Uh oh!

bcmdarroch Mar 6, 2020

Uh oh!

tylercloke Mar 6, 2020

Uh oh!

blakestier Mar 6, 2020

Uh oh!

tylercloke Mar 6, 2020

Uh oh!

tylercloke Mar 6, 2020

Uh oh!

blakestier Mar 6, 2020

Uh oh!

blakestier Mar 6, 2020

Uh oh!

bcmdarroch Mar 9, 2020

Uh oh!

blakestier Mar 6, 2020

Uh oh!

blakestier Mar 6, 2020

Uh oh!

blakestier left a comment

Uh oh!

bcmdarroch Mar 9, 2020

Uh oh!

tylercloke Mar 9, 2020

Uh oh!

bcmdarroch left a comment

Uh oh!

msorens left a comment

Uh oh!

Uh oh!

Fix docker local tests to be compatible with newer versions of pg image #3047

Fix docker local tests to be compatible with newer versions of pg image #3047

Uh oh!

Conversation

tylercloke commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔩 Description: What code changed, and why?

👟 How to Build and Test the Change

✅ Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blakestier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcmdarroch left a comment

Choose a reason for hiding this comment

Uh oh!

msorens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tylercloke commented Mar 6, 2020 •

edited

Loading