Dev 1014 #2
Dev 1014 #2
Conversation
Summary of ChangesHello @bcfre, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the command-line interface for managing RoleBasedGroup resources within Kubernetes. It introduces a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by creating a new CLI tool, kubectl-rbgctl, with several commands for managing RoleBasedGroup resources. The code is well-structured, with each command in its own file. However, there are several critical issues regarding argument validation in the new Cobra commands that could lead to panics. Additionally, there are opportunities to improve error handling, reduce code duplication, and fix minor documentation formatting.
cmd/cli/cmd/diffrevision.go
Outdated
| func init() { | ||
| flags := rollbackCmd.Flags() | ||
|
|
||
| if err := viper.BindPFlags(flags); err != nil { | ||
| fmt.Println(err) | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
This init function appears to have a copy-paste error from another command, as it's referencing rollbackCmd.Flags() instead of diffrevisionCmd.Flags(). This is incorrect and could lead to unexpected behavior. It should be updated to reference its own command.
func init() {
flags := diffrevisionCmd.Flags()
if err := viper.BindPFlags(flags); err != nil {
fmt.Println(err)
os.Exit(1)
}
}
cmd/cli/cmd/diffrevision.go
Outdated
| } | ||
| var raw map[string]interface{} | ||
| if err := yaml.Unmarshal(rev.Data.Raw, &raw); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Using panic(err) for error handling in a CLI tool is not user-friendly as it will print a stack trace. It's better to return the error from extractSpec and handle it in the calling function, runDiffRevision. This would require changing the function signature to func extractSpec(...) (interface{}, error).
cmd/cli/cmd/listrevision.go
Outdated
| var listrevisionCmd = &cobra.Command{ | ||
| Use: "listrevision <rbgName>", | ||
| Short: "List all revision of an current rbg", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runListRevision(context.Background(), args[0]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
The command does not validate the number of arguments. If it's called with no arguments, it will panic due to an index-out-of-bounds error on args[0]. You should use the Args field on the cobra.Command struct to enforce that exactly one argument is provided.
| var listrevisionCmd = &cobra.Command{ | |
| Use: "listrevision <rbgName>", | |
| Short: "List all revision of an current rbg", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runListRevision(context.Background(), args[0]) | |
| }, | |
| } | |
| var listrevisionCmd = &cobra.Command{ | |
| Use: "listrevision <rbgName>", | |
| Short: "List all revision of an current rbg", | |
| Args: cobra.ExactArgs(1), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runListRevision(cmd.Context(), args[0]) | |
| }, | |
| } |
cmd/cli/cmd/rollback.go
Outdated
| var rollbackCmd = &cobra.Command{ | ||
| Use: "rollback <rbgName> <revisionName>", | ||
| Short: "Rollback current RBG to target ControllerRevision", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runRollback(context.Background(), args[0], args[1]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
The command does not validate the number of arguments. If it's called with fewer than two arguments, it will panic due to an index-out-of-bounds error. You should use the Args field on the cobra.Command struct to enforce that exactly two arguments are provided.
| var rollbackCmd = &cobra.Command{ | |
| Use: "rollback <rbgName> <revisionName>", | |
| Short: "Rollback current RBG to target ControllerRevision", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runRollback(context.Background(), args[0], args[1]) | |
| }, | |
| } | |
| var rollbackCmd = &cobra.Command{ | |
| Use: "rollback <rbgName> <revisionName>", | |
| Short: "Rollback current RBG to target ControllerRevision", | |
| Args: cobra.ExactArgs(2), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runRollback(context.Background(), args[0], args[1]) | |
| }, | |
| } |
cmd/cli/cmd/showrevision.go
Outdated
| var showrevisionCmd = &cobra.Command{ | ||
| Use: "showrevision <revisionName>", | ||
| Short: "Show specific revision details", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runShowRevision(context.Background(), args[0]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
The command does not validate the number of arguments. If it's called with no arguments, it will panic due to an index-out-of-bounds error on args[0]. You should use the Args field on the cobra.Command struct to enforce that exactly one argument is provided.
| var showrevisionCmd = &cobra.Command{ | |
| Use: "showrevision <revisionName>", | |
| Short: "Show specific revision details", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runShowRevision(context.Background(), args[0]) | |
| }, | |
| } | |
| var showrevisionCmd = &cobra.Command{ | |
| Use: "showrevision <revisionName>", | |
| Short: "Show specific revision details", | |
| Args: cobra.ExactArgs(1), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runShowRevision(context.Background(), args[0]) | |
| }, | |
| } |
cmd/cli/cmd/listrevision.go
Outdated
| // List ControllerRevision | ||
| revisions, err := k8sClient.AppsV1(). | ||
| ControllerRevisions(namespace). | ||
| List(context.TODO(), metav1.ListOptions{ |
There was a problem hiding this comment.
Using context.TODO() is generally discouraged. It's better to use the context from the command. You can get it from cmd.Context() in the RunE function and pass it down to runListRevision.
| List(context.TODO(), metav1.ListOptions{ | |
| List(ctx, metav1.ListOptions{ |
cmd/cli/cmd/rollback.go
Outdated
| if rbgObject == nil { | ||
| return fmt.Errorf("RoleBasedGroup %s not found", rbgName) | ||
| } |
There was a problem hiding this comment.
The check if rbgObject == nil is unreachable. If the Get call on the preceding line doesn't find the resource, it will return a NotFound error, and err will not be nil. If err is nil, rbgObject is guaranteed to be non-nil. It's better to check for a NotFound error after the Get call.
cmd/cli/cmd/showrevision.go
Outdated
| } | ||
|
|
||
| scheme := runtime.NewScheme() | ||
| _ = workloadsv1alpha1.AddToScheme(scheme) |
There was a problem hiding this comment.
The error returned by workloadsv1alpha1.AddToScheme(scheme) is being ignored. While it might not fail in this context, it's a best practice to handle all potential errors.
if err := workloadsv1alpha1.AddToScheme(scheme); err != nil {
return err
}
cmd/cli/cmd/util.go
Outdated
| func GetRBGClient() (*rbgclient.Clientset, error) { | ||
| // Fetch Kubernetes configuration | ||
| config, err := getConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get kubeconfig: %w", err) | ||
| } | ||
| return rbgclient.NewForConfig(config) | ||
| } | ||
|
|
||
| func GetK8SClientSet() (*kubernetes.Clientset, error) { | ||
| // Fetch Kubernetes configuration | ||
| config, err := getConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get kubeconfig: %w", err) | ||
| } | ||
| return kubernetes.NewForConfig(config) | ||
| } | ||
|
|
||
| func GetDefaultDynamicClient() (*dynamic.DynamicClient, error) { | ||
| // Fetch Kubernetes configuration | ||
| config, err := getConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get kubeconfig: %w", err) | ||
| } | ||
|
|
||
| // Create a dynamic client | ||
| dynamicClient, err := dynamic.NewForConfig(config) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create dynamic client: %w", err) | ||
| } | ||
| return dynamicClient, nil | ||
| } |
There was a problem hiding this comment.
The functions GetRBGClient, GetK8SClientSet, and GetDefaultDynamicClient all contain duplicated logic for getting the Kubernetes config by calling getConfig(). This could be refactored to reduce code duplication, for instance, by having getConfig be called once and the resulting config passed to functions that create the specific clients.
doc/features/kubectl-rbgctl.md
Outdated
| ( | ||
| """ | ||
| ... // 24 identical lines | ||
| patchWorkerTemplate: null | ||
| name: worker | ||
| - replicas: 3 | ||
| + replicas: 2 | ||
| template: | ||
| metadata: {} | ||
| ... // 5 identical lines | ||
| - containerPort: 8080 | ||
| protocol: TCP | ||
| - resources: {} | ||
| + resources: | ||
| + limits: | ||
| + memory: 512Mi | ||
| + requests: | ||
| + memory: 100Mi | ||
| workload: | ||
| apiVersion: apps/v1 | ||
| ... // 2 identical lines | ||
| """ | ||
| ) | ||
|
|
There was a problem hiding this comment.
The example output for the diffrevision command contains non-breaking space characters ( ) instead of regular spaces. This is likely a copy-paste artifact and could cause rendering issues in some markdown viewers. It would be best to replace them with regular spaces for consistency and correct rendering.
|
Acknowledged. I will now perform a comprehensive code review of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI tool into a more comprehensive kubectl-rbgctl plugin with multiple subcommands for managing RoleBasedGroup (RBG) resources. This is a great enhancement, adding valuable functionality for revision management like diffing, listing, and rolling back. The code is well-structured, but I've identified several areas for improvement. My main concerns are around argument validation in the new cobra commands, which could lead to panics, the use of os.Exit(1) in init functions, and a critical bug related to unsafe slice access. I've also pointed out some minor issues with context handling and documentation.
cmd/cli/cmd/diffrevision.go
Outdated
| if rev1.OwnerReferences[0].UID != rev2.OwnerReferences[0].UID { | ||
| return fmt.Errorf("revisions %s and %s are not owned by the same resource", revision1, revision2) | ||
| } |
There was a problem hiding this comment.
Accessing OwnerReferences[0] without checking the slice length can lead to a panic if OwnerReferences is empty. You should validate the length of the slice before accessing its elements to prevent a crash.
if len(rev1.OwnerReferences) == 0 || len(rev2.OwnerReferences) == 0 || rev1.OwnerReferences[0].UID != rev2.OwnerReferences[0].UID {
return fmt.Errorf("revisions %s and %s are not owned by the same resource", revision1, revision2)
}
cmd/cli/cmd/diffrevision.go
Outdated
| var diffrevisionCmd = &cobra.Command{ | ||
| Use: "diffrevision <revision1> <revision2>", | ||
| Short: "Diffrevision displays the diff fields of two revisions", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if len(args) != 2 { | ||
| fmt.Println("Please specify two revisions") | ||
| os.Exit(1) | ||
| } | ||
| return runDiffRevision(context.Background(), args[0], args[1]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
The command's arguments are being validated manually, and os.Exit(1) is called directly, which bypasses Cobra's standard error handling. It's better to use Cobra's built-in argument validation by setting the Args field on the cobra.Command struct. This will provide better error messages to the user and make the code cleaner.
var diffrevisionCmd = &cobra.Command{
Use: "diffrevision <revision1> <revision2>",
Short: "Diffrevision displays the diff fields of two revisions",
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
return runDiffRevision(context.Background(), args[0], args[1])
},
}
cmd/cli/cmd/diffrevision.go
Outdated
| } | ||
| var raw map[string]interface{} | ||
| if err := yaml.Unmarshal(rev.Data.Raw, &raw); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Panicking on a YAML unmarshal error can cause the CLI to crash unexpectedly. It's better to handle this as a regular error and return it, allowing the caller to display a user-friendly message.
return err
cmd/cli/cmd/listrevision.go
Outdated
| var listrevisionCmd = &cobra.Command{ | ||
| Use: "listrevision <rbgName>", | ||
| Short: "List all revision of an current rbg", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runListRevision(context.Background(), args[0]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
The command Use field suggests one argument is required, but there is no validation. This will cause a panic if the command is run without arguments. Please add Args: cobra.ExactArgs(1) to the cobra.Command definition to enforce this and provide a better user experience.
| var listrevisionCmd = &cobra.Command{ | |
| Use: "listrevision <rbgName>", | |
| Short: "List all revision of an current rbg", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runListRevision(context.Background(), args[0]) | |
| }, | |
| } | |
| var listrevisionCmd = &cobra.Command{ | |
| Use: "listrevision <rbgName>", | |
| Short: "List all revision of an current rbg", | |
| Args: cobra.ExactArgs(1), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runListRevision(context.Background(), args[0]) | |
| }, | |
| } |
cmd/cli/cmd/rollback.go
Outdated
| var rollbackCmd = &cobra.Command{ | ||
| Use: "rollback <rbgName> <revisionName>", | ||
| Short: "Rollback current RBG to target ControllerRevision", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runRollback(context.Background(), args[0], args[1]) | ||
| }, | ||
| } |
There was a problem hiding this comment.
The command expects two arguments, but there is no validation. This will cause a panic if the command is run with fewer than two arguments. Please add Args: cobra.ExactArgs(2) to the cobra.Command definition to enforce this.
| var rollbackCmd = &cobra.Command{ | |
| Use: "rollback <rbgName> <revisionName>", | |
| Short: "Rollback current RBG to target ControllerRevision", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runRollback(context.Background(), args[0], args[1]) | |
| }, | |
| } | |
| var rollbackCmd = &cobra.Command{ | |
| Use: "rollback <rbgName> <revisionName>", | |
| Short: "Rollback current RBG to target ControllerRevision", | |
| Args: cobra.ExactArgs(2), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runRollback(context.Background(), args[0], args[1]) | |
| }, | |
| } |
cmd/cli/cmd/listrevision.go
Outdated
| func init() { | ||
| flags := listrevisionCmd.Flags() | ||
|
|
||
| if err := viper.BindPFlags(flags); err != nil { | ||
| fmt.Println(err) | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling os.Exit(1) from an init function is considered bad practice as it can make the package non-importable and hard to test. It's better to panic on initialization errors. This pattern is repeated across several of the new command files (diffrevision.go, rollback.go, root.go, showrevision.go) and should be corrected in all of them.
func init() {
flags := listrevisionCmd.Flags()
if err := viper.BindPFlags(flags); err != nil {
panic(err)
}
}
cmd/cli/cmd/listrevision.go
Outdated
| // List ControllerRevision | ||
| revisions, err := k8sClient.AppsV1(). | ||
| ControllerRevisions(namespace). | ||
| List(context.TODO(), metav1.ListOptions{ |
There was a problem hiding this comment.
Using context.TODO() is generally discouraged as it provides an empty, non-cancellable context. It's better to pass the context from the command down to this function call. This allows for better control, such as cancellation and deadline propagation.
List(ctx, metav1.ListOptions{
cmd/cli/cmd/root.go
Outdated
|
|
||
| var ( | ||
| namespace string | ||
| name string |
There was a problem hiding this comment.
The global variable name is declared but appears to be unused within the new command structure. It might be a leftover from a previous implementation. If it's no longer needed, it should be removed to improve code clarity.
cmd/cli/cmd/status.go
Outdated
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return runStatus(context.TODO(), args[0]) | ||
| }, |
There was a problem hiding this comment.
Using context.TODO() is generally discouraged. The context from the command should be passed down to allow for cancellation and deadline propagation. You can get the context from cmd.Context() in the RunE function and then update the runStatus and runWithClient function signatures to accept and pass it along.
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runStatus(context.TODO(), args[0]) | |
| }, | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| return runStatus(cmd.Context(), args[0]) | |
| }, |
doc/features/kubectl-rbgctl.md
Outdated
|
|
||
| ## View RBG Status | ||
| ```shell | ||
| $ kubectl rbgctl nginx-cluster -n default |
There was a problem hiding this comment.
This example command for viewing the RBG status is incorrect as it's missing the status subcommand. The correct command should be kubectl rbgctl status nginx-cluster -n default.
| $ kubectl rbgctl nginx-cluster -n default | |
| $ kubectl rbgctl status nginx-cluster -n default |
bcfre
force-pushed
the
dev-1014
branch
2 times, most recently
from
fde043c to
21c38f2
Compare
Signed-off-by: 柏存 <guoxiongfeng.gxf@alibaba-inc.com>
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.