[kn-curl] with support for namespace and better help by maximilien · Pull Request #52 · knative/client-contrib

maximilien · 2020-06-18T23:33:04Z

kn-curl second attempt

supports namespace
better help usage message matching kn style
supports -h and --help

Supports getting INGRESS with Istio. Needs to following:

needs to be updated to support other ingress types.
needs at least an integration test and run in new vendor-less fashion.

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

navidshaikh

nice!

maximilien · 2020-06-25T22:47:42Z

Thanks for review @navidshaikh 🙏🏽

Will address comments today. Best.

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

navidshaikh

/lgtm
/approve

/hold
for @daisy-ycguo @rhuss to take a look

knative-prow-robot · 2020-06-26T05:19:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

rhuss · 2020-06-26T08:31:31Z

@@ -0,0 +1,77 @@
+#!/bin/bash


maybe we could restrict to a simple bourne shell to make it the plugin more conservative and allow it to run on more platforms (e.g. also on Alpine which only has ash by default) ?

This would require to avoid bash specific extensions, but since this plugin is quite simple I think this should be no issue.

Did you really mean ash?

@maximilien : /bin/sh

maximilien · 2020-06-29T20:19:20Z

Great feedback @rhuss and @navidshaikh. Addressing. Thank you 🙏

knative-prow-robot · 2020-06-29T23:13:59Z

New changes are detected. LGTM label has been removed.

maximilien · 2020-06-29T23:14:03Z

OK latest update addresses all comments except for the two TODOs I had: 1) e2e tests (almost done) and 2) other ingress, have not started this...

mattmoor

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

maximilien · 2020-07-11T00:18:47Z

Added checks for istio and first e2e tests. Need to address some of auto the linting feedback

maximilien · 2020-08-05T23:15:56Z

/hold remove

maximilien · 2020-08-19T23:02:15Z

/unhold

maximilien · 2020-09-09T21:02:23Z

@navidshaikh can I get a final review and LGTM on this? Thanks

navidshaikh · 2020-10-20T06:31:54Z

@@ -0,0 +1,88 @@
+#!/bin/bash


Suggested change

#!/bin/bash

#!/bin/sh

navidshaikh · 2020-10-20T06:32:15Z

@@ -0,0 +1,77 @@
+#!/bin/bash


@maximilien : /bin/sh

navidshaikh · 2020-10-20T06:34:27Z

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+USAGE=$"Usage:


not sure why $ is prefixed here

navidshaikh · 2020-10-20T06:35:45Z

+# Need at least the service name
+if [ -z "$1" ]; then
+	echo "$USAGE"
+	exit 0
+fi
+
+# If -h or --help is first argument
+if [ "$1" == "-h" ] || [ "$1" == "--help" ]; then
+  echo "$USAGE"
+  exit 0
+fi


just combine the if checks then it'd print USAGE in all the three cases ?

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 18, 2020

knative-prow-robot requested review from evankanderson and sixolet June 18, 2020 23:33

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 Jun 18, 2020

mattmoor reviewed Jun 18, 2020

View reviewed changes

maximilien force-pushed the kn-curl2 branch from ae8fd51 to 291c72a Compare June 18, 2020 23:40

mattmoor reviewed Jun 18, 2020

View reviewed changes

maximilien force-pushed the kn-curl2 branch from 291c72a to 0e98137 Compare June 18, 2020 23:41

mattmoor reviewed Jun 18, 2020

View reviewed changes

kn-curl with support for namespace and better help

70ed35a

maximilien force-pushed the kn-curl2 branch from 0e98137 to 70ed35a Compare June 18, 2020 23:43

navidshaikh reviewed Jun 25, 2020

View reviewed changes

addressed review comments

7ef6e18

maximilien force-pushed the kn-curl2 branch from f7b9549 to 7ef6e18 Compare June 26, 2020 02:39

mattmoor reviewed Jun 26, 2020

View reviewed changes

navidshaikh approved these changes Jun 26, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2020

knative-prow-robot assigned navidshaikh Jun 26, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020

rhuss suggested changes Jun 26, 2020

View reviewed changes

addressed @rhuss and @navidshaik comments

07ad07f

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2020

maximilien added 2 commits July 10, 2020 17:06

added istio ingress check

1c3ce95

first version of e2e tests

35f40aa

mattmoor reviewed Jul 11, 2020

View reviewed changes

Comment thread plugins/kn-curl/README.md Outdated

Comment thread plugins/kn-curl/README.md Outdated

mattmoor reviewed Jul 11, 2020

View reviewed changes

Comment thread plugins/kn-curl/README.md Outdated

Comment thread plugins/kn-curl/README.md Outdated

removed extra space in README.md

7554aa4

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2020

navidshaikh reviewed Oct 20, 2020

View reviewed changes

Base automatically changed from master to main March 8, 2021 17:37

Conversation

maximilien commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maximilien commented Jun 25, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jun 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhuss Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

maximilien Jul 11, 2020

Choose a reason for hiding this comment

Uh oh!

navidshaikh Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maximilien commented Jun 29, 2020

Uh oh!

knative-prow-robot commented Jun 29, 2020

Uh oh!

maximilien commented Jun 29, 2020

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maximilien commented Jul 11, 2020

Uh oh!

maximilien commented Aug 5, 2020

Uh oh!

maximilien commented Aug 19, 2020

Uh oh!

maximilien commented Sep 9, 2020

Uh oh!

navidshaikh Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

navidshaikh Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

navidshaikh Oct 20, 2020

Choose a reason for hiding this comment

maximilien commented Jun 18, 2020 •

edited

Loading