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

Bool flags in the paired --foo and --no-foo format #346

Merged
merged 7 commits into from
Aug 14, 2019
Merged

Bool flags in the paired --foo and --no-foo format #346

merged 7 commits into from
Aug 14, 2019

Conversation

sixolet
Copy link
Contributor

@sixolet sixolet commented Aug 8, 2019

Adds a library to add and understand paired boolean flags in the format we agreed upon by voting. Uses it for --log-http, for now.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 8, 2019
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 8, 2019
navidshaikh
navidshaikh reviewed Aug 8, 2019
View reviewed changes
pkg/kn/flags/bool.go Outdated Show resolved Hide resolved
rhuss
rhuss reviewed Aug 8, 2019
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.

very neat idea and very useful. I have some comments though and wonder whether we should allow values to these flags add all or just check for their existence (what I would suggest).

pkg/kn/flags/bool.go Outdated Show resolved Hide resolved
pkg/kn/flags/bool_test.go Show resolved Hide resolved
pkg/kn/flags/bool.go Show resolved Hide resolved
maximilien
maximilien approved these changes Aug 8, 2019
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.

I like this! Please address the other comments and would lgtm

@maximilien
Copy link
Contributor

/test pull-knative-client-integration-tests

@sixolet
Copy link
Contributor Author

sixolet commented Aug 8, 2019

@maximilien I think I did address @rhuss's comments, though unfortunately by pointing to an issue I opened upstream. I don't think I can do better than that RN.

@sixolet
Copy link
Contributor Author

sixolet commented Aug 8, 2019

/retest

@rhuss
Copy link
Contributor

rhuss commented Aug 9, 2019

I'm fine, but maybe we could just check for the existence of a flag via flag.Lookup() instead of evaluating its value? (see #346 (comment))

rhuss
rhuss requested changes Aug 10, 2019
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.

looks good. some minor nits though, feel free to fix or not. would merge then on Monday in any case.

pkg/kn/flags/bool.go Show resolved Hide resolved
pkg/kn/flags/bool.go Show resolved Hide resolved
pkg/kn/flags/bool.go Outdated Show resolved Hide resolved
pkg/kn/flags/bool_test.go Outdated Show resolved Hide resolved
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/kn/core/root.go 59.8% 59.2% -0.6
pkg/kn/flags/bool.go Do not exist 87.5%

@sixolet
Copy link
Contributor Author

sixolet commented Aug 13, 2019

/retest

@duglin
Copy link
Contributor

duglin commented Aug 14, 2019

overall it looks good - just a few minor comments

navidshaikh
navidshaikh requested changes Aug 14, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

This is good, 👍 There are two suggestions

  • error message string to return the actual value provided by user than defaulting to false
  • removing always passing if condition

pkg/kn/flags/bool.go Show resolved Hide resolved
pkg/kn/flags/bool.go Show resolved Hide resolved
pkg/kn/flags/bool.go Show resolved Hide resolved
pkg/kn/flags/bool.go Show resolved Hide resolved
navidshaikh
navidshaikh reviewed Aug 14, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

Just one suggestion, mark it as resolved otherwise.

pkg/kn/flags/bool.go Show resolved Hide resolved
navidshaikh
navidshaikh approved these changes Aug 14, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh, sixolet

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 [maximilien,navidshaikh,sixolet]

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 merged commit 17df8c0 into knative:master Aug 14, 2019
@astefanutti astefanutti mentioned this pull request Sep 24, 2019
Merged
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
@tcnghia @knative-prow-robot
Allow cluster size parameters to be set by caller. (knative#346)
8b4694b 
* Allow cluster size parameters to be set by caller.

* Address PR comments.

* Address PR feedback.

* Fix comment.
@otaviof otaviof mentioned this pull request May 26, 2022
Merged
@wagoodman wagoodman mentioned this pull request May 30, 2023
Merged
dsimansk added a commit to dsimansk/client that referenced this pull request Feb 12, 2024
@dsimansk
[release-v1.11] Add kn-plugin-workflow v0.33.0 (but from main) (knati…
32f6124 
…ve#346)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duglin duglin duglin left review comments

@maximilien maximilien maximilien approved these changes

@rhuss rhuss rhuss requested changes

@navidshaikh navidshaikh navidshaikh approved these changes

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@navidshaikh navidshaikh

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. lgtm Indicates that a PR is ready to be merged. 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.
8 participants
@sixolet @maximilien @rhuss @knative-metrics-robot @duglin @knative-prow-robot @navidshaikh @googlebot