-
Notifications
You must be signed in to change notification settings - Fork 263
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
kn source-list types: adding column to specify built-in source #1246
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Welcome @eletonia! It looks like this is your first PR to knative/client 🎉 |
Hi @eletonia. 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 Once the patch is verified, the new status will be reflected by the 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. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
1 similar comment
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@eletonia thanks for the contribution! The change looks good at a first glance. However, google-cla bot is confused due to mix of 2 accounts adding commits. It'd better if you could squash all those commits into one under your preferred account. You can check how to fill the licence agreement for the future contributions. /ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/assign @rhuss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. The question is whether this could be done dynamically since as new sources are added and introduced you’ll need to change your hard coded list. Perhaps having a different issue to “upgrade” to that in future could allow this to be merged now and the enhancement done in future. WDYT?
Details for
In a future issue with plugin support, we can dynamically display the description and source type if it is a plugin as suggested in the comments of this code |
Thanks for your contribution, couple of things,
|
Well, that'd easily imply that such a plugin actually exists to support the source, but it's not necessary true for a lot of eventing sources. |
Thanks for the PR, looks good in general, but I would be denser to save screen real estate. Also, I think that for the user it is only important whether a source is supported by the client or not, regardless of whether it is built-in or supported via a plugin. E.g. instead of YES / NO, just use an "X" (and nothing if not builtin), and instead of "BUILT-IN" header, maybe just an "S" letter (or something else), or even no header. Then, I would put that in the second or third column, before the description because this column is of fixed length (ideally only 1 char), while a description can be long and varying in length. In general, the longest possible column should be last because this can be the only one that possible needs not to be truncated when being longer than a certain limit. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@eletonia at least we know it's not my "consent". :) |
Thanks @dsimansk There are no other authors that need to consent. It may have to be manually confirmed:
|
/retest |
@itsmurugappan could you please try to comment with @eletonia I believe for the manual confirmation it needs to be also Googler. It might be quicker to just create a fresh PR. @rhuss any suggestion would be welcome pls? |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
# This is the 1st commit message: adding BUILT-IN SOURCE column for kn source list-types # The commit message knative#2 will be skipped: # changing list test to check for BUILT-IN SOURCE column # The commit message knative#3 will be skipped: # changing e2e source list test to check for BUILT-IN SOURCE column # The commit message knative#4 will be skipped: # adding CHANGELOG entry # The commit message knative#5 will be skipped: # kn source list-types: changing BUILT-IN SOURCE to BUILT-IN and moving DESCRIPTION column to the end # The commit message knative#6 will be skipped: # changing BUILT-IN SOURCE to BUILT-IN in changelog # The commit message knative#7 will be skipped: # Update CHANGELOG.adoc # # Co-authored-by: David Simansky <[email protected]> # The commit message knative#8 will be skipped: # kn source list-types: changing column header to S, values to X, and moving to second column # The commit message knative#9 will be skipped: # fixing CHANGELOG merge conflict
/retest |
1 similar comment
/retest |
David meets all criteria for an approver: - [x] Reviewer for at least 3 months - [x] Primary reviewer for at least 10 substantial PRs to the codebase, e.g. * knative#1246 * knative#1194 * knative#738 * knative#832 * knative#1016 * knative#877 * knative#667 * knative#697 * knative#1212 * knative#835 - [x] Reviewed at least 30 PRs to the codebase ([38 assigned PRs](https://github.com/knative/client/pulls?q=type%3Apr+assignee%3Adsimansk+)) - [x] Nominated by a WG lead (rhuss) Congrats David ! Thanks a lot for your awesome work & contributions.
David meets all criteria for an approver: - [x] Reviewer for at least 3 months - [x] Primary reviewer for at least 10 substantial PRs to the codebase, e.g. * knative#1246 * knative#1194 * knative#738 * knative#832 * knative#1016 * knative#877 * knative#667 * knative#697 * knative#1212 * knative#835 - [x] Reviewed at least 30 PRs to the codebase ([38 assigned PRs](https://github.com/knative/client/pulls?q=type%3Apr+assignee%3Adsimansk+)) - [x] Nominated by a WG lead (rhuss) Congrats David ! Thanks a lot for your awesome work & contributions.
David meets all criteria for an approver: - [x] Reviewer for at least 3 months - [x] Primary reviewer for at least 10 substantial PRs to the codebase, e.g. * #1246 * #1194 * #738 * #832 * #1016 * #877 * #667 * #697 * #1212 * #835 - [x] Reviewed at least 30 PRs to the codebase ([38 assigned PRs](https://github.com/knative/client/pulls?q=type%3Apr+assignee%3Adsimansk+)) - [x] Nominated by a WG lead (rhuss) Congrats David ! Thanks a lot for your awesome work & contributions.
Thanks @eletonia! I think it's all good now. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
Description
kn source list-types
currently shows theTYPE
,NAME
, andDESCRIPTION
of available sources.This PR adds a
S
column which shows whether the source is built-in or not:Changes
human_readable_flags.go
to add the newS
columnlist_test.go
now checks forS
column and appropriate valuessource_list_test.go
e2e test now checks forS
columnReference
Fixes a part of #633 More discussion is needed on how to handle plugin sources.