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.

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

✨ Provide a cli option to enable and disable completion command #1776

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Nov 3, 2020

Description

Provide a WithCompletion CLI Option that enables adds a compeltion generation command instead of having to create it and provide it as an extra command.

Changes

  • ✨ New argument type for cli.New: WithCompletion. Creating a command and providing it with WithExtraCommands should still work with the previous behavior, so this is not a breaking change, but frameworks using kubebuilder should migrate to the new approach.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 3, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 3, 2020
@Adirio Adirio changed the title Provide a cli option to enable and disable completion command ✨ Provide a cli option to enable and disable completion command Nov 3, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Nov 3, 2020

@camilamacedo86 This PR goes in the same direction as #1691 and #1773, moving another command out of the extra commands. I think that the API for frameworks with the new options is clearer, they only have to provide an argument instead of a full COBRA command. WDYT?

@Adirio
Copy link
Contributor Author

Adirio commented Nov 4, 2020

A note in coverage: this PR moves part of the remaining code from cmd to pkg. As cmd was not being checked by coveralls, it says that the coverage has decreased, but in reality we just have included in the covered package part of the code that wasn't being covered by coveralls.

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It is only missing tests.
Otherwise,
/approve

Looking for your tests for we get it merged.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Looking for the small nit fixes for we get it merged.
See the comments

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
@Adirio Adirio force-pushed the completion-cmd branch 2 times, most recently from ed1396a to 9e583cd Compare November 4, 2020 11:13
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Great work 👍

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Nov 4, 2020

Rebased and added the tests for this option to improve coverage.

@Adirio Adirio requested a review from camilamacedo86 November 4, 2020 13:58
Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0489c99 into kubernetes-sigs:master Nov 4, 2020
@Adirio Adirio deleted the completion-cmd branch November 4, 2020 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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
Development

Successfully merging this pull request may close these issues.

3 participants