Drop Dockerfile in test images by yolocs · Pull Request #1792 · knative/serving

yolocs · 2018-08-03T07:41:05Z

Not sure if this is the right way of doing this.
This will change the repo for test images:
e.g. gcr.io/myproj/knative-serving => gcr.io/myproj//github.com/knative/serving/test/conformance/test_images

And the repo will be different for e2e and comformance tests unless we move all test images to a single import path, e.g. github.com/knative/serving/test/images

google-prow-robot · 2018-08-03T07:41:19Z

Hi @yolo3301. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-prow-robot · 2018-08-03T07:41:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yolo3301
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: adrcunha

If they are not already assigned, you can assign the PR to them by writing /assign @adrcunha in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

test/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adrcunha · 2018-08-03T16:39:45Z

I'm fine with dropping Dockerfiles. They were actually a leftover from the time where the first test apps were written in NodeJS. :)

Yes, this will break the E2E tests. I think it makes sense moving the test images to a single location as you proposed; now they can even share common code and stuff.

Even better, this change would allow the e2e-tests.sh script to get rid of prebuilt images (thus fixing #1700), as now they can be built and published by ko to the test' projects GCR during the test.

yolocs · 2018-08-04T06:09:34Z

@adrcunha I made some updates. But I couldn't verify much because I couldn't run e2e-tests.sh successfully. Does it require access to test-infra? Some doc would be helpful.

adrcunha · 2018-08-04T06:42:27Z

Thanks for doing this, yolo3301.

e2e-tests.sh doesn't require access to test-infra. All the necessary documentation should be in test/README.md and in the script header, but it's as simple as setting PROJECT_ID to your personal GCP project and calling e2e-tests.sh. If you're facing issues, please describe them so I can help you.

adrcunha · 2018-08-04T06:43:04Z

test/README.md

@@ -102,24 +102,20 @@ go test -v -tags=e2e -count=1 ./test/e2e --resolvabledomain
 The [`upload-test-images.sh`](./upload-test-images.sh) script can be used to build and push the
 test images used by the conformance and e2e tests. It requires:


Please note here that this is now only required when running the tests locally.

adrcunha · 2018-08-04T06:43:44Z

test/README.md

-
-The new test images will also need to be uploaded to the e2e tests Docker repo. You will need one
-of the owners found in [`/test/OWNERS`](OWNERS) to do this.
+New test images should be placed `./test/test_images`.


typo: "placed in"

adrcunha · 2018-08-04T06:44:49Z

test/README.md

-of the owners found in [`/test/OWNERS`](OWNERS) to do this.
+New test images should be placed `./test/test_images`.

 Because the test images are uploaded to the same folder, they **must** have different names.


This paragraph can now be removed.

adrcunha · 2018-08-04T06:47:04Z

test/e2e-tests.sh

+function publish_test_images() {
+  image_dirs="$(find test/test_images -depth 1 -type d)"
+  for image_dir in ${image_dirs}; do
+    echo "Publishing test image github.com/knative/serving/${image_dir}"


Isn't this echo redundant? Isn't ko publish verbose about the publishing?

adrcunha · 2018-08-04T06:48:31Z

test/e2e-tests.sh

 wait_until_pods_running istio-system || fail_test "Istio system is not up"
 wait_until_service_has_external_ip istio-system knative-ingressgateway || fail_test "Ingress has no external IP"

+publish_test_images


Move this to line 131, so it fails fast if something goes wrong.

adrcunha · 2018-08-04T07:06:13Z

test/e2e-tests.sh

 }

+function publish_test_images() {
+  image_dirs="$(find test/test_images -depth 1 -type d)"


Shouldn't it be maxdepth here?

maxdepth will include the current path (which is test/test_images). ko will report error because it doesn't has a go package inside.

adrcunha · 2018-08-04T07:06:25Z

test/upload-test-images.sh


-DOCKER_FILES="$(find $@ -name Dockerfile)"
-: ${DOCKER_FILES:?"No subdirectories with Dockerfile files found in $@"}
+IMAGE_DIRS="$(find $@ -depth 1 -type d)"


Shouldn't it be maxdepth here?

adrcunha · 2018-08-04T07:07:15Z

test/upload-test-images.sh

@@ -17,14 +17,10 @@
 set -o errexit

 : ${1:?"Pass the directories with the test images as arguments"}


The location of the test images is now fixed, you don't need arguments for this script anymore.

adrcunha · 2018-08-04T07:07:55Z

test/README.md


 ```bash
-./test/upload-test-images.sh ./test/e2e/test_images ./test/conformance/test_images
+./test/upload-test-images.sh test/test_images


The location of the test images is now fixed, you don't need arguments for this script anymore (see my comment below).

adrcunha · 2018-08-04T07:11:05Z

test/upload-test-images.sh


-DOCKER_FILES="$(find $@ -name Dockerfile)"
-: ${DOCKER_FILES:?"No subdirectories with Dockerfile files found in $@"}
+IMAGE_DIRS="$(find $@ -depth 1 -type d)"


Instead of $@, use the location relative to this script, which is $(dirname $0)/test_images.

yolocs · 2018-08-05T21:13:09Z

@adrcunha I'm still having some trouble running e2e-tests.sh. I always got error:

./test/e2e-tests.sh: line 74: [: -v: unary operator expected

I tried to upgrade my bash version but still got the same error.

jcrossley3 · 2018-08-06T17:26:33Z

test/e2e-tests.sh

 }

+function publish_test_images() {
+  image_dirs="$(find $(dirname $0)/test_images -depth 1 -type d)"


Should this be -mindepth 1? On Linux, it's an error to pass an arg to -depth

Indeed, that's why I mentioned -maxdepth. Also, on my Linux box it should be .../test_images/* otherwise it will also include test_images itself.

Also, use ${REPO_ROOT_DIR}/test/test_images as the absolute location.

In my tests on Mac and Linux, I found using find ${REPO_ROOT_DIR}/test/test_images -mindepth 1 -maxdepth 1 -type d works the best. @jcrossley3 @adrcunha Let me know if that works for you. .../test/test_images/* with just -maxdepth doesn't give the right paths.

jcrossley3 · 2018-08-06T18:50:33Z

test/upload-test-images.sh


-DOCKER_FILES="$(find $@ -name Dockerfile)"
-: ${DOCKER_FILES:?"No subdirectories with Dockerfile files found in $@"}
+IMAGE_DIRS="$(find $(dirname $0)/test_images -depth 1 -type d)"


-mindepth 1?

Same comment about maxdepth and ${REPO_ROOT_DIR}.

mattmoor · 2018-08-06T19:33:18Z

test/README.md

-* [`DOCKER_REPO_OVERRIDE`](/DEVELOPMENT.md#environment-setup) to be set
+* [`KO_DOCKER_REPO`](/DEVELOPMENT.md#environment-setup) to be set
 * You to be [authenticated with your
  `DOCKER_REPO_OVERRIDE`](/docs/setting-up-a-docker-registry.md)


change this too

Actually, either keep DOCKER_REPO_OVERRIDE or update the documentation, the flags and the scripts, as they still rely on this value for the default value of --dockerrepo.

I changed it back. In upload-test-images.sh, it also changed it back to require DOCKER_REPO_OVERRIDE so that the doc here can still make sense. But I added export KO_DOCKER_REPO=${DOCKER_REPO_OVERRIDE} just like it in e2e-test.sh. Let me know if that makes sense.

adrcunha · 2018-08-06T20:01:10Z

./test/e2e-tests.sh: line 74: [: -v: unary operator expected

I only see this happening so far on Macs. That would also explain the differences we're seeing with the arguments for find. For development purposes, you can remove this check now, it's going to be dropped today or tomorrow.

adrcunha

Also please update test_images/README.md, as it still references the conformance tests (I could swear I already wrote this, but...)

adrcunha · 2018-08-06T19:49:21Z

test/README.md

-
-New test images should be placed in their own subdirectories. Be sure to to include a `Dockerfile`
-for building and running the test image.
+Note: this is only required when you run comformance/e2e tests locally with `go test` commands.


Better move this note to the top of the section.

adrcunha · 2018-08-06T19:50:16Z

test/e2e-tests.sh

 }

+function publish_test_images() {
+  image_dirs="$(find $(dirname $0)/test_images -depth 1 -type d)"


Indeed, that's why I mentioned -maxdepth. Also, on my Linux box it should be .../test_images/* otherwise it will also include test_images itself.

Also, use ${REPO_ROOT_DIR}/test/test_images as the absolute location.

adrcunha · 2018-08-06T19:58:22Z

test/README.md

-* [`DOCKER_REPO_OVERRIDE`](/DEVELOPMENT.md#environment-setup) to be set
+* [`KO_DOCKER_REPO`](/DEVELOPMENT.md#environment-setup) to be set
 * You to be [authenticated with your
  `DOCKER_REPO_OVERRIDE`](/docs/setting-up-a-docker-registry.md)


Actually, either keep DOCKER_REPO_OVERRIDE or update the documentation, the flags and the scripts, as they still rely on this value for the default value of --dockerrepo.

adrcunha · 2018-08-06T20:06:19Z

test/upload-test-images.sh


-DOCKER_FILES="$(find $@ -name Dockerfile)"
-: ${DOCKER_FILES:?"No subdirectories with Dockerfile files found in $@"}
+IMAGE_DIRS="$(find $(dirname $0)/test_images -depth 1 -type d)"


Same comment about maxdepth and ${REPO_ROOT_DIR}.

jcrossley3 · 2018-08-08T13:24:26Z

test/upload-test-images.sh


-: ${KO_DOCKER_REPO:?"You must set 'KO_DOCKER_REPO', see DEVELOPMENT.md"}
+: ${DOCKER_REPO_OVERRIDE:?"You must set 'DOCKER_REPO_OVERRIDE', see DEVELOPMENT.md"}
+: ${REPO_ROOT_DIR:?"You must set 'REPO_ROOT_DIR' to knative serving repo root"}


Why require users to set this? For developers running local go test ... tests, it seems like a path relative to dirname is more convenient, or at least a reasonable default.

Sorry if I wasn't clear. ${REPO_ROOT_DIR} is a convenience variable set for e2e-tests.sh only. Indeed, use dirname to figure out the location of test_images without adding any further dependency.

Done. I made the change because you mentioned ${REPO_ROOT_DIR} in this file also.

Yes, my fault, I apologize for the misguidance.

adrcunha · 2018-08-08T17:22:55Z

Great work, @yolo3301. You should be able to run the e2e tests locally now if you temporarily comment line 32 in e2e-tests.sh. Make sure you have $PROJECT_ID set to a GCP project of yours.

yolocs · 2018-08-09T06:30:32Z

@adrcunha I was able to run e2e-tests.sh with some hack. The problem was introduced by eval test-infra scripts. But they should still be Mac issue.

It doesn't recognize ${1^^} in library.sh
It doesn't recognize -v in e2e-tests.sh under test-infra
wait_* functions failed to return when all pods are running

adrcunha · 2018-08-09T06:54:18Z

Thanks, @yolo3301.

I'm aware of [1]; [2] should go away once the helper scripts are vendored, but [3] is concerning. Are you able to debug and understand why it's failing?

/ok-to-test

yolocs · 2018-08-09T14:54:27Z

/retest

adrcunha · 2018-08-09T17:12:48Z

@yolo3301, /retest won't help. All e2e tests failed, except those were failure is expected. I skimmed through the summary and I believe that the images are not being found (or, if they are, they are broken somehow). I suggest you get at least HelloWorld working using go test on your machine before trying again (that should help you figure out what's wrong with all tests).

yolocs · 2018-08-09T18:06:02Z

@adrcunha Manually running upload-test-images.sh and go test locally does work for me.

I checked the build log, it seems the ko publish behaved differently. HelloWorld for example, it published to gcr.io/knative-boskos-04/kserving-e2e-img/helloworld-6ff6a48702d2f0ff1473a2223e9fe0f3:latest but from the error:

"Failed to fetch \"latest\" from request \"/v2/knative-boskos-04/kserving-e2e-img/github.com/knative/serving/test/test_images/helloworld/manifests/latest\"."".'

It was pulling gcr.io/knative-boskos-04/kserving-e2e-img/github.com/knative/serving/test/test_images/helloworld

adrcunha · 2018-08-09T18:16:02Z

Ah! Use ko -P to preserve the paths when publishing. That should solve the problem.

yolocs · 2018-08-09T19:21:49Z

/retest

yolocs · 2018-08-09T20:27:17Z

@adrcunha after fixing another bug, the integ tests finally passed!

adrcunha · 2018-08-09T20:38:12Z

test/e2e-tests.sh

  image_dirs="$(find ${REPO_ROOT_DIR}/test/test_images -mindepth 1 -maxdepth 1 -type d)"
  for image_dir in ${image_dirs}; do
-    ko publish "github.com/knative/serving/test/test_images/$(basename ${image_dir})"
+    ko publish -P "github.com/knative/serving/test/test_images/$(basename ${image_dir})"


Isn't it also necessary in the upload-test-images.sh script?

srinivashegde86 · 2018-08-09T22:26:20Z

test/upload-test-images.sh

-  docker build "${image_dir}" -f "${docker_file}" -t "${versioned_name}"
-  docker push "${versioned_name}"
+for image_dir in ${IMAGE_DIRS}; do
+  ko publish "github.com/knative/serving/test/test_images/$(basename ${image_dir})"


should this also have -P to preserve the paths?

mattmoor · 2018-08-09T23:53:22Z

test/upload-test-images.sh

-  docker build "${image_dir}" -f "${docker_file}" -t "${versioned_name}"
-  docker push "${versioned_name}"
+for image_dir in ${IMAGE_DIRS}; do
+  ko publish -P "github.com/knative/serving/test/test_images/$(basename ${image_dir})"


cc @imjasonh 🦜 thanks for ko publish 🎉

mattmoor · 2018-08-09T23:54:10Z

test/e2e-tests.sh

+function publish_test_images() {
+  image_dirs="$(find ${REPO_ROOT_DIR}/test/test_images -mindepth 1 -maxdepth 1 -type d)"
+  for image_dir in ${image_dirs}; do
+    ko publish -P "github.com/knative/serving/test/test_images/$(basename ${image_dir})"


It is notable that this will not work with DockerHub (and possible other registries), which is why I'd changed the ko default behavior :(

Shall I fix this separately?

mattmoor · 2018-08-09T23:55:26Z

/approve

adrcunha · 2018-08-10T00:50:58Z

/approve
/lgtm

knative-prow-robot · 2018-08-10T00:51:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrcunha, mattmoor, yolo3301

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [adrcunha,mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* removed dockerfiles * moved test images to the same location * adjust repo * fix command and docs * update upload-test-images.sh * preserve import paths * fix appYaml path * preserve path in upload-test-images.sh Backport of #1792.

…2208) * Fix SSH keys workaround for kubetest Create the ~/.ssh dir if it doesn't exist, don't assume it always exist. Backported from knative/test-infra#151 * Fix authentication for test clusters Instead of relying on default options, use basic authentication for test cluster. Also make acquire_cluster_admin_role() handle auth through certificates, since it's used also on deployment. Backport of knative/test-infra#115 * Increase E2E tests timeout to 20 minutes Currently 10 minutes may not be enough, for example as in #1701. Backport of #1702. * Drop Dockerfile in test images * removed dockerfiles * moved test images to the same location * adjust repo * fix command and docs * update upload-test-images.sh * preserve import paths * fix appYaml path * preserve path in upload-test-images.sh Backport of #1792.

google-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 3, 2018

google-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 3, 2018

google-prow-robot requested review from adrcunha and steuhs August 3, 2018 07:41

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2018

adrcunha suggested changes Aug 4, 2018

View reviewed changes

jcrossley3 reviewed Aug 6, 2018

View reviewed changes

mattmoor reviewed Aug 6, 2018

View reviewed changes

adrcunha suggested changes Aug 6, 2018

View reviewed changes

yolocs added 5 commits August 7, 2018 23:47

Use KO to upload test images

886b48e

removed dockerfiles

6676a24

moved test images to the same location

89397f4

adjust repo

179d7ba

fix command and docs

d7c88d1

jcrossley3 reviewed Aug 8, 2018

View reviewed changes

Update upload-test-images.sh

17517ff

jcrossley3 mentioned this pull request Aug 8, 2018

Support conformance tests w/ minikube #609

Closed

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2018

adrcunha approved these changes Aug 9, 2018

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2018

preserve import paths

5835163

fix appYaml path

d3ff17c

adrcunha reviewed Aug 9, 2018

View reviewed changes

srinivashegde86 reviewed Aug 9, 2018

View reviewed changes

yolocs and others added 2 commits August 9, 2018 16:05

preserve path in upload-test-images.sh

763bdd4

Fix a typo

266c7ec

mattmoor reviewed Aug 9, 2018

View reviewed changes

knative-prow-robot assigned adrcunha Aug 10, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2018

knative-prow-robot merged commit 56865b8 into knative:master Aug 10, 2018

		@@ -102,24 +102,20 @@ go test -v -tags=e2e -count=1 ./test/e2e --resolvabledomain
		The [`upload-test-images.sh`](./upload-test-images.sh) script can be used to build and push the
		test images used by the conformance and e2e tests. It requires:

		@@ -17,14 +17,10 @@
		set -o errexit

		: ${1:?"Pass the directories with the test images as arguments"}

Conversation

yolocs commented Aug 3, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

adrcunha commented Aug 3, 2018

Uh oh!

yolocs commented Aug 4, 2018

Uh oh!

adrcunha commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

yolocs commented Aug 5, 2018

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!

adrcunha commented Aug 6, 2018

Uh oh!

adrcunha 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!

Choose a reason for hiding this comment

adrcunha commented Aug 4, 2018 •

edited

Loading