Add the support to build container image for kn #152

houshengbo · 2019-05-30T19:46:34Z

Fixes-Bug: #80

knative-prow-robot · 2019-05-30T19:46:48Z

Hi @houshengbo. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

houshengbo · 2019-05-30T19:59:09Z

/assign @sixolet

sixolet · 2019-05-31T00:23:03Z

I feel like setting up a whole deployment yaml is misleading. How about this alternate section in release.sh:

  echo "🚧 🐳 Building the container image"
  ko publish ${KO_FLAGS}  github.com/knative/client/cmd/kn 2>&1 | grep -E '(Loaded|Published)' | rev | cut -d ' ' -f 1 | rev > kn-image-location
  ARTIFACTS_TO_PUBLISH="kn-darwin-amd64 kn-linux-amd64 kn-windows-amd64.exe kn-image-location"

Aside to @mattmoor : I'd love if ko put the images it published on stdout instead of logged to stderr.

sixolet · 2019-05-31T00:43:37Z

Re aside to @mattmoor , see ko-build/ko#36

houshengbo · 2019-05-31T15:52:50Z

@sixolet I was trying to generate a yaml at the beginning, but later quit.
I tested source github.com/knative/test-infra/scripts/release.sh. It seems it did not parse the image name like <KO_REPO>/<project_full_name>/kn-7f6fb725d2d3fb3de6c3751a3e000f87@sha256:490de70d5a223703422d02c5073b1b4f8213d166011a6d2e3bda09e2cfa7344a.
We can reply on ko publish command to push the container image upstream, like ko publish ${KO_FLAGS} github.com/knative/client/cmd/kn -t ${version}, which is currently the PR looks like.
We do not need to generate a yaml or any image location. As long as we check on (( ! PUBLISH_RELEASE )) to decide whether the container image goes to local repository ko.local, or remotely to gc.io/knative-nightly, we should be able to push the image.
In addition, we use the short name kn instead of github.com/knative/client/cmd/kn.
What do you think?

sixolet · 2019-05-31T18:07:23Z

We can reply on ko publish command to push the container image upstream, like ko publish ${KO_FLAGS} github.com/knative/client/cmd/kn -t ${version}, which is currently the PR looks like.

This approach is generally good.

We do not need to generate a yaml or any image location. As long as we check on (( ! PUBLISH_RELEASE )) to decide whether the container image goes to local repository ko.local, or remotely to gc.io/knative-nightly, we should be able to push the image.

But I think we need more, from a release management standpoint. We can't just tag every image as :latest; that should be reserved for the most recent release cannonical image, and should be managed by the release branching publishing scripts (attn: @adrcunha ). We can and should use a file (not yaml, in this case, I think) to point to the image we published by digest, so we can always know what image goes with what run of the release scripting.

In addition, we use the short name kn instead of github.com/knative/client/cmd/kn.

No strong opinions; the long name is clear, the short name is concise.

houshengbo · 2019-05-31T18:35:54Z

I will generate a file with the image link.

@sixolet Based on the current PR, the tag latest is only used when we push the image to the local ko.local(PUBLISH_RELEASE is false). What else can be a good option?
If we are about to release it(PUBLISH_RELEASE is true), the tag uses {version}, not latest. Is this OK?

sixolet · 2019-05-31T19:08:36Z

/ok-to-test

sixolet · 2019-05-31T19:09:08Z

/assign @adrcunha Can you weigh in on best practices for image tagging, and what step should do that?

adrcunha · 2019-05-31T19:20:48Z

hack/release.sh

