Conversation
freeznet
requested review from
a team,
mattisonchao,
nlu90 and
zymap
as code owners
|
@freeznet:Thanks for your contribution. For this PR, do we need to update docs? |
github-actions
bot
added
the
doc-info-missing
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 60 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| timestamp, err := time.Parse(time.RFC3339Nano, datetime) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
--datetime help text says RFC3339 or RFC3339Nano, but the implementation parses using time.RFC3339Nano only. time.Parse(time.RFC3339Nano, ...) will reject RFC3339 timestamps without fractional seconds (e.g. 2021-06-28T16:53:08Z). Consider accepting both formats (try RFC3339Nano then RFC3339) or update the flag description/examples to match the supported format.
| vc.FlagSetGroup.InFlagSet("SchemaCompatibilityStrategy", func(set *pflag.FlagSet) { | ||
| set.StringVarP(&strategy, "compatibility", "c", "", "schema compatibility strategy") | ||
| }) |
There was a problem hiding this comment.
--compatibility is optional here, but utils.ParseSchemaCompatibilityStrategy(strategy) will fail (or potentially default unexpectedly) when the flag is omitted/empty. This should be treated as a required input (e.g., mark the flag required) to provide a clearer CLI contract and error message.
| vc.FlagSetGroup.InFlagSet("ReplicationClusters", func(set *pflag.FlagSet) { | ||
| set.StringVarP(&clusterIDs, "clusters", "c", "", "comma separated cluster names") | ||
| }) |
There was a problem hiding this comment.
--clusters isn't required, so running the command without it will call strings.Split("", ",") and send a slice containing an empty string to the admin API. Mark the flag required and/or validate non-empty cluster names before calling SetReplicationClusters.
| escapedParts := make([]string, len(parts)) | ||
| for i, part := range parts { | ||
| escapedParts[i] = url.PathEscape(part) |
There was a problem hiding this comment.
namespaceAdminEndpoint applies url.PathEscape to each path part. Passing ns.String() (which contains a /) will encode it as %2F, which some HTTP servers/frameworks reject or route differently (encoded slashes are often blocked). Consider building the endpoint from tenant + namespace segments (or splitting ns.String() on / and escaping each segment) so the request path matches Pulsar's expected /namespaces/{tenant}/{namespace}/... format reliably.
| escapedParts := make([]string, len(parts)) | |
| for i, part := range parts { | |
| escapedParts[i] = url.PathEscape(part) | |
| escapedParts := make([]string, 0, len(parts)) | |
| for _, part := range parts { | |
| for _, segment := range strings.Split(part, "/") { | |
| escapedParts = append(escapedParts, url.PathEscape(segment)) | |
| } |
| func TestTopicPoliciesCommands(newVerb func(cmd *cmdutils.VerbCmd), args []string) (out *bytes.Buffer, execErr, nameErr, err error) { | ||
| var execError error | ||
| cmdutils.ExecErrorHandler = func(err error) { | ||
| execError = err | ||
| } | ||
|
|
||
| var parsedNameError error | ||
| cmdutils.CheckNameArgError = func(err error) { | ||
| parsedNameError = err | ||
| } |
There was a problem hiding this comment.
This helper overwrites the global cmdutils.ExecErrorHandler and cmdutils.CheckNameArgError but never restores the previous handlers. That can leak state between tests and make failures order-dependent. Capture the old values and restore them via defer/t.Cleanup.
(If this PR fixes a github issue, please add
Fixes #<xyz>.)Fixes #
(or if this PR is one task of a github issue, please add
Master Issue: #<xyz>to link to the master issue.)Master Issue: #
Motivation
Explain here the context, and why you're making that change. What is the problem you're trying to solve.
Modifications
Describe the modifications you've done.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Documentation
Check the box below.
Need to update docs?
doc-required(If you need help on updating docs, create a doc issue)
no-need-doc(Please explain why)
doc(If this PR contains doc changes)