build test images for e2e

[itsmurugappan]

Description

Build test images for e2e.

Changes

• upload-test-images.sh , builds and publishes all images under test/test_images to the KO_DOCKER_REPO
• upload-test-images.sh triggered from common.sh
• ImagePath function resolves the image name referred in the test to a path that can be pulled
• There is a func added recently to look for the test image in the env variable, have not modified that.
• Made the changes only TestBasicWorkflow test to refer the built image, after the feedback from reviewers will change for others.

Reference

Fixes #942 , #949

/assign @navidshaikh

[knative-prow-robot]

@itsmurugappan: 0 warnings.

In response to this:

Description

Build test images for e2e.

Changes

• upload-test-images.sh , builds and publishes all images under test/test_images to the KO_DOCKER_REPO
• upload-test-images.sh triggered from common.sh
• ImagePath function resolves the image name referred in the test to a path that can be pulled
• There is a func added recently to look for the test image in the env variable, have not modified that.
• Made the changes only TestBasicWorkflow test to refer the built image, after the feedback from reviewers will change for others.

Reference

Fixes #942

/assign @navidshaikh

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.

[itsmurugappan]

/test pull-knative-client-integration-tests-latest-release

[navidshaikh]

thanks for picking this up!

Suggested to use knative.dev/pkg/test for ImagePath to streamline with other components and override base image for kn image

[navidshaikh]

There is a func added recently to look for the test image in the env variable, have not modified that.

I think we should remove it and align the test images building and reusing with other components, because if we add another test image (say h2c) we'll need to add another env var or more processing. cc: @mvinkler

[mvinkler]

I think we should remove it and align the test images building and reusing with other components, because if we add another test image (say h2c) we'll need to add another env var or more processing. cc: @mvinkler

Aligning with other components is the best approach IMHO. I am fine with removing the env var part.

[itsmurugappan]

Thanks for comments @navidshaikh have incorporated them.

[maximilien]

Is there a way to test this in CI?

[navidshaikh]

lgtm, can you please update the PR description to also fix #949 and add change-log entries? suggested a few minor nits as well.

[navidshaikh]

@itsmurugappan : smoke tests would still use the stale image and I was thinking if we could form the image URL in shell script using the available env vars

the test image URL formed is like:

gcr.io/knative-boskos-01/kclient-e2e-img/22383/helloworld:latest

where we $E2E_PROJECT_ID is knative-boskos-01, $E2E_BASE_NAME is kclient-e2e-img
I am not sure if 22383 is set in an env var, is there one for this value? @chaodaiG @imjasonh OR even better way to get the resolved path of ko image ?

[itsmurugappan]

@navidshaikh the image path is $KO_DOCKER_REPO/image-name. Have made the change on smoke test also. Please let me know if it looks good.

[navidshaikh]

@itsmurugappan lgtm 👍 , lets replace all the references of the test image and remove any provision for getting test image via env var (see https://github.com/knative/client/pull/957/files). Also, we'll run the base image selection for kn image through WG call later today.

[imjasonh]

This repo only defines one cmd/ package, right? Does defaultBaseImage accomplish anything, since it's just overridden below?

[navidshaikh]

Correct.

The test image should be using this base image(test/upload-test-images.sh), but we dont have any particular use case in test images to use this base image. Also just checked their sizes, the default one is 3 times larger than alpine image. @itsmurugappan I think we can simply default to alpine base image here.

[itsmurugappan]

The test images(present and future ones) will refer this base image. A non root image will give the flexibility to run in contributors clusters in which non root images are not allowed. Maybe I can change it to 'gcr.io/distroless/static:nonroot' which is used in all the knative repos.

[imjasonh]

What does this file accomplish? It's not a valid K8s config so ko apply will fail. Does something else consume the output of ko resolve on this file?

If you just want to publish an image, you should be able to simply run ko publish ./test/test_images/helloworld

[itsmurugappan]

The goal is build and publish the images to be used in tests. Currently there is only one image, we are planning to add more. So, if it is specified in this way , we could build and publish all images using 'ko resolve --jobs=4 ${tag_option} -RBf "${image_dir}" > /dev/null' .

[itsmurugappan]

@navidshaikh now all the tests are referring to the built image. The build tests are failing, even after running the build script am not able to see the changes that it is complaining about .

[navidshaikh]

@itsmurugappan : its failing in Verify_CodeGen step

ERROR: Modified files found:
 M vendor/knative.dev/pkg/test/logging/tlogger.go
 M vendor/knative.dev/pkg/test/tinterface.go

ref

please rebase onto master and re-run the hack/build.sh

[itsmurugappan]

@navidshaikh added the non root image in ko.yaml

[navidshaikh]

/lgtm
/approve

thank you @itsmurugappan !

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: itsmurugappan, maximilien, navidshaikh

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [maximilien,navidshaikh]

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