Add -a flag as alias for --annotation #782

duglin · 2020-04-06T20:10:14Z

I find that I'm using annotations more and more and since -a
wasn't being used I decided we should save people from typing too much

Signed-off-by: Doug Davis [email protected]

Changes

add support for -a as an alias for --annotation

/lint

knative-prow-robot

@duglin: 0 warnings.

In response to this:

I find that I'm using annotations more and more and since -a
wasn't being used I decided we should save people from typing too much

Signed-off-by: Doug Davis [email protected]

Changes

add support for -a as an alias for --annotation

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

navidshaikh · 2020-04-07T07:45:55Z

We also have other flags starting with a, --async (deprecated but still there), --all-namespaces with -A alias, --args and --autoscale-window. IMO, if there are multiple flags starting with same letter, it becomes confusing to map aliases (--args flag is closest ).
Also, this depends on how popular the flag is to make a short hand alias for (as done for --label flag).

@duglin : should auto-completion help here?

duglin · 2020-04-07T11:20:09Z

Yep - it's a bit of a guessing game, and popularity contest.

I don't think it's confusing though since people learn quickly what works. -l seems to be ok and we have lots of --l... long named flags.

But, as of now, of all the --a... flags, I do think --annotation would be the most popular, so I'm just trying to make life a bit easier.

rhuss · 2020-04-07T14:49:51Z

docs/cmd/kn_service_create.md

@@ -45,7 +45,7 @@ kn service create NAME --image IMAGE [flags]
 ### Options

 ```
-      --annotation stringArray       Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
+  -a, --annotation stringArray       Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).


Im fine with that (except maybe that it can be easily confused with -A which is "for all namespaces")

on the other hand, why not -a for --arg ?

But as we already have -e and -l, for consistencies sake I'm fine with -a (but in general we should take care to give away single letter options not easily)

I think --annotation would be used more than --arg, plus "arg" is shorter than "annotation" - so between the two I don't think "arg" needs as much UX help :-)

As for -A and -a .... not sure what to say, other than these things are case sensitive for a reason.

rhuss · 2020-04-08T09:34:54Z

After no objections from the latest WG call, lets merge this PR.

Thanks, @duglin !

/lgtm
/approve

rhuss

There are some test errors, probably because of some e2e test refactorings.

Also, could you please an an entry to the CHANGELOG.adoc as well to reflect this change ?

rhuss · 2020-04-08T10:00:52Z

test/e2e/service_options_test.go

+	serviceUpdate(t, it, r, "svc3", "-a", "alpha=direwolf", "-a", "brave-")
+	validateServiceAnnotations(t, it, r, "svc3", map[string]string{"alpha": "direwolf", "brave": ""})
+	serviceDelete(t, it, r, "svc3")
+


You probably have to adapt this part to fit the refactoring landed recently (that's why the build tests are failing)

duglin · 2020-04-08T17:19:45Z

@rhuss fixed testing issues

duglin · 2020-04-09T01:03:19Z

Added an entry to the change log too - I think it's all ready now

rhuss

thanks !

/lgtm
/approve

rhuss · 2020-04-09T10:50:43Z

/lgtm

rhuss · 2020-04-09T16:40:27Z

/retest

Still looks like a flake

duglin · 2020-04-10T02:24:27Z

/hold

I'm doing some testing to see why the tests are failing

I find that I'm using annotations more and more and since `-a` wasn't being used I decided we should save people from typing too much Signed-off-by: Doug Davis <[email protected]>

duglin · 2020-04-10T10:50:00Z

/hold cancel

duglin · 2020-04-10T10:50:15Z

/retest

navidshaikh

/lgtm

knative-prow-robot · 2020-04-10T11:44:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: duglin, navidshaikh, rhuss

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~~ [navidshaikh,rhuss]

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

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

knative-prow-robot reviewed Apr 6, 2020

View reviewed changes

knative-prow-robot requested review from cppforlife and evankanderson April 6, 2020 20:10

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 6, 2020

rhuss reviewed Apr 7, 2020

View reviewed changes

knative-prow-robot assigned rhuss Apr 8, 2020

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 8, 2020

rhuss requested changes Apr 8, 2020

View reviewed changes

knative-prow-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2020

duglin force-pushed the addA branch from 6796d8d to ba0c164 Compare April 8, 2020 15:53

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

duglin force-pushed the addA branch from ba0c164 to c1ffada Compare April 9, 2020 00:28

rhuss reviewed Apr 9, 2020

View reviewed changes

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

duglin force-pushed the addA branch from 080cdcd to 39be6f4 Compare April 9, 2020 12:34

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

duglin force-pushed the addA branch from 39be6f4 to df5c135 Compare April 9, 2020 13:02

duglin force-pushed the addA branch 2 times, most recently from 5094df2 to cf39cd6 Compare April 10, 2020 02:23

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

Add -a flag as alias for --annotations

001f046

I find that I'm using annotations more and more and since `-a` wasn't being used I decided we should save people from typing too much Signed-off-by: Doug Davis <[email protected]>

duglin force-pushed the addA branch from cf39cd6 to 001f046 Compare April 10, 2020 03:16

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

navidshaikh changed the title ~~Add -a flag as alias for --annotations~~ Add -a flag as alias for --annotation Apr 10, 2020

navidshaikh approved these changes Apr 10, 2020

View reviewed changes

knative-prow-robot assigned navidshaikh Apr 10, 2020

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

knative-prow-robot merged commit 030279b into knative:master Apr 10, 2020

duglin deleted the addA branch April 10, 2020 11:55

navidshaikh added this to the v0.14.0 milestone Apr 20, 2020

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Jessie is moving, remove her from OWNERS (knative#782)

3b1236f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add -a flag as alias for --annotation #782

Add -a flag as alias for --annotation #782

duglin commented Apr 6, 2020

knative-prow-robot left a comment

navidshaikh commented Apr 7, 2020 •

edited

Loading

duglin commented Apr 7, 2020

rhuss Apr 7, 2020

rhuss Apr 7, 2020

duglin Apr 7, 2020 •

edited

Loading

rhuss commented Apr 8, 2020

rhuss left a comment

rhuss Apr 8, 2020

duglin commented Apr 8, 2020

duglin commented Apr 9, 2020

rhuss left a comment

rhuss commented Apr 9, 2020

rhuss commented Apr 9, 2020

duglin commented Apr 10, 2020

duglin commented Apr 10, 2020

duglin commented Apr 10, 2020

navidshaikh left a comment

knative-prow-robot commented Apr 10, 2020

Add -a flag as alias for --annotation #782

Add -a flag as alias for --annotation #782

Conversation

duglin commented Apr 6, 2020

Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Changes

navidshaikh commented Apr 7, 2020 • edited Loading

duglin commented Apr 7, 2020

rhuss Apr 7, 2020

Choose a reason for hiding this comment

rhuss Apr 7, 2020

Choose a reason for hiding this comment

duglin Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

rhuss commented Apr 8, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss Apr 8, 2020

Choose a reason for hiding this comment

duglin commented Apr 8, 2020

duglin commented Apr 9, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss commented Apr 9, 2020

rhuss commented Apr 9, 2020

duglin commented Apr 10, 2020

duglin commented Apr 10, 2020

duglin commented Apr 10, 2020

navidshaikh left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Apr 10, 2020

navidshaikh commented Apr 7, 2020 •

edited

Loading

duglin Apr 7, 2020 •

edited

Loading