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.

Sign up for GitHub

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

Jump to bottom

Don't Display kn --help message with Plugin Errors #910

Closed
wants to merge 2 commits into from
Closed

Don't Display kn --help message with Plugin Errors #910

wants to merge 2 commits into from

Conversation

danielhelfand
Copy link
Contributor

Description

This pull request removes the kn --help message with a plugin error as detailed in #904.

Changes

  • Use a boolean pluginErr in main.go to determine whether to print an
  • Add e2e test in plugin_test.go to verify error message isn't present

Reference

Fixes #904

/lint

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 26, 2020
knative-prow-robot
knative-prow-robot reviewed Jun 26, 2020
View reviewed changes
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.

@danielhelfand: 0 warnings.

In response to this:

Description

This pull request removes the kn --help message with a plugin error as detailed in #904.

Changes

  • Use a boolean pluginErr in main.go to determine whether to print an
  • Add e2e test in plugin_test.go to verify error message isn't present

Reference

Fixes #904

/lint

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
Copy link
Contributor

Hi @danielhelfand. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2020
maximilien
maximilien approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

Nice. I was seeing this today. Thanks for the contribution. Cheers.

/ok-to-test

@knative-prow-robot knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 26, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand, maximilien

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:

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

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 26, 2020
rhuss
rhuss requested changes Jun 29, 2020
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks, but please don't use global state variables as this will lead to hard understand spaghetti code (because of the non-local nature of global variables). See a suggestion how to improve this below in the comments.

Please note also that in the future we might want to re-add that line for plugins as a plugin is supposed to support a --help flag, too (and plugins will be listed along regular commands with kn --help in an extra section, as this is what the user does when looking for usage help).

cmd/kn/main.go Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2020
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/kn/main.go 78.1% 75.3% -2.8

@danielhelfand
Copy link
Contributor Author

Please note also that in the future we might want to re-add that line for plugins as a plugin is supposed to support a --help flag, too (and plugins will be listed along regular commands with kn --help in an extra section, as this is what the user does when looking for usage help).

Sure, if you would like to place a hold on this to discuss whether this is worthwhile to introduce, no issue with me.

@danielhelfand
Copy link
Contributor Author

/test pull-knative-client-integration-tests

@navidshaikh
Copy link
Collaborator

/retest

Error: ReconcileIngressFailed: Ingress reconciliation failed

@rhuss
Copy link
Contributor

rhuss commented Jun 30, 2020 •
edited
Loading

@danielhelfand thanks ! I hope to get the plugin help message for 0.16.0 which will be released in two weeks. So let's hold the PR for a moment and resolve the issue while adding plugins to the --help message.

@danielhelfand
Copy link
Contributor Author

@danielhelfand thanks ! I hope to get the plugin help message for 0.16.0 which will be released in two weaks. So let's hold the PR for a moment and resolve the issue while adding plugins to the --help message.

/hold

No worries! Sounds good.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2020
rhuss added a commit to rhuss/knative-client that referenced this pull request Jul 14, 2020
@rhuss
Add test and ported knative#910 over.
e157a02
@rhuss
Copy link
Contributor

rhuss commented Jul 14, 2020

@danielhelfand : Added the fix to #929 so closing this PR for now. Thanks !

@rhuss rhuss closed this Jul 14, 2020
@rhuss rhuss mentioned this pull request Jul 14, 2020
Merged
knative-prow-robot pushed a commit that referenced this pull request Jul 14, 2020
@rhuss
feat: Add plugin listing to "kn --help" (#929)
490fe57 
* feat: Add plugin listing to "kn --help"

This works on all levels. Test needs to be expanded still.

Fixes #266

* chore: Fix test

* Add test and ported #910 over.

* changelog update

* fix test

* Fix test

* fix integration tests

* fix test

* chore: Add some explanatory comments

* fix test
dsimansk pushed a commit to dsimansk/client that referenced this pull request Dec 21, 2021
@cardil
[0.26] Use sender image promoted by OpenShift CI (knative#910)
cf0c36e 
Align the default sender image to one promoted by OpenShift CI.

This aligns defaults with openshift-knative/serverless-operator#1337

Further refs.:

 - openshift/release#24476
 - openshift/release#24549
dsimansk pushed a commit to dsimansk/client that referenced this pull request Feb 23, 2022
@cardil
Embed kn-event for release-1.0 (at af13ecf492aa) (knative#976)
e9dc7d2 
* Allow overriding of build.sh for downstream projects (knative#1544)

* Adding override script for kn-plugin-event (knative#910)

* Embed event plugin (knative#905)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@rhuss rhuss rhuss requested changes

@maximilien maximilien maximilien approved these changes

@sixolet sixolet Awaiting requested review from sixolet

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Plugin handling if it retuns with non-zero exit status
7 participants
@danielhelfand @knative-prow-robot @knative-metrics-robot @navidshaikh @rhuss @maximilien @googlebot