Skip to content
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

Add [antctl mc] command for antrea multicluster resources #3341

Merged
merged 1 commit into from Mar 21, 2022

Conversation

hjiajing
Copy link
Contributor

@hjiajing hjiajing commented Feb 22, 2022

antctl: add multicluster subcommand

Add a new subcommand for antrea multicluster and add get verb for query resources. We could list or get mc resource by antctl mc get .

Example:

# ./antctl mc get ri -A
NAMESPACE                NAME                                                       KIND
antrea-mcs-ns            antrea-multicluster-test-east-nginx-endpoints              ResourceImport
antrea-mcs-ns            antrea-multicluster-test-east-nginx-service                ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-endpoints              ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-service                ResourceImport

In the future, other verb like create or delete also could be added to the sub-command mc if it is nesscessary.

@hjiajing
Copy link
Contributor Author

@luolanzone Could you please help to review. Thanks

@hjiajing
Copy link
Contributor Author

@bangqipropel Could you please help to review. Thanks

@lgtm-com
Copy link

lgtm-com bot commented Feb 22, 2022

This pull request introduces 1 alert when merging cc281ca into 3f30d84 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #3341 (ab76b64) into main (24f85ce) will decrease coverage by 7.06%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3341      +/-   ##
==========================================
- Coverage   62.11%   55.05%   -7.07%     
==========================================
  Files         269      375     +106     
  Lines       26852    41332   +14480     
