Skip to content
This repository was archived by the owner on Dec 1, 2022. It is now read-only.

[release-v0.23.2] TestServiceExport works with images specified by digest (#1435)#794

Merged
openshift-merge-robot merged 1 commit intoopenshift:release-v0.23.2from
mgencur:service_export_digest_v0.23.2
Aug 31, 2021
Merged

[release-v0.23.2] TestServiceExport works with images specified by digest (#1435)#794
openshift-merge-robot merged 1 commit intoopenshift:release-v0.23.2from
mgencur:service_export_digest_v0.23.2

Conversation

@mgencur
Copy link
Copy Markdown

@mgencur mgencur commented Aug 30, 2021

Backport of knative#1435 to release-0.23.2 branch.
This should allow openshift/release#21285 to pass and allow for using cluster-pools.

  • TestServiceExport works with images specified by digest

  • Do not expect image name in TestSourceContainer test

  • it might not be available if the test image is passed as image digest

* TestServiceExport works with images specified by digest

* Do not expect image name in TestSourceContainer test

* it might not be available if the test image is passed as image digest
@mgencur mgencur changed the title TestServiceExport works with images specified by digest (#1435) [release-v0.23.2] TestServiceExport works with images specified by digest (#1435) Aug 30, 2021
servingtest.WithConfigSpec(test.BuildConfigurationSpec()),
servingtest.WithBYORevisionName("hello-rev1"),
test.WithRevisionAnnotations(map[string]string{"client.knative.dev/user-image": pkgtest.ImagePath("helloworld")}),
test.WithRevisionAnnotations(map[string]string{"client.knative.dev/user-image": userImage}),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the whole annotation not be set if the userImage is "" (instead of setting the annotation with an empty string ?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've chosen this approach to keep the tests simple. In the end the annotation is set to an empty value by the kn client anyway: https://github.com/knative/client/blob/release-0.23/pkg/kn/commands/service/create.go#L261

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but as it seems to work upstream also with an empty value, looks fine to me. Please note though that this fix might not work with --lock-to-digest option as this option relies on this annotation to contain the value with digest (and if I think twice, I think this annotation is supposed to always hold the digest, but let me check)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no. it's the other way round, actually the annotation shouldn't contain any digest, so the fix looks good to me.

/approve
/lgtm

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2021
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Aug 31, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mgencur, rhuss

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2021
@openshift-merge-robot openshift-merge-robot merged commit 22aba44 into openshift:release-v0.23.2 Aug 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants