-
Notifications
You must be signed in to change notification settings - Fork 263
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
Uniform multiple writeSink(s) to DescribeSink #1075
Conversation
pkg/kn/commands/flags/sink.go
Outdated
|
||
// WriteSink writes a sink to a PrefixWriter which is used in describe command | ||
func WriteSink(dw printers.PrefixWriter, sink *duckv1.Destination) { | ||
subWriter := dw.WriteAttribute("Sink", "") | ||
ref := sink.Ref | ||
if ref != nil { | ||
subWriter.WriteAttribute("Name", sink.Ref.Name) | ||
subWriter.WriteAttribute("Namespace", sink.Ref.Namespace) | ||
subWriter.WriteAttribute("Resource", fmt.Sprintf("%s (%s)", sink.Ref.Kind, sink.Ref.APIVersion)) | ||
} | ||
uri := sink.URI | ||
if uri != nil { | ||
subWriter.WriteAttribute("URI", uri.String()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while doing the subscription CRUD, separated this util here
client/lib/printing/describe.go
Line 28 in abb75e4
func DescribeSink(dw printers.PrefixWriter, attribute, namespace string, sink *duckv1.Destination) { |
lets refer this util at following places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the information. I updated. Please review again.
36c1be6
to
e54a9f4
Compare
52e7d1c
to
a0b401b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, with a minor change request.
lib/printing/describe.go
Outdated
@@ -29,6 +29,9 @@ func DescribeSink(dw printers.PrefixWriter, attribute, namespace string, sink *d | |||
if sink == nil { | |||
return | |||
} | |||
if len(attribute) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that this method is never called with an attribute
that is different to "". So I just would remove that argument and maybe re-introduce it if there is ever a need for a different label than Sink
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhuss : subscription CRUD uses it for DeadLetterSink etc
printing.DescribeSink(dw, "Subscriber", subscription.Namespace, subscription.Spec.Subscriber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daisy-ycguo : can you please update the function docstring to mention this behavior ? (like 'Sink' is used for attribute value if empty string is provided
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. I will update the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But maybe it's just better to require an explicit argument ? This could then also serve as documentation when calling the function (so just changing "" everywhere to "Sink") ? Not sure about this, so feel free to keep it .
Yet an alternative would be to move attribute
to the end of the argument list and make it an optional parameter like attribute ...string
. But I'm not dogmatic here, feel to chose what you think fits best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe it's just better to require an explicit argument ? This could then also serve as documentation when calling the function (so just changing "" everywhere to "Sink") ? Not sure about this, so feel free to keep it .
I was thinking same about it when I first saw this. As we are exporting this util, IMO we shouldn't carry any assumption and require explicit attribute value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the code to make it mandatory.
a0b401b
to
89a607e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
89a607e
to
a75a7b7
Compare
a75a7b7
to
4f51722
Compare
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: daisy-ycguo, 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:
Approvers can indicate their approval by writing |
Description
There are multiple
writeSink
in source commands to print Sink object. And there is a utilityDescribeSink
inlib/printing
. It's better to uniform them.Changes