core: move project to golang #79

subhamkrai · 2023-01-04T13:17:24Z

starting migration from bash to go, starting with
basic ceph commands and getting mons endpoint

Signed-off-by: subhamkrai srai@redhat.com

travisn

A few questions to get started

pkg/mons/kubectl.go

pkg/kube-client.go

pkg/ceph/ceph-commands.go

kubectl-rook-ceph.go

subhamkrai · 2023-01-09T13:00:25Z

btw this how ci is executing commands, not sure why

kubectl rook-ceph ceph status
ceph status --connect-timeout=10 --conf=/var/lib/rook/rook-ceph/rook-ceph.config

+ kubectl rook-ceph mons
a=10.99.115.54:67

.github/workflows/go-test.yaml

travisn · 2023-01-09T18:55:57Z

cmd/ceph/ceph.go

@@ -0,0 +1,35 @@
+/*


How about if we put all the go files with the cobra definitions directly in cmd/? Seems like we don't need the subdirectories for ceph, rbd, etc.

I kept main.go under cmd folder and all other commands inside commands folder otherwise everything go file will be under main package and now current structure seems okay to me

travisn · 2023-01-09T18:56:42Z

cmd/ceph/ceph.go

+var CephCmd = &cobra.Command{
+	Use:                "ceph",
+	Short:              "call a 'ceph' CLI command with arbitrary args",
+	Long:               `call a 'ceph' CLI command with arbitrary args`,


If we don't define Long, will it default to the same as Short? It would be nice to avoid the redundancy, or else define a different Short and Long description.

removed the Long

cmd/mon/mons.go

cmd/root/root.go

pkg/cephrbd/ceph_rbd_commands.go

pkg/mons/mon-endpoints.go

cmd/commands/ceph.go

cmd/commands/root.go

pkg/cephrbd/ceph_rbd_commands.go

pkg/kube-client.go

pkg/mons/mon-endpoints.go

cmd/commands/root.go

travisn · 2023-01-11T17:40:28Z

.github/workflows/go-test.yaml

+
+      - name: build the binary
+        run: |
+          go build -o kubectl-rook-ceph cmd/main.go


Instead of running go build in this action, we need a makefile. Then developers can easily and consistently build locally during development, and the CI can also call make. But let's try to make it much simpler than the rook makefile. :)

created makefile, I think this is one of the simplest makefile you will come across 😅

Simple makefiles are the best kind!

pkg/k8sutil/ceph_rbd_commands.go

pkg/mons/mon-endpoints.go

Makefile

cmd/commands/root.go

pkg/k8sutil/exec.go

travisn · 2023-01-13T15:06:49Z

pkg/mons/mon-endpoints.go

+
+const monConfigMap = "rook-ceph-mon-endpoints"
+
+func GetMonEndpoint(clusterNamespace string) {


Let's add a unit test for this method so we can establish the unit tests from the start. You'll just need to have the kube client as a parameter to the method.

this method is not returning anything, how we should write the unit test?

As discussed, how about refactoring lines 40-44 to a separate method that can be unit tested.

added a unit-test

cmd/commands/ceph.go

Makefile

go.mod

pkg/k8sutil/exec.go

Madhu-1

Some nits. LGTM

cmd/commands/mons.go

pkg/k8sutil/exec.go

pkg/k8sutil/kube-client.go

pkg/mons/mon-endpoints.go

parth-gr · 2023-01-17T14:43:10Z

cmd/commands/root.go

+
+	RootCmd.PersistentFlags().StringVar(&KubeConfig, "kubeconfig", "", "kubernetes config path")
+	RootCmd.PersistentFlags().StringVar(&OperatorNamespace, "opeator-namespace", "rook-ceph", "Kubernetes namespace where rook operator is running")
+	RootCmd.PersistentFlags().StringVar(&CephClusterNamespace, "namespace", "rook-ceph", "Kubernetes namespace where ceph cluster is created")


How the krew cmd will run can you give a doc example for ceph health,

I can't how you might be calling the status, does it like calling the k8s client and then calling ceph cmd through it?

And one more question/doubt What if we just create and call the cmds from toolbox and wrap them into a go package?

Currently, we are using operator pod for running ceph commands and toolbox and operator pod are almost same. So we prefer rook operator pod as rook/rook also uses operator pod for running ceph command internally

travisn · 2023-01-17T18:39:25Z

pkg/k8sutil/exec.go

+		clientcmd.NewDefaultClientConfigLoadingRules(),
+		&clientcmd.ConfigOverrides{},
+	)
+	rest, err := kubeconfig.ClientConfig()


Seems like these lines 42-49 are duplicated from what is in GetKubeClient(). Instead of duplicating, how about a different helper method instead of GetKubeClient() that gives back the rest and client? Or some other refactoring.

In the future we may also want to unit test commands where we can mock K8s and the exec commands, like we do in the Rook core. So I'd suggest that this method should take the kubeconfig as a parameter, then the core implementation takes parameters for the context that can be mocked. The kubeconfig would likely be initialized at the top layer under the cmd package.

now, I'm returning the rest and client from GetKubeClient only.
I'm not sure about second part could you refer some example

travisn · 2023-01-17T18:41:18Z

pkg/mons/mon-endpoints.go

+
+const monConfigMap = "rook-ceph-mon-endpoints"
+
+func GetMonEndpoint(clusterNamespace string) {


As discussed, how about refactoring lines 40-44 to a separate method that can be unit tested.

travisn · 2023-01-18T19:39:09Z

In the future we may also want to unit test commands where we can mock K8s and the exec commands, like we do in the Rook core. So I'd suggest that this method should take the kubeconfig as a parameter, then the core implementation takes parameters for the context that can be mocked. The kubeconfig would likely be initialized at the top layer under the cmd package.

now, I'm returning the rest and client from GetKubeClient only.
I'm not sure about second part could you refer some example

Rook has a method to create the context, which initializes most of the context here. By default, it will create types that will exec real commands and expect a real k8s cluster. But then the unit tests can mock the exec and k8s methods by passing a different context, for example as created here for the K8s tests, or search for MockExecutor for mocking the commands. This can all be done in a separate PR though.

.github/workflows/go-test.yaml

cmd/commands/root.go

pkg/k8sutil/exec.go

pkg/mons/mon-endpoints_test.go

subhamkrai · 2023-01-19T07:15:05Z

In the future we may also want to unit test commands where we can mock K8s and the exec commands, like we do in the Rook core. So I'd suggest that this method should take the kubeconfig as a parameter, then the core implementation takes parameters for the context that can be mocked. The kubeconfig would likely be initialized at the top layer under the cmd package.

now, I'm returning the rest and client from GetKubeClient only.
I'm not sure about second part could you refer some example

Rook has a method to create the context, which initializes most of the context here. By default, it will create types that will exec real commands and expect a real k8s cluster. But then the unit tests can mock the exec and k8s methods by passing a different context, for example as created here for the K8s tests, or search for MockExecutor for mocking the commands. This can all be done in a separate PR though.

I think I'm not very much confident about how that's works, may be discussed in call will make it clear.

travisn

Approved with one more suggestion, thanks

travisn · 2023-01-19T17:55:36Z

Makefile

+build:
+	gofmt -w $(shell find . -type f -name '*.go')
+	@echo
+	env GOOS=$(shell go env GOOS) GOARCH=$(shell go env GOARCH) go build -o kubectl-rook-ceph  cmd/main.go


After a build, the binary shows up in the local repo as a new file, and could be accidentally committed. How about if we output the binary to a bin/ subfolder, then add bin to a .gitignore?

starting migration from bash to go, starting with basic ceph commands and getting mons endpoint Signed-off-by: subhamkrai <srai@redhat.com>

travisn · 2023-01-20T14:49:35Z

LGTM!

travisn reviewed Jan 4, 2023

View reviewed changes

pkg/mons/kubectl.go Outdated Show resolved Hide resolved

pkg/kube-client.go Outdated Show resolved Hide resolved

pkg/ceph/ceph-commands.go Outdated Show resolved Hide resolved

kubectl-rook-ceph.go Outdated Show resolved Hide resolved

subhamkrai mentioned this pull request Jan 4, 2023

ci: update ubuntu to 20.04 #80

Merged

subhamkrai force-pushed the move-to-go branch 2 times, most recently from 90401ac to aa7a5fd Compare January 9, 2023 05:10

subhamkrai marked this pull request as ready for review January 9, 2023 05:12

subhamkrai force-pushed the move-to-go branch 2 times, most recently from e5407b8 to 52e573f Compare January 9, 2023 13:33

subhamkrai requested a review from travisn January 9, 2023 14:04

travisn requested changes Jan 9, 2023

View reviewed changes

subhamkrai force-pushed the move-to-go branch 2 times, most recently from 6d0346c to 1da0c4e Compare January 10, 2023 15:17

travisn requested changes Jan 10, 2023

View reviewed changes

cmd/commands/ceph.go Show resolved Hide resolved

cmd/commands/root.go Outdated Show resolved Hide resolved

pkg/cephrbd/ceph_rbd_commands.go Outdated Show resolved Hide resolved

pkg/kube-client.go Outdated Show resolved Hide resolved

pkg/mons/mon-endpoints.go Outdated Show resolved Hide resolved

subhamkrai force-pushed the move-to-go branch 2 times, most recently from c1f4493 to aee859e Compare January 11, 2023 14:52

travisn requested changes Jan 11, 2023

View reviewed changes

subhamkrai force-pushed the move-to-go branch from aee859e to 03cc1b1 Compare January 12, 2023 03:30

subhamkrai requested a review from travisn January 12, 2023 03:31

travisn requested changes Jan 12, 2023

View reviewed changes

subhamkrai force-pushed the move-to-go branch from 03cc1b1 to 83bd39f Compare January 13, 2023 05:29

travisn requested changes Jan 13, 2023

View reviewed changes

Madhu-1 reviewed Jan 16, 2023

View reviewed changes

subhamkrai force-pushed the move-to-go branch 2 times, most recently from 4fe837b to b6207fe Compare January 16, 2023 13:54

Madhu-1 reviewed Jan 17, 2023

View reviewed changes

cmd/commands/mons.go Show resolved Hide resolved

pkg/k8sutil/exec.go Outdated Show resolved Hide resolved

pkg/k8sutil/exec.go Outdated Show resolved Hide resolved

pkg/k8sutil/kube-client.go Outdated Show resolved Hide resolved

pkg/mons/mon-endpoints.go Outdated Show resolved Hide resolved

subhamkrai force-pushed the move-to-go branch from b6207fe to 06cec60 Compare January 17, 2023 09:06

subhamkrai requested a review from Madhu-1 January 17, 2023 09:17

Madhu-1 approved these changes Jan 17, 2023

View reviewed changes

parth-gr reviewed Jan 17, 2023

View reviewed changes

travisn requested changes Jan 17, 2023

View reviewed changes

subhamkrai force-pushed the move-to-go branch from 06cec60 to 1a349cb Compare January 18, 2023 13:43

travisn requested changes Jan 18, 2023

View reviewed changes

subhamkrai force-pushed the move-to-go branch from 1a349cb to 4bf91e2 Compare January 19, 2023 07:12

subhamkrai force-pushed the move-to-go branch from 4bf91e2 to 202f903 Compare January 19, 2023 07:16

travisn approved these changes Jan 19, 2023

View reviewed changes

core: move project to golang

4af4d5b

starting migration from bash to go, starting with basic ceph commands and getting mons endpoint Signed-off-by: subhamkrai <srai@redhat.com>

subhamkrai force-pushed the move-to-go branch from 202f903 to 4af4d5b Compare January 20, 2023 11:47

subhamkrai requested review from travisn and Madhu-1 January 20, 2023 12:56

subhamkrai merged commit 1ff1c5c into rook:master Jan 20, 2023

subhamkrai deleted the move-to-go branch January 20, 2023 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: move project to golang #79

core: move project to golang #79

subhamkrai commented Jan 4, 2023

travisn left a comment

subhamkrai commented Jan 9, 2023 •

edited

Loading

travisn Jan 9, 2023

subhamkrai Jan 10, 2023

travisn Jan 9, 2023

subhamkrai Jan 10, 2023

travisn Jan 11, 2023

subhamkrai Jan 12, 2023

travisn Jan 12, 2023

travisn Jan 13, 2023

subhamkrai Jan 16, 2023

travisn Jan 17, 2023

subhamkrai Jan 18, 2023

Madhu-1 left a comment

parth-gr Jan 17, 2023

subhamkrai Jan 18, 2023

travisn Jan 17, 2023

travisn Jan 17, 2023

subhamkrai Jan 18, 2023

travisn Jan 17, 2023

travisn commented Jan 18, 2023

subhamkrai commented Jan 19, 2023

travisn left a comment

travisn Jan 19, 2023

subhamkrai Jan 20, 2023

travisn commented Jan 20, 2023


		const monConfigMap = "rook-ceph-mon-endpoints"

		func GetMonEndpoint(clusterNamespace string) {

core: move project to golang #79

core: move project to golang #79

Conversation

subhamkrai commented Jan 4, 2023

travisn left a comment

Choose a reason for hiding this comment

subhamkrai commented Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn commented Jan 18, 2023

subhamkrai commented Jan 19, 2023

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn commented Jan 20, 2023

subhamkrai commented Jan 9, 2023 •

edited

Loading