Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix kn trigger list command to show correct v1/service sink output #1428

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

Abirdcfly
Copy link
Contributor

Description

If the sink of the trigger is a normal service, the kn trigger list command will show it as ksvc, which is incorrect

Changes

Reference

Fixes #

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 14, 2021
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@Abirdcfly: 0 warnings.

In response to this:

Description

If the sink of the trigger is a normal service, the kn trigger list command will show it as ksvc, which is incorrect

Changes

Reference

Fixes #

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.

@knative-prow-robot 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 Aug 14, 2021
@knative-prow-robot
Copy link
Contributor

Welcome @Abirdcfly! It looks like this is your first PR to knative/client 🎉

@knative-prow-robot
Copy link
Contributor

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

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 14, 2021
@Abirdcfly
Copy link
Contributor Author

/assign @RichieEscarez

@itsmurugappan
Copy link
Contributor

/ok-to-test

@knative-prow-robot 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 Aug 14, 2021
@knative-prow-robot 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 Aug 14, 2021
Comment on lines -86 to -87
"kind": "Service",
"name": "foo",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only kind and name are not enough here. An error will appear: "Unable to get the Subscriber's URI"

Comment on lines -191 to -192
"kind": "Service",
"name": "foo",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only kind and name are not enough here. An error will appear: "Unable to get the Subscriber's URI"

@itsmurugappan
Copy link
Contributor

itsmurugappan commented Aug 15, 2021

looks like Group is not stabilized, more details here -> knative/eventing#5086. But if you include the version in the check, won't it need to be changed when a new knative serving version comes?

@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #1428 (126e869) into main (2e71451) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 126e869 differs from pull request most recent head 19f270a. Consider uploading reports for the commit 19f270a to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1428   +/-   ##
=======================================
  Coverage   78.70%   78.70%           
=======================================
  Files         162      162           
  Lines        8368     8368           
=======================================
  Hits         6586     6586           
  Misses       1098     1098           
  Partials      684      684           
Impacted Files Coverage Δ
pkg/kn/commands/flags/sink.go 87.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e71451...19f270a. Read the comment docs.

Copy link
Contributor

@itsmurugappan itsmurugappan left a comment

Choose a reason for hiding this comment

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

everything looks good, one small suggestion.

pkg/kn/commands/flags/sink.go Outdated Show resolved Hide resolved
Co-authored-by: Murugappan Chetty <itsmurugappan@gmail.com>
@itsmurugappan
Copy link
<