Run google-cloud-python system tests against new runtime images. #39

duggelz · 2016-11-03T03:00:56Z

No description provided.

dlorenc

Meta comment: It looks like you're running all of the tests inside a "docker build". It might be a little more straightforward to inject the tests into your docker image using a volume mount during a "docker run", and then run the tests that way.

dlorenc · 2016-11-03T03:03:51Z

system_tests/.gitignore

@@ -0,0 +1,2 @@
+data/
+secrets.tar


Nit: missing newline?

dlorenc · 2016-11-03T03:04:31Z

system_tests/Dockerfile

+# Inject secrets
+# TODO: Encrypt/decrypt secrets
+ADD data/ data/
+ENV GOOGLE_APPLICATION_CREDENTIALS /home/vmagent/app/data/cloud-python-runtime-qa-credentials.json


I think /home/vmagent/app is symlinked to /app. It might be easier to just use that path.

It is indeed.

dlorenc · 2016-11-03T03:05:04Z

system_tests/Dockerfile

@@ -0,0 +1,40 @@
+FROM google/python


Does this assume you've just built an image called "google/python" locally?

It might be nice to make this an invalid image name then. If you forgot to build your image or built it with a different name this would give you false positives, since google/python does exist on Dockerhub, but isn't updated at all IIRC.

Let's file a bug and fix that separately.

Filed as #38

dlorenc · 2016-11-03T03:09:48Z

system_tests/Dockerfile

+
+# Install gcloud so we can do test setup
+RUN apt-get update && apt-get install curl
+RUN curl https://sdk.cloud.google.com | bash


You might want to follow these steps instead: https://cloud.google.com/sdk/downloads#versioned

These will ensure you always get the same version of gcloud, which is especially important when you're using commands in the "preview" group.

Sounds like a good way to end up with a test that constantly runs an outdated SDK. Assuming we run these tests regularly, is there any reason we shouldn't just target the latest?

As long as your goal isn't to actually test the SDK it should be fine. I doubt these commands are going to change on you much in practice though.

One other benefit of fetching these tarballs is that the dl.google.com server which is used by the install script has no SLO and will cause flakes eventually. The versioned tarballs are published to GCS, which will have far fewer flakes.

The App Engine team just switched some internal tests over because of flakiness introduced by dl.google.com.

Actually, @duggelz if you drop the gcloud preview datastore create-indexes system_tests/data/index.yaml you should be able to get by with not installing the SDK at all. Setting GOOGLE_APPLICATION_CREDENTIALS and GCLOUD_PROJECT are sufficient.

Filed #40 and GoogleContainerTools/base-images-docker#50 .

Meanwhile, this is good enough for me.

dlorenc · 2016-11-03T03:10:41Z

system_tests/Dockerfile

+# System test setup as per google-cloud-python/CONTRIBUTING.rst
+RUN gcloud config set project cloud-python-runtime-qa
+RUN gcloud components install app-engine-python
+RUN gcloud auth activate-service-account \


I think if you run this docker build inside cloudbuild you might be able to skip all authentication since it'll be running inside GCE and gcloud can auth itself automatically.

Does cloudcloud give credentials for the current project? What about scopes? Using a service account is predictable auth, so maybe there's some benefit there.

Yeah, the credentials used are a service account in your project. The scopes are cloud-platform I believe, and you can use IAM to grant more permissions to that service account.

Deferring this as #40

dlorenc · 2016-11-03T03:11:34Z

system_tests/Dockerfile

+
+# Run Python 2.7, 3.4 system tests
+# TODO(https://github.com/GoogleCloudPlatform/google-cloud-python/issues/2670)
+RUN GOOGLE_CLOUD_TESTS_API_KEY=$(cat /home/vmagent/app/data/cloud-python-runtime-qa-api-key.txt) \


It might be nice to make this a "CMD" instead of a RUN, that way you can build this image once and then run tests several times.

I was doing 'CACHE= make' to get the same effect from Docker layer caching. Hmm, the name made more sense in my head ("CACHE=" enables caching?!)

theacodes · 2016-11-03T03:32:54Z

Meta comment: It looks like you're running all of the tests inside a "docker build". It might be a little more straightforward to inject the tests into your docker image using a volume mount during a "docker run", and then run the tests that way.

@dlorenc how does that play with cloud build?

dlorenc · 2016-11-03T03:45:05Z

@dlorenc how does that play with cloud build?

It works fine with cloud build. You get full access to a docker daemon.

theacodes · 2016-11-03T03:48:52Z

system_tests/Dockerfile

+
+# System test setup as per google-cloud-python/CONTRIBUTING.rst
+RUN gcloud config set project cloud-python-runtime-qa
+RUN gcloud components install app-engine-python


This shouldn't be needed at all.

Deferred as #40

I'd actually prefer if we went ahead and removed this in this PR. Did you have trouble with just setting GOOGLE_APPLICATION_CREDENTIALS and GCLOUD_PROJECT?

Previous code was always checking out head because of missing --abbrev flag. Once I added --abbrev, I found out the latest release, 0.20, is missing some things we need. We can reenable this when 0.21+ is released.

duggelz · 2016-11-03T20:54:54Z

Please take another look. I also addressed some things that Jon and I talked about over email.

Currently, everything but 'logging' works. See: googleapis/google-cloud-python#2669

theacodes · 2016-11-03T21:14:57Z

.gitignore

@@ -0,0 +1,3 @@
+# Editor


Drop this. Add it to your global gitignore: https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

theacodes · 2016-11-03T21:15:27Z

Makefile

@@ -25,3 +25,7 @@ tests:
 .PHONY: benchmarks
 benchmarks:
 	make -C tests benchmarks
+
+.PHONY: system-tests


Go ahead and name this google-cloud-system-tests, as we may want to add system tests for other packages in the future?

Done. No doubt we'll need to move files and directories around later when we add more system tests.

theacodes · 2016-11-03T21:16:26Z

tests/google-cloud-python/Dockerfile

@@ -8,7 +8,8 @@ RUN git clone https://github.com/GoogleCloudPlatform/google-cloud-python.git
 WORKDIR google-cloud-python

 # Checkout the latest release.
-RUN git checkout $(git describe --tags)
+# TODO: Enable once run_unit_tests.py is released


No TODOs in code, please, file an issue instead?

#41

Do you want to see something like:

# TODO(#41): Enable once run_unit_tests.py is released

Or just not have that comment at all?

Prefer not to have it at all. Put all the needed context in the issue.

Done. Asking me to program without TODOs is like asking me to program without my left hand. But I'm always up for a challenge.

theacodes · 2016-11-03T21:16:59Z

system_tests/Dockerfile

+
+# System test setup as per google-cloud-python/CONTRIBUTING.rst
+RUN gcloud config set project cloud-python-runtime-qa
+RUN gcloud components install app-engine-python


I'd actually prefer if we went ahead and removed this in this PR. Did you have trouble with just setting GOOGLE_APPLICATION_CREDENTIALS and GCLOUD_PROJECT?

This reverts commit e3a7cf8.

See #41

Test setup will be handled elsewhere.

theacodes · 2016-11-03T22:14:10Z

system_tests/Dockerfile

@@ -4,26 +4,23 @@ FROM google/python
 # TODO: Encrypt/decrypt secrets
 ADD data/ data/
 ENV GOOGLE_APPLICATION_CREDENTIALS /app/data/cloud-python-runtime-qa-credentials.json
+ENV GCLOUD_PROJECT cloud-python-runtime-qa


Will you file an issue to figure out how to not hardcode these? (I'm thinking just passing them to docker run, but we can address it later).

Filed as #42

theacodes · 2016-11-03T22:14:41Z

system_tests/Dockerfile

-RUN apt-get update && apt-get install curl
-RUN curl https://sdk.cloud.google.com | bash
-ENV PATH ${PATH}:/root/google-cloud-sdk/bin
+#RUN apt-get update && apt-get install curl


Are you keeping this around in case you need it again in the future? Maybe just make a gist?

Oops, that was supposed to be amended before I pushed. Fixed.

#43 #44

duggelz · 2016-11-03T22:37:51Z

Ok, can you take a final(?) look?

Run google-cloud-python system tests against new runtime images.

9aeedd3

duggelz assigned duggelz, theacodes and dlorenc and unassigned duggelz Nov 3, 2016

dlorenc reviewed Nov 3, 2016

View reviewed changes

theacodes reviewed Nov 3, 2016

View reviewed changes

dhermes mentioned this pull request Nov 3, 2016

Easier system test setup/teardown googleapis/google-cloud-python#2669

Closed

Doug Greiman added 5 commits November 3, 2016 13:01

Add missing newline

3b2abef

Ignore Emacs autosave files

e3a7cf8

Shorten paths (/home/vmagent/app is symlinked to /app)

87db4d3

Add top-level system-tests target

c0933d0

Fix "checkout latest release", then disable it to always checkout head.

5439e04

Previous code was always checking out head because of missing --abbrev flag. Once I added --abbrev, I found out the latest release, 0.20, is missing some things we need. We can reenable this when 0.21+ is released.

Enable system tests that work now.

29c73aa

Currently, everything but 'logging' works. See: googleapis/google-cloud-python#2669

theacodes reviewed Nov 3, 2016

View reviewed changes

Doug Greiman added 3 commits November 3, 2016 14:23

Revert "Ignore Emacs autosave files"

6960bb4

This reverts commit e3a7cf8.

Use google-cloud-python from head, not the released version.

2e6f704

See #41

Don't install or use the Google Cloud SDK for system tests.

d24cb12

Test setup will be handled elsewhere.

duggelz mentioned this pull request Nov 3, 2016

Remove Cloud SDK install from system test #40

Closed

theacodes reviewed Nov 3, 2016

View reviewed changes

Doug Greiman added 3 commits November 3, 2016 15:18

Remove cruft

d108c5a

Rename system-tests target to google-cloud-system-tests

6ad7fb2

Remove TODOs in favor of opening issues

5632d0e

#43 #44

theacodes approved these changes Nov 3, 2016

View reviewed changes

duggelz merged commit 4964ee0 into master Nov 3, 2016

duggelz deleted the dgreiman/system-test branch November 3, 2016 23:11

Run google-cloud-python system tests against new runtime images. #39

Run google-cloud-python system tests against new runtime images. #39

Conversation

duggelz commented Nov 3, 2016

dlorenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duggelz Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

theacodes commented Nov 3, 2016

dlorenc commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duggelz commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duggelz commented Nov 3, 2016

duggelz Nov 3, 2016 •

edited

Loading