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

feat: Add aliases to commands #1041

Merged
merged 1 commit into from
Oct 6, 2020
Merged

feat: Add aliases to commands #1041

merged 1 commit into from
Oct 6, 2020

Conversation

arghya88
Copy link
Contributor

@arghya88 arghya88 commented Oct 4, 2020

Description

Adds aliases to commands

Reference

Fixes #940

/lint

knative-prow-robot
knative-prow-robot reviewed Oct 4, 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.

@arghya88: 0 warnings.

In response to this:

Description

Adds aliases to commands

Reference

Fixes #940

/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 knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 4, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 4, 2020
@arghya88 arghya88 force-pushed the alias branch 2 times, most recently from 92f24b5 to 837efda Compare October 4, 2020 15:25
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 4, 2020

/retest

2 similar comments
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 4, 2020

/retest

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 5, 2020

/retest

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 5, 2020

/retest

Ingress reconciliation failed

@rhuss
Copy link
Contributor

rhuss commented Oct 5, 2020

/retest

rhuss
rhuss requested changes Oct 5, 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 ! Looks good, but could you please add some tests ? This shouldn't be hard by either copying already existing tests or modifying some of the existing tests to use an alias instead of the original command.

I'd suggest to add additional test for the unit tests (possible wrapping existing tests into a loop). This does not have to be done every, some tests are fine (as I think we can confident if the alias resolution worked once that it then works everywhere)

For the integration tests please modify some of the existing tests to use aliases, too, so that you have a mix of tests. The reason why I would like to avoid to add additional e2e tests is, that this would add to the overall time for the e2e test suite, which I think is not needed for just testing that aliases are resolved properly.

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 5, 2020

/retest

maximilien
maximilien suggested changes Oct 5, 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.

Thanks for contribution. An integration tests that mixes aliases and non-alias commands would be nice.

docs/cmd/kn_broker.md Outdated Show resolved Hide resolved
@maximilien
Copy link
Contributor

/approve
/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 5, 2020
@arghya88 arghya88 force-pushed the alias branch 3 times, most recently from ee2a66e to 798bd02 Compare October 6, 2020 04:09
navidshaikh
navidshaikh requested changes Oct 6, 2020
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1 to the point raised by @danielhelfand , but can be done in a subsequent PR once we know what preventing cobra to document that. Lets get this in today (release day), just a minor nit to follow the alias hint for command group as well.

pkg/kn/commands/service/service.go Outdated Show resolved Hide resolved
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 6, 2020

/retest

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 6, 2020

/retest

@rhuss
Copy link
Contributor

rhuss commented Oct 6, 2020

+1 for adding this now and adding the testing + doc tuning after this PR got merged

@arghya88
Copy link
Contributor Author

arghya88 commented Oct 6, 2020

/retest

1 similar comment
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 6, 2020

/retest

@arghya88 arghya88 requested a review from navidshaikh October 6, 2020 09:29
@arghya88
Copy link
Contributor Author

arghya88 commented Oct 6, 2020

@navidshaikh tests passed finally..PTAL

@navidshaikh navidshaikh mentioned this pull request Oct 6, 2020
Closed
@navidshaikh navidshaikh changed the title add alias to commands feat: Add alias to commands Oct 6, 2020
@navidshaikh navidshaikh changed the title feat: Add alias to commands feat: Add aliases to commands Oct 6, 2020
navidshaikh
navidshaikh approved these changes Oct 6, 2020
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
/approve

thanks @arghya88 !
The CI flakes should be solved in upcoming patch release.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arghya88, maximilien, 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:
  • OWNERS [maximilien,navidshaikh]

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

@navidshaikh
Copy link
Collaborator

logged #1045 to track the auto doc for aliases
thanks @danielhelfand

@knative-prow-robot knative-prow-robot merged commit 5025137 into knative:master Oct 6, 2020
rhuss pushed a commit to rhuss/knative-client that referenced this pull request Oct 12, 2020
@arghya88 @rhuss
add alias to commands (knative#1041)
ea82f38
@arghya88 arghya88 deleted the alias branch November 28, 2020 07:17
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

@danielhelfand danielhelfand danielhelfand left review comments

@rhuss rhuss rhuss requested changes

@maximilien maximilien maximilien requested changes

@navidshaikh navidshaikh navidshaikh approved these changes

@cppforlife cppforlife Awaiting requested review from cppforlife

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. 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.

Proposal: Add aliases to commands
7 participants
@arghya88 @rhuss @maximilien @knative-prow-robot @navidshaikh @danielhelfand @googlebot