search

knative-extensions / kn-plugin-event

Kn plugin for sending events to Knative sinks.
Apache License 2.0
7 stars 22 forks source link

:gift: Easier debugging, especially for the in-cluster sender #375

Closed cardil closed 1 week ago

cardil commented 2 weeks ago

Changes

/kind enhancement

Fixes #129

Release Note

The errors are easier to debug because the in-cluster jobs and their pods are logged and users are directed to them.
knative-prow[bot] commented 2 weeks ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 74.88987% with 57 lines in your changes missing coverage. Please review.

Project coverage is 68.92%. Comparing base (b7dd9cb) to head (3d07d88). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/k8s/job_gatherer.go 73.58% 9 Missing and 5 partials :warning:
internal/cli/errors.go 82.35% 6 Missing and 3 partials :warning:
pkg/sender/in_cluster.go 55.00% 6 Missing and 3 partials :warning:
pkg/cli/context.go 12.50% 5 Missing and 2 partials :warning:
pkg/errors/wrap.go 72.72% 4 Missing and 2 partials :warning:
pkg/k8s/jobrunner.go 87.17% 5 Missing :warning:
pkg/ics/send.go 0.00% 4 Missing :warning:
internal/cli/send.go 0.00% 2 Missing :warning:
internal/cli/build.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #375 +/- ## ========================================== + Coverage 66.52% 68.92% +2.39% ========================================== Files 48 52 +4 Lines 1440 1622 +182 ========================================== + Hits 958 1118 +160 - Misses 412 424 +12 - Partials 70 80 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cardil commented 2 weeks ago

/test all

cardil commented 2 weeks ago

/test all

rhuss commented 1 week ago

Nice PR, great work!

I do not have much time to review it thoroughly, but I am happy to ack it. @cardil, is there anything you want to add before merging?

cardil commented 1 week ago

@rhuss: PR, great work!

Thanks. The PR works great, so I'm happy to merge it.

BTW. I think we can adopt very similar error processing across all of kn and fun. WDYT?

dsimansk commented 1 week ago

/approve /lgtm

knative-prow[bot] commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, dsimansk

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](https://github.com/knative-extensions/kn-plugin-event/blob/main/OWNERS)~~ [cardil,dsimansk] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
rhuss commented 1 week ago

BTW. I think we can adopt very similar error processing across all of kn and fun. WDYT?

That sounds like a good idea. Maybe start with talking to the func team, as this has more interactive elements already anyway. Also, we need to be sure, that the spinner is only active if there is a tty. Not sure if the PR deals already with that, that when no tty is available (i.e. used in a pipe or a redirection), then no control characters or colors should be printed.

cardil commented 1 week ago

@rhuss: we need to be sure, that the spinner is only active if there is a tty. Not sure if the PR deals already with that, that when no tty is available (i.e. used in a pipe or a redirection), then no control characters or colors should be printed.

That's already addressed in knative.dev/client/pkg library and in bubblegum library as well.