==========================================
+ Hits        16680    22756    +6076     
- Misses       8334    16179    +7845     
- Partials     1838     2397     +559     
Flag Coverage Δ
integration-tests 35.76% <ø> (?)
kind-e2e-tests 55.80% <ø> (+6.85%) ⬆️
unit-tests 42.55% <50.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/antctl/antctl.go 66.66% <ø> (ø)
pkg/antctl/command_definition.go 44.18% <33.33%> (-7.79%) ⬇️
pkg/antctl/command_list.go 48.48% <100.00%> (+0.79%) ⬆️
pkg/agent/util/ethtool/ethtool_linux.go 0.00% <0.00%> (-70.00%) ⬇️
pkg/agent/openflow/meters_linux.go 36.36% <0.00%> (-18.19%) ⬇️
pkg/agent/flowexporter/exporter/certificate.go 33.33% <0.00%> (-16.67%) ⬇️
pkg/apis/controlplane/v1beta2/sets.go 51.21% <0.00%> (-9.76%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 74.51% <0.00%> (-6.78%) ⬇️
...kg/agent/flowexporter/connections/conntrack_ovs.go 72.72% <0.00%> (-4.85%) ⬇️
.../registry/networkpolicy/clustergroupmember/rest.go 84.31% <0.00%> (-3.93%) ⬇️
... and 176 more

Comment on lines 95 to 96
Short: "Sub-commands of multiple cluster",
Long: "Sub-commands of multiple cluster",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe 'Sub-commands of Multi-cluster feature'?

func init() {
GetCmd = &cobra.Command{
Use: "get",
Short: "List or query resources of multiple cluster",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/List or query resources of multiple cluster/Display one or many resources in a ClusterSet/

"resourceimports",
"ri",
},
Short: "Print Multi-Cluster resource imports",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/resource imports/ResourceImports/

pkg/antctl/raw/multicluster/resourceimport.go Outdated Show resolved Hide resolved
}
o := &resourceImportOptions{}
options = o
command.Flags().StringVarP(&o.namespace, "namespace", "n", "", "namespace of resource import")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/namespace of resource import/Namespace of ResourceImports/

options = o
command.Flags().StringVarP(&o.namespace, "namespace", "n", "", "namespace of resource import")
command.Flags().StringVarP(&o.outputFormat, "output", "o", "", "Output format. Supported formats: json|yaml")
command.Flags().BoolVarP(&o.allNamespace, "all-namespace", "A", false, "Get resource of all namespace or not")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to refer kubectl get help info and change it as below:

If present, list ResourceImports across all namespaces.


func output(resourceImports []multiclusterv1alpha1.ResourceImport) string {
var output strings.Builder
formatter := "%-25s%-50s%-25s\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the name column's length be wider? I got below results:

NAMESPACE                NAME                                              KIND
docs                     default-nginx-endpoints                           ResourceImport
docs                     default-nginx-service                             ResourceImport
docs                     test-cluster-temp-kube-system-lan-k8s-temp-0-tunnelendpointResourceImport
docs                     test-cluster-west-kube-system-lan-k8s-0-0-tunnelendpointResourceImport

fmt.Println("No resource found")
return nil
}
return fmt.Errorf("Error from server (NotFound): resourceimport %s not found.", args[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/resourceimport/ResourceImport/

}
if len(res) == 0 {
if len(args) == 0 {
fmt.Println("No resource found")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to return a message like below to allow user to understand it's empty result in a given Namespace.

No resources found in kube-system namespace.

resourceImportList := &multiclusterv1alpha1.ResourceImportList{}
err = k8sClient.List(context.TODO(), resourceImportList, &client.ListOptions{Namespace: options.namespace})

for _, ri := range resourceImportList.Items {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assign len(args) to a var if you will use it multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

var resourceImportExample = strings.Trim(`
Gel all resource imports of cluster set in default namesapce
$ antctl get resourceimport
Get all resource imports of cluster set in all namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/all namespace/all namespaces/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

var options *resourceImportOptions

var resourceImportExample = strings.Trim(`
Gel all resource imports of cluster set in default namesapce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/of cluster set/ of a cluster set/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

$ antctl get resourceimport -o json
`, "\n")

func (o *resourceImportOptions) validateAndComplete() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned err is always nil, you can remove the return parameter error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 62 to 74
if o.allNamespace {
o.namespace = metav1.NamespaceAll
} else if o.namespace == "" {
o.namespace = metav1.NamespaceDefault
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be:

	if o.allNamespace {
		o.namespace = metav1.NamespaceAll
		return
	}
	if o.namespace == "" {
		o.namespace = metav1.NamespaceDefault
		return
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
return nil
}
return fmt.Errorf("Error from server (NotFound): ResourceImport %s not found.", args[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Error/error/
and remove the punctuation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it (NotFound) error when res size is zero? I suppose only when args[0] is not empty, you can tell it's NotFound. you should also handle the case when other args are not empty but args[0] is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

})

for _, ri := range resourceImports {
fmt.Fprintf(&output, formatter, ri.Namespace, ri.Name, ri.Kind)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kind here shouldn't be ri.Kind, it's always ResourceImport, it should be re.Spec.Kind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

options = o
command.Flags().StringVarP(&o.namespace, "namespace", "n", "", "Namespace of ResourceImports")
command.Flags().StringVarP(&o.outputFormat, "output", "o", "", "Output format. Supported formats: json|yaml")
command.Flags().BoolVarP(&o.allNamespace, "all-namespace", "A", false, "If present, list ResourceImports across all namespaces")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/all-namespace/all-namespaces/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@luolanzone
Copy link
Contributor

@jianjuns @antoninbas Could you help to review this PR? we plan to have a mc sub command and move all related commands into this command.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR description, replace:

In the future, other verb like create or delete also could be added to the sub-command mc if it is nesscessary.

with

In the future, other verbs like create or delete also could be added to the mc sub-command if it is necessary.

BTW, we usually try to include more information (same as in the PR description) in the commit message (right now yours is empty), so that this information stays available in the git history after we merge the PR. See https://cbea.ms/git-commit/

Gel all resource imports of cluster set in default namesapce
$ antctl get resourceimport
Get all resource imports of cluster set in all namespace
$ antctl get resoruceimport -A
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: s/resoruceimport/resourceimport

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


var resourceImportExample = strings.Trim(`
Gel all resource imports of cluster set in default namesapce
$ antctl get resourceimport
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't those sample commands missing the mc group?

e.g. antctl mc get resourceimport

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -63,7 +63,7 @@ func (cl *commandList) applyToRootCommand(root *cobra.Command, client AntctlClie

for _, cmd := range cl.rawCommands {
if (runtime.Mode == runtime.ModeAgent && cmd.supportAgent) ||
(runtime.Mode == runtime.ModeController && cmd.supportController) {
(runtime.Mode == runtime.ModeController && cmd.supportController) || cmd.commandGroup == mc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand this. So the mc commands are always added, even when I run antctl from inside an antrea-agent Pod. Is this right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it will be added no matter it's inside a Pod or not, @hjiajing I think you need to check both cmd.supportController and cmd.supportAgent so it won't add mc sub-command when it's running in a Pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"ri",
},
Short: "Print Multi-Cluster ResourceImports",
Args: cobra.MaximumNArgs(1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reference to this positional argument in help messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 129 to 131
if argsNum > 0 && ri.Name == args[0] {
res = append(res, ri)
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you use a k8sClient.Get instead of a k8sClient.List for this case (single resource needs to be retrieved by name)?

if len(res) == 0 {
if argsNum == 0 {
if options.namespace != "" {
fmt.Printf("No resource found in namespace %s\n", options.namespace)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/namespace/Namespace everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if err != nil {
return err
}
fmt.Println(string(yamlOutput))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check the yaml output? I got a quite strange output like below, something like raw byte data.

   managedfields:
    - manager: antrea-mc-controller
      operation: Update
      apiversion: multicluster.crd.antrea.io/v1alpha1
      time: "2022-01-11T16:53:56+08:00"
      fieldstype: FieldsV1
      fieldsv1:
        raw:
        - 123
        - 34
        - 102
        - 58
        - 115
        - 112
        - 101
        - 99
        - 34
        - 58
        - 123
        - ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, why am I getting empty string in kind and apiversion? and the yaml format should be similar as 'kubectl get pod podname -o yaml', Could you check it? thanks.

./bin/antctl-darwin mc get resourceimports default-nginx-endpoints -n docs -o yaml
- typemeta:
    kind: ""
    apiversion: ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hjiajing Could you also check if possible to remove the last empty line when we execute antctl command? I think we should remote this unnecessary empty line. maybe fix it in a separate PR since it's not a specific issue for mc, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is caused by yaml module, and I changed it to sigs.k8s.io/yaml then it works fine. About the empty line, I will fix it later in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hjiajing I tried -o yaml but below info are missing, Could you double check? the json output has the same problem.

apiVersion: multicluster.crd.antrea.io/v1alpha1
kind: ResourceImport

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luolanzone Got it. I found that when I tried to get or list all resources, not only CRDs but also k8s resources like Pod. The Typemeta field is empty. It's confused, I'm trying to solve it now.

return err
}

fmt.Println(prettyJson.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn't print json in an array when user gives one specific ResourceImport.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

options = o
command.Flags().StringVarP(&o.namespace, "namespace", "n", "", "Namespace of ResourceImports")
command.Flags().StringVarP(&o.outputFormat, "output", "o", "", "Output format. Supported formats: json|yaml")
command.Flags().BoolVarP(&o.allNamespace, "all-namespaces", "A", false, "If present, list ResourceImports across all Namespaces")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to rename allNamespace to allNamespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@bangqipropel
Copy link
Contributor

@luolanzone #3341 (comment) we don't want the raw data? maybe I have the same output

@luolanzone
Copy link
Contributor

@luolanzone #3341 (comment) we don't want the raw data? maybe I have the same output

Please compare json and yaml output, they are obviously different beside format. that's not correct.

Comment on lines 127 to 134
var outputType string
if argsNum > 0 {
outputType = singleType
} else {
outputType = multipleType
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just a bool variable? I feel it's redundant to use two variables. it's can be a bool with a name like isSingleType

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@luolanzone
Copy link
Contributor

@hjiajing it looks good to me overall, but there is a tidy check failure, please fix it: https://github.com/antrea-io/antrea/runs/5402785503?check_suite_focus=true
@antoninbas @jianjuns Could you help to take a look? thanks.

$ antctl mc get resourceimport -A
Get all resource imports of specified namespace
$ antctl mc get resourceimport -n <NAMESPACE>
Get all resource imports and print json format
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Get all resource imports and print json format
Get all resource imports and print then in JSON format

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. BTW, maybe it is "them" not "then" ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct

$ antctl mc get resourceimport
Get all resource imports of a cluster set in all Namespaces
$ antctl mc get resourceimport -A
Get all resource imports of specified namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Get all resource imports of specified namespace
Get all resource imports of in the specified Namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

$ antctl mc get resourceimport -n <NAMESPACE>
Get all resource imports and print json format
$ antctl mc get resourceimport -o json
Get a specified resourceimport
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Get a specified resourceimport
Get the specified resourceimport

return err
}

isSingleType := false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this called isSingleType?

I would suggest something like singleResource or retrieveByName

Comment on lines 132 to 133
switch isSingleType {
case true:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use switch / case for a single boolean value, instead of if / else?

maybe write helper functions for the 2 cases for code readability if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


var options *resourceImportOptions

var resourceImportExample = strings.Trim(`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: resourceImportExample -> resourceImportExamples

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

res = resourceImportList.Items
}

switch options.outputFormat {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These codes can be common for all commands? And do we have a way to share output code in antctl/command_definition.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Jianjun. Thanks for comment. I tried to share output code in comman_definition.go, but I found that the output method is used by commandDefinition rather than raw command, so maybe it is not easy to share the output code. And the command antctl mc get resources will list and get different resources, so maybe it's difficult to be common for all list&get commands. I don't know if you have some detailed suggestions, thank you .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least line 174 - 185 can be shared? For line 187 - 195, could we define an interface similar to (or even share) TableOutput in antctl/transform/common?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, thanks. I'll do it.

luolanzone
luolanzone previously approved these changes Mar 7, 2022
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luolanzone
Copy link
Contributor

@hjiajing there are some new comments from Jianjun and antonin, please address it. thanks.

@jianjuns
Copy link
Contributor

we have clusterSet structure, the table outputs are different if we enter a clusterSetID or not
Could you point to me the code of the above? I guess that is what I missed.

@bangqipropel
Copy link
Contributor

@jianjuns Sorry my reply to comment have some problem, just reply it here.
Here is my codes for ClusterSet output, similar to Jiajing's code.

https://github.com/antrea-io/antrea/pull/3287/files#:~:text=Println(string(yamlByte))-,default%3A,%7D,-51%20%20pkg/antctl

the whole PR is : #3287

@jianjuns
Copy link
Contributor

@bangqipropel: I checked. But if outputAllClusterSet is the only case, we can just handle it specially. The question is do we believe the TableOutput interface works for most commands.

It is not an issue tableOutputForGetCommands() is in commandDefinition, we can always move the func out to be shared. I checked its code seems does not depends on any commandDefinition state.

var options *resourceImportOptions

var resourceImportExamples = strings.Trim(`
Gel all resource imports of a cluster set in default Namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we also need address this comment here? #3287 (comment)

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Several nits.

@@ -387,7 +393,7 @@ func jsonEncode(obj interface{}, output *bytes.Buffer) error {
return nil
}

func (cd *commandDefinition) jsonOutput(obj interface{}, writer io.Writer) error {
func JsonOutput(obj interface{}, writer io.Writer) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move these funcs to a separate file like output.go. If it is easier for import, even a separate pkg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. I moved all the output functions to the output.go. Now that the command_definition.go only contains the commandDefinition(cd) methods

var options *resourceImportOptions

var resourceImportExamples = strings.Trim(`
Gel all resource imports of a cluster set in default Namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we capitalize the first letter of resource types, so: ResourceImports, ClusterSet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jianjuns
jianjuns previously approved these changes Mar 15, 2022
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Mar 16, 2022

Just found #3287 is based on this PR.
I have left some comments there, could you check #3287 (comment) and #3287 (review)? @hjiajing

@hjiajing
Copy link
Contributor Author

@tnqn hello Quan. Thanks for comment. I have got the comment this morning so I made some changes. Just move the output.go to a new package in order to avoid cycle import. About other comments, I'm addressing them now. Thanks.

@hjiajing
Copy link
Contributor Author

@tnqn Hello Quan, Could you please help to review, thanks

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits, otherwise LGTM


if len(resourceImportList.Items) == 0 {
if options.namespace != "" {
fmt.Printf("No resource found in Namespace %s\n", options.namespace)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #3287 (comment)
I think this information normally goes to stderr to make user able to distinguish it from valid output easily.

fmt.Fprintln(cmd.ErrOrStderr(), "No resources found")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/antctl/raw/multicluster/scheme/scheme.go Show resolved Hide resolved
@tnqn
Copy link
Member

tnqn commented Mar 18, 2022

@hjiajing could you add more details to the commit message, for example:

antctl: add multicluster subcommand

Add a new subcommand for antrea multicluster and add get verb for query
resources. We could list or get mc resource by antctl mc get <Resource>.

Example:

# ./antctl mc get ri -A
NAMESPACE                NAME                                                                                  KIND
antrea-mcs-ns            antrea-multicluster-test-east-nginx-endpoints              ResourceImport
antrea-mcs-ns            antrea-multicluster-test-east-nginx-service                  ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-endpoints             ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-service                 ResourceImport

In the future, other verb like create or delete also could be added to
the sub-command mc if it is nesscessary.

BTW, why the output in the example is not aligned? Is it caused by copy-paste or the real output is not aligned?

@hjiajing
Copy link
Contributor Author

hjiajing commented Mar 18, 2022

@tnqn I updated the commit message, but I found that the output is not aligned again. But I pasted the output to the editor, it's aligned. Maybe it cause by the copy-paste. But, it's odd that when I paste again, it's aligned.

@tnqn
Copy link
Member

tnqn commented Mar 18, 2022

@hjiajing you didn't update commit message, it's still empty. I see Antonin suggested the same here: #3341 (review)
Note that commit message is different from PR description.

antctl: add multicluster subcommand  <---- this is the commit message title I suggested

Add a new subcommand for antrea multicluster and add get verb for query       <---- this is the commit message body
resources. We could list or get mc resource by antctl mc get <Resource>.

Example:

# ./antctl mc get ri -A
NAMESPACE                NAME                                                                                  KIND
antrea-mcs-ns            antrea-multicluster-test-east-nginx-endpoints              ResourceImport
antrea-mcs-ns            antrea-multicluster-test-east-nginx-service                  ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-endpoints             ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-service                 ResourceImport

In the future, other verb like create or delete also could be added to
the sub-command mc if it is nesscessary.

@tnqn
Copy link
Member

tnqn commented Mar 18, 2022

/test-multicluster-e2e
/test-e2e
/skip-conformance
/skip-networkpolicy

@tnqn
Copy link
Member

tnqn commented Mar 18, 2022

code is not gofmted:

===> Running golangci (linux) <===
pkg/antctl/command_definition_test.go:33: File is not `gofmt`-ed with `-s` (gofmt)
	"antrea.io/antrea/pkg/agent/apiserver/handlers/podinterface"

Add a new subcommand for antrea multicluster and add get verb for query
resources. We could list or get mc resource by antctl mc get <Resource>.

Example:

NAMESPACE                NAME                                                         KIND
antrea-mcs-ns            antrea-multicluster-test-east-nginx-endpoints                ResourceImport
antrea-mcs-ns            antrea-multicluster-test-east-nginx-service                  ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-endpoints                ResourceImport
antrea-mcs-ns            antrea-multicluster-test-west-nginx-service                  ResourceImport

In the future, other verb like create or delete also could be added to
the sub-command mc if it is nesscessary.

Signed-off-by: hjiajing <hjiajing@vmware.com>
@hjiajing
Copy link
Contributor Author

@tnqn done. Thanks for your comments.

@hjiajing
Copy link
Contributor Author

/test-multicluster-e2e
/test-e2e
/skip-conformance
/skip-networkpolicy

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Mar 21, 2022

/test-integration

@tnqn
Copy link
Member

tnqn commented Mar 21, 2022

/skip-conformance
/skip-networkpolicy

@tnqn tnqn merged commit 9879f4c into antrea-io:main Mar 21, 2022
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants