-
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
feature: --no-headers flag for resource listing #262
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
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.
@komaldhull: 0 warnings.
In response to this:
Adds support for a "--no-headers" flag for resource listing commands that prints the default table output with column headers removed
Fixes #233
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.
Hi @komaldhull. 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. |
/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.
Thanks ! will give a review on Monday, but could you please remove the .DS_Store
file that you accidentally checked in ? (and please squash then to remove it from any commit)
@komaldhull : Also you would need to sign the google CLA. Thanks ! |
/retest |
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.
looks good with some nit-pick.
Now checking why the test failed.
/retest There is yet another flaky test introduced which checks for a revision to be ready too fast. |
@komaldhull could you squash you commits to a single commit ? It looks like that one of your current commits uses an email that is not CLAed. If you squash with your current account which has signed an CLA, and force push, then the CLA check should be happy. |
Hmm, CLA check still failing ;-( Can you please check that you did the steps in #262 (comment) ? Probably as an corporate signer under IBM ? If this is fixed, we are good to merge. |
looks like my request to be added as an IBM contributor is still pending :( just followed up so I should be added soon, I'll comment when I hear back. |
@rhuss I should be added to the CLA now! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
ping @komaldhull Can you update PR following recent changes to bool flags as referenced above? |
@navidshaikh my apologies for not seeing this earlier! could you please clarify what requires changing to meet the conventions? as far as I can tell, this PR matches the documentation (the default behavior is to print headers, so adding the flag "--no-headers" disables the headers), sorry if I'm missing something. |
@komaldhull : I think the current flag |
5fae74d
to
59d1a62
Compare
The following is the coverage report on pkg/.
|
/hold cancel |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: komaldhull, navidshaikh 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 |
…#262) They're not available on these branches and will always fail.
Adds support for a "--no-headers" flag for resource listing commands that prints the default table output with column headers removed
Fixes #233
Release notes