Move subscription management to new subscription subcommand#775
Move subscription management to new subscription subcommand#775fbiville wants to merge 4 commits into
Conversation
|
We may not need to split into that many commits. |
|
Also, shall we wait for knative/eventing#421 to be merged before merging this? |
glyn
left a comment
glyn
left a comment
There was a problem hiding this comment.
Some minor comments and some flag renaming which came out of the sync call on 18th September.
|
|
||
| err := createCommand.Execute() | ||
|
|
||
| Expect(err.Error()).To(HavePrefix("a DNS-1123 subdomain must consist")) |
There was a problem hiding this comment.
The approach taken elsewhere is:
Expect(err).To(MatchError(HavePrefix("...")))
| func defineFlags(command *Command, options *core.CreateSubscriptionOptions) { | ||
| flags := command.Flags() | ||
| flags.StringVarP(&options.Subscriber, "processor", "s", "", "the subscriber registered in the subscription") | ||
| flags.StringVarP(&options.Channel, "from", "i", "", "the input channel the service binds to") |
There was a problem hiding this comment.
I believe the consensus in the team is to change this flag to --channel with abbreviation -c.
| flags := command.Flags() | ||
| flags.StringVarP(&options.Subscriber, "processor", "s", "", "the subscriber registered in the subscription") | ||
| flags.StringVarP(&options.Channel, "from", "i", "", "the input channel the service binds to") | ||
| flags.StringVarP(&options.ReplyTo, "to", "o", "", "the optional output channel the service binds to") |
There was a problem hiding this comment.
I believe the consensus in the team is to change this flag to --reply-to with abbreviation -r.
|
|
||
| func defineFlags(command *Command, options *core.CreateSubscriptionOptions) { | ||
| flags := command.Flags() | ||
| flags.StringVarP(&options.Subscriber, "processor", "s", "", "the subscriber registered in the subscription") |
There was a problem hiding this comment.
I believe the consensus in the team is to change this flag to --subscriber with abbreviation -s.
| Expect(stdout.String()).To(Equal("create completed successfully\n")) | ||
| }) | ||
|
|
||
| It("should create the subscription in the output provided namespace", func() { |
There was a problem hiding this comment.
Should this say "... in the provided namespace"?
| Channel: "coco-chanel", | ||
| }).Return(nil, nil) | ||
|
|
||
| err := createCommand.Execute() |
There was a problem hiding this comment.
Note that some other tests put the equivalent of this line of code in a JustBeforeEach rather than repeating it in each test. Take a look at this example and see what you think. This is just a suggestion for a change as these tests are pretty readable as they stand.
| Expect(stdout.String()).To(Equal("create completed successfully\n")) | ||
| }) | ||
|
|
||
| It("should propagate the client error", func() { |
There was a problem hiding this comment.
Should this say "should propagate a client error" as there won't necessarily be an error.
|
Applied the requested changes and merged as 515f26a |
|
Thanks a lot @glyn! |
2 participants
Fixes #771.