update mw client doc and build cli#487
Conversation
Signed-off-by: Wei Liu <liuweixa@redhat.com>
WalkthroughThe changes introduce a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/manifestwork/README.md (1)
265-278:⚠️ Potential issue | 🟡 MinorRemove shell prompt
$to satisfy markdownlint MD014.The lint warning is triggered by
$without output; consider removing the prompt marker for command-only blocks.Example fix
- $ cat << EOF | kubectl apply -f - + cat << EOF | kubectl apply -f -
🤖 Fix all issues with AI agents
In `@examples/manifestwork/client.go`:
- Around line 87-99: The custom argument parsing loop uses
flag.CommandLine.Lookup() before klog flags are registered, so user klog flags
(e.g. -v 4) get mis-parsed; move the klog flag initialization call
(klog.InitFlags(nil)) and any check for existing klog flags
(flag.CommandLine.Lookup("alsologtostderr")) to run before the custom argument
parsing loop, ensuring all klog flags are discoverable, then proceed to parse
otherArgs and apply the verbose handling (flag.Set("v", "...") /
klog.SetLogger(logr.Discard())) as currently implemented.
🧹 Nitpick comments (2)
Makefile (1)
192-194: Addmaestro-clitocleanto avoid stale binaries.The new target produces
./maestro-cli, butcleandoesn’t remove it. Consider cleaning it alongside other build artifacts.Proposed update to clean target
clean: rm -rf \ $(binary) \ + maestro-cli \ data/generated/openapi/*.json \examples/manifestwork/client.go (1)
175-201: Keep usage strings consistent across the CLI.Since
printUsagenow advertisesmaestro-cli, consider updating thelog.Fatalf("usage: client.go ...")strings to match.Example consistency update (outside this block)
- log.Fatalf("usage: client.go get <work-name>") + log.Fatalf("usage: maestro-cli get <work-name>")
| // check if the klog flag is already registered to avoid duplicate flag define error | ||
| if flag.CommandLine.Lookup("alsologtostderr") == nil { | ||
| klog.InitFlags(nil) | ||
| } | ||
| flag.CommandLine.Parse(otherArgs) | ||
|
|
||
| // Configure klog based on verbose flag | ||
| if *verbose { | ||
| flag.Set("v", "4") | ||
| } else { | ||
| // Completely disable klog output by setting a discard logger | ||
| klog.SetLogger(logr.Discard()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "client.go" -path "*/manifestwork/*"Repository: openshift-online/maestro
Length of output: 101
🏁 Script executed:
cat -n examples/manifestwork/client.go | head -120Repository: openshift-online/maestro
Length of output: 4931
🏁 Script executed:
cat > /tmp/test_flags.go << 'EOF'
package main
import (
"flag"
"fmt"
"k8s.io/klog/v2"
)
func main() {
// Before registering klog flags
fmt.Println("Before klog.InitFlags:")
v := flag.CommandLine.Lookup("v")
fmt.Printf(" flag.Lookup(\"v\"): %v\n", v)
// Register klog flags
klog.InitFlags(nil)
// After registering klog flags
fmt.Println("After klog.InitFlags:")
v = flag.CommandLine.Lookup("v")
fmt.Printf(" flag.Lookup(\"v\"): %v\n", v)
}
EOFRepository: openshift-online/maestro
Length of output: 50
🏁 Script executed:
cat > /tmp/analyze_parsing.py << 'EOF'
# Simulate the custom argument parsing logic
import sys
def simulate_parsing(os_args):
"""Simulate the custom parsing loop from the code"""
command = ""
commandArgs = []
otherArgs = []
# Simulate registered flags (before klog.InitFlags)
registered_flags = {
"source", "maestro-server", "grpc-server", "grpc-server-ca-file",
"grpc-client-cert-file", "grpc-client-key-file", "grpc-client-token-file",
"consumer-name", "server-healthiness-timeout", "print-work-details",
"insecure-skip-verify", "verbose"
}
i = 0
while i < len(os_args):
arg = os_args[i]
if arg.startswith("-"):
# Keep the flag token itself
otherArgs.append(arg)
# Check if flag has a value
if "=" not in arg and i + 1 < len(os_args) and not os_args[i + 1].startswith("-"):
name = arg.lstrip("-")
# Simulate flag.CommandLine.Lookup
if name in registered_flags:
# For simplicity, assume all are not bool flags (except print-work-details, verbose, insecure-skip-verify)
bool_flags = {"print-work-details", "verbose", "insecure-skip-verify"}
if name not in bool_flags:
i += 1
otherArgs.append(os_args[i])
# If flag NOT in registered_flags, don't consume next arg
else:
if command == "":
command = arg
else:
commandArgs.append(arg)
i += 1
return command, commandArgs, otherArgs
# Test with "-v 4" (klog flag, not in registered_flags)
print("Test 1: User passes '-v 4' (klog flag not yet registered)")
os_args = ["prog", "-v", "4", "watch"]
cmd, cmdargs, otherargs = simulate_parsing(os_args)
print(f" os.Args: {os_args}")
print(f" command: {cmd}")
print(f" commandArgs: {cmdargs}")
print(f" otherArgs: {otherargs}")
print(f" Problem: '4' was treated as command instead of value for '-v'!")
print()
# Test with "-verbose" (custom flag, registered)
print("Test 2: User passes '-verbose true' (custom flag, registered)")
os_args = ["prog", "-verbose", "true", "watch"]
cmd, cmdargs, otherargs = simulate_parsing(os_args)
print(f" os.Args: {os_args}")
print(f" command: {cmd}")
print(f" commandArgs: {cmdargs}")
print(f" otherArgs: {otherargs}")
print(f" Correct: '-verbose' and 'true' both in otherArgs")
EOF
python /tmp/analyze_parsing.pyRepository: openshift-online/maestro
Length of output: 534
🏁 Script executed:
python /tmp/analyze_parsing.pyRepository: openshift-online/maestro
Length of output: 534
Move klog flag initialization before the custom argument parsing loop.
The custom argument parsing uses flag.CommandLine.Lookup() to detect whether a flag takes a value. Since klog flags are not registered until line 89, user-provided klog flags like -v 4 will be incorrectly parsed—the value 4 will be treated as a command instead of the flag's argument. Initialize klog flags before the parsing loop to ensure all flags are discoverable.
Suggested adjustment
func main() {
+ // Register klog flags before custom parsing
+ if flag.CommandLine.Lookup("alsologtostderr") == nil {
+ klog.InitFlags(nil)
+ }
+
// Custom argument parsing to allow flags anywhere
var command string
var commandArgs []string
var otherArgs []string
@@
- // Parse flags
- // check if the klog flag is already registered to avoid duplicate flag define error
- if flag.CommandLine.Lookup("alsologtostderr") == nil {
- klog.InitFlags(nil)
- }
+ // Parse flags
flag.CommandLine.Parse(otherArgs)🤖 Prompt for AI Agents
In `@examples/manifestwork/client.go` around lines 87 - 99, The custom argument
parsing loop uses flag.CommandLine.Lookup() before klog flags are registered, so
user klog flags (e.g. -v 4) get mis-parsed; move the klog flag initialization
call (klog.InitFlags(nil)) and any check for existing klog flags
(flag.CommandLine.Lookup("alsologtostderr")) to run before the custom argument
parsing loop, ensuring all klog flags are discoverable, then proceed to parse
otherArgs and apply the verbose handling (flag.Set("v", "...") /
klog.SetLogger(logr.Discard())) as currently implemented.
|
/cc @clyang82 |
| .PHONY: binary | ||
|
|
||
| maestro-cli: | ||
| ${GO} build $(BUILD_OPTS) -o maestro-cli ./examples/manifestwork/client.go |
There was a problem hiding this comment.
can we support cloudevents as well?
There was a problem hiding this comment.
there may be some gap in get/list/watch if we support both, I think we can think this later
There was a problem hiding this comment.
maybe a unify client for resources
2 participants
No description provided.