@@ -32,7 +32,9 @@ function build_release() {
  GOOS=darwin GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-darwin-amd64 ./cmd/...
  echo "🚧 🎠 Building for Windows"
  GOOS=windows GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-windows-amd64.exe ./cmd/...
-  ARTIFACTS_TO_PUBLISH="kn-darwin-amd64 kn-linux-amd64 kn-windows-amd64.exe"
+  echo "🚧 🐳 Building the container image"
+  ko publish ${KO_FLAGS} github.com/knative/client/cmd/kn 2>&1 | grep -E '(Loaded|Published)' | rev | cut -d ' ' -f 1 | rev > kn-image-location


This will hide any error messages from ko publish. At a minimum, tee should be used, although I think this is a unnecessary convoluted and brittle way of extracting the location of the generated image: this can be easily derived from KO_REPO_PATH and the package name.

It is brittle; ko-build/ko#36 is now merged and solves this problem, but ko isn't released with that yet.

Could you elaborate more how to integrate ko publish with tee command?

Just pipe it. Although I do not recommend, specially now that the problem is solved.

@sixolet has a PR for ko ko-build/ko#36 already merged. @adrcunha I do not think we need tee any more, since we can print in stdout.

I still insist on not using such brittle approach. Instead I would recommend cleanly getting the hash through gcloud, like

gcloud container images describe gcr.io/XXXXXX/github.com/knative/client/cmd/kn:latest --format="value(image_summary.digest)"

adrcunha · 2019-05-31T19:22:10Z

hack/release.sh

-  ARTIFACTS_TO_PUBLISH="kn-darwin-amd64 kn-linux-amd64 kn-windows-amd64.exe"
+  echo "🚧 🐳 Building the container image"
+  ko publish ${KO_FLAGS} github.com/knative/client/cmd/kn 2>&1 | grep -E '(Loaded|Published)' | rev | cut -d ' ' -f 1 | rev > kn-image-location
+  ARTIFACTS_TO_PUBLISH="kn-darwin-amd64 kn-linux-amd64 kn-windows-amd64.exe kn-image-location"


This seems weird to me. You really want to publish a text file with a GCR URL in it? Wouldn't it be better to add that to the release notes instead?

Goal: Tie the image by hash to the release.

The published yamls from the other repos, the ones you can kubectl apply, accomplish this goal, but that doesn't make sense for kn. Once Tekton is further along we could publish a Tekton Task as a yaml file, but I don't think we're ready yet. So just a text file containing the image by hash seems simplest — is there a better way?

Initially I used ko resolve command. That generates a yaml file, so that github.com/knative/test-infra/scripts/release.sh can process later in the function tag_images_in_yamls ${ARTIFACTS_TO_PUBLISH} to change the tag.

But later I quit, since tag_images_in_yamls has bug, and it does not parse any image name ko resolve generates.

If we can specify the tag here, then we do not need to bother github.com/knative/test-infra/scripts/release.sh. But I do not know how to specify the tag for the image in this release.sh.

@hohaichi tag_images_in_yamls() works fine for all other repos (parsing yamls that ko resolve generates), can you please clarify what bug you're experiencing?

@sixolet I understanding that you're interested in the image hash, and that's unrelated to tagging it on GCR. If so, would adding it to the release notes be enough?

I'm getting a 404 for the URL you mentioned. Anyway, the regex is not meant to catch image names with hashes. I think you're running an old version of ko because newer versions won't append the hash by default, and you don't need to specify -B:

$ export KO_DOCKER_REPO=gcr.io/XXXXXXXXX $ ko publish -P github.com/knative/client/cmd/kn 2019/06/03 10:30:25 Building github.com/knative/client/cmd/kn 2019/06/03 10:30:29 Using base gcr.io/distroless/base:latest for github.com/knative/client/cmd/kn 2019/06/03 10:30:30 Publishing gcr.io/XXXXXXXXX/github.com/knative/client/cmd/kn:latest 2019/06/03 10:30:31 existing blob: sha256:1558143043601a425aa864511da238799b57fcf7d062d47044f6ddd0e04fe99a 2019/06/03 10:30:31 existing blob: sha256:4003b5b92ca98a8926d9112839f3f17e69f4ec4f995abb188a3ce3ccf93cd6d9 2019/06/03 10:30:31 existing blob: sha256:5f5edd681dcbc3a4a9df93e200e59e1708031e65b2299970eabdc91a78cc8234 2019/06/03 10:30:33 pushed blob: sha256:9e0fd63e6f10b3e99b134f68e8f68ba5de196080141d6254d875994c4868030b 2019/06/03 10:30:35 pushed blob: sha256:3ace94f228dcde95346dabc5a0e17c0cff33442a0fd49691d0233d1cfd25e8ed 2019/06/03 10:30:36 gcr.io/XXXXXXXXX/github.com/knative/client/cmd/kn:latest: digest: sha256:4d5a06964d68d645c743d5e02588a8340f749548db47024470ed917d84fc52ce size: 915 2019/06/03 10:30:36 Published gcr.io/XXXXXXXXX/github.com/knative/client/cmd/kn@sha256:4d5a06964d68d645c743d5e02588a8340f749548db47024470ed917d84fc52ce

OK , as long as it is able to parse the image name, that's fine.

will do ko publish ==> ko resolve

@hohaichi tag_images_in_yamls() works fine for all other repos (parsing yamls that ko resolve generates), can you please clarify what bug you're experiencing?

@adrcunha I don't have any context. Did you mean houshengbo? :)

Sorry, wrong username from Auto complete. :/

adrcunha · 2019-05-31T19:24:17Z

the tag latest is only used when we push the image to the local ko.local(PUBLISH_RELEASE is false).

No. latest is applied to any images you publish; a date/hash tag is also applied later, see the markdown file for details.

If we are about to release it(PUBLISH_RELEASE is true), the tag uses {version}, not latest. Is this OK?

The version is used only if you specify it in the command-line. Currently, it only happens for named releases, not nightly releases.

adrcunha · 2019-05-31T19:32:49Z

We can reply on ko publish command to push the container image upstream, like ko publish ${KO_FLAGS} github.com/knative/client/cmd/kn -t ${version}, which is currently the PR looks like.

You don't need to pass -t, KO_FLAGS already has it when necessary. But I see that you're already doing it.

We do not need to generate a yaml or any image location. As long as we check on (( ! PUBLISH_RELEASE )) to decide whether the container image goes to local repository ko.local, or remotely to gc.io/knative-nightly, we should be able to push the image.

This sentence confuses me. You can call ko publish without any special case, the right repo is already set in KO_REPO_PATH (local, nightly, release or custom).

But I think we need more, from a release management standpoint. We can't just tag every image as :latest; that should be reserved for the most recent release cannonical image, and should be managed by the release branching publishing scripts (attn: @adrcunha ).

Yes. That's done by the release Prow jobs. Unless you're a Prow job or an admin, you won't be able to push to nightly GCR.

We can and should use a file (not yaml, in this case, I think) to point to the image we published by
digest, so we can always know what image goes with what run of the release scripting.

As I mentioned in another comment, you can easily assemble the path to the binary, based on the version or nightly tag, or latest. Like gcr.io/knative-nightly/github.com/knative/client/cmd/kn:latest or gcr.io/knative-nightly/github.com/knative/client/cmd/kn:v20193005-fooba4 or gcr.io/knative-releases/github.com/knative/client/cmd/kn:v0.2.0

In addition, we use the short name kn instead of github.com/knative/client/cmd/kn.

Do you mean simply gcr.io/knative-nightly/kn? Although we can support that, it would be against all the GCR/GCS organization used by all the other Knative modules. I'd check with the TOC first to avoid confusion and unexpected surprises.

sixolet · 2019-05-31T19:36:05Z

Yes. That's done by the release Prow jobs. Unless you're a Prow job or an admin, you won't be able to push to nightly GCR.

What determines which images the Prow jobs push where?

adrcunha · 2019-05-31T21:24:01Z

Yes. That's done by the release Prow jobs. Unless you're a Prow job or an admin, you won't be able to push to nightly GCR.
What determines which images the Prow jobs push where?

The build_release() function. It can push anything to KO_DOCKER_REPO.

rhuss · 2019-06-03T19:02:37Z

Integration test should be fixed if #155 is merged and this PR rebased on top of it

sixolet · 2019-06-03T21:00:59Z

config/release.yaml

+# limitations under the License.
+
+apiVersion: apps/v1
+kind: Deployment


Can we do something that doesn't imply you should want to apply this to a k8s cluster, if we must have yaml? Would plain old

image: github.com/knative/client/cmd/kn

work?

I tried. It worked if we only kept "image: github.com/knative/client/cmd/kn".
Already changed it.

maximilien · 2019-06-05T22:36:57Z

hack/release.sh

@@ -32,7 +32,9 @@ function build_release() {
  GOOS=darwin GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-darwin-amd64 ./cmd/...
  echo "🚧 🎠 Building for Windows"
  GOOS=windows GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-windows-amd64.exe ./cmd/...
-  ARTIFACTS_TO_PUBLISH="kn-darwin-amd64 kn-linux-amd64 kn-windows-amd64.exe"
+  echo "🚧 🐳 Building the container image"
+  ko resolve ${KO_FLAGS} -f config/ > kn-image-location.yaml


Does that now assume ko present and the default container image builder? If that's a widely held assumption in Knative then cool. I'd assume some folks would want to use docker?

Yes, ko is used in all other repos. If you change to docker it will break the release process. Also, release.sh will be hardly used locally, and mostly by the release jobs.

sixolet · 2019-06-07T00:18:39Z

/approve
/lgtm

knative-prow-robot · 2019-06-07T00:18:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, sixolet

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~~ [sixolet]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 30, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2019

knative-prow-robot requested review from evankanderson and sixolet May 30, 2019 19:46

knative-prow-robot assigned sixolet May 30, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 31, 2019

sixolet assigned adrcunha May 31, 2019

adrcunha reviewed May 31, 2019

View reviewed changes

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 3, 2019

sixolet reviewed Jun 3, 2019

View reviewed changes

Add the support to build container image for kn

9e56500

maximilien reviewed Jun 5, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2019

knative-prow-robot merged commit 5d2a7aa into knative:master Jun 7, 2019

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Use https for cloning repo on go coverage jobs (knative#152)

dac6fbe

Add the support to build container image for kn #152

Add the support to build container image for kn #152

Conversation

houshengbo commented May 30, 2019 • edited Loading

knative-prow-robot commented May 30, 2019

houshengbo commented May 30, 2019

sixolet commented May 31, 2019

sixolet commented May 31, 2019

houshengbo commented May 31, 2019

sixolet commented May 31, 2019

houshengbo commented May 31, 2019 • edited Loading

sixolet commented May 31, 2019

sixolet commented May 31, 2019

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

adrcunha commented May 31, 2019

adrcunha commented May 31, 2019

sixolet commented May 31, 2019

adrcunha commented May 31, 2019

rhuss commented Jun 3, 2019

Choose a reason for hiding this comment

houshengbo Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixolet commented Jun 7, 2019

knative-prow-robot commented Jun 7, 2019

houshengbo commented May 30, 2019 •

edited

Loading

houshengbo commented May 31, 2019 •

edited

Loading

houshengbo Jun 3, 2019 •

edited

Loading