✨ Add Grafana Plugin migration in alpha generate subcommand -- Step 2 #3509

yyy1000 · 2023-07-25T05:28:19Z

Description:

Support Grafana Plugin migration in the alpha generate subcommand.

This is the second step, which will copy the config.yaml in metrics.

k8s-ci-robot · 2023-07-25T05:28:29Z

Hi @yyy1000. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

varshaprasad96 · 2023-07-25T14:27:44Z

pkg/rescaffold/migrate.go

+		err := store.Config().DecodePluginConfig(grafanaPluginKey, grafanaPlugin)
+		// If the grafana plugin is not found, we don't need to migrate
+		if errors.As(err, &config.PluginKeyNotFoundError{}) {
+			return nil


Let us add an warning here, instead of skipping the migration silently.

varshaprasad96 · 2023-07-25T14:32:02Z

pkg/rescaffold/migrate.go

+		if errors.As(err, &config.PluginKeyNotFoundError{}) {
+			return nil
+		}
+		if err != nil {
+			return fmt.Errorf("Failed to Decode Grafana Plugin: %s. %v", grafanaPluginKey, err)
+		}


Suggested change

if errors.As(err, &config.PluginKeyNotFoundError{}) {

return nil

}

if err != nil {

return fmt.Errorf("Failed to Decode Grafana Plugin: %s. %v", grafanaPluginKey, err)

}

if err != nil {

if errors.As(err, &config.PluginKeyNotFound) {

// Add warning

return nil

}

return fmt.Errorf("failed to decode.....")

}

Also, a small nit: why are we using errors.As ? Errors.As checks if the error is of a particular type or follows an interface, errors.Is is much safer if we know the exact error which is getting returned.

Could we modify it to be errors.Is if we know what the error object contains?

Here is some doc to differentiate between the both.

Oh, I just search the code repo and copy it. :)

kubebuilder/pkg/config/v3/config_test.go

Lines 420 to 429 in a5b246f

Expect(err).To(HaveOccurred())

Expect(errors.As(err, &config.PluginKeyNotFoundError{})).To(BeTrue())

})

It("DecodePluginConfig should fail to retrieve data from a non-existent plugin", func() {

var pluginConfig PluginConfig

err := c1.DecodePluginConfig("plugin-y", &pluginConfig)

Expect(err).To(HaveOccurred())

Expect(errors.As(err, &config.PluginKeyNotFoundError{})).To(BeTrue())

})

Ah, after testing it. I think it should be errors.As here. Changing it to Is will fail. :)
Though I tried to add the key like '&config.PluginKeyNotFoundError{Key: grafanaPluginKey}' when using Is, it still fails.

varshaprasad96 · 2023-07-25T14:52:31Z

test/e2e/alphagenerate/generate_test.go

@@ -191,4 +204,11 @@ func ReGenerateProject(kbc *utils.TestContext) {
 		filepath.Join(kbc.Dir, "testdir2", "PROJECT"), grafanaPlugin)
 	ExpectWithOffset(1, err).NotTo(HaveOccurred())
 	ExpectWithOffset(1, fileContainsExpr).To(BeTrue())
+
+	By("checking if the generated grafana config file has the same content as the old one")
+	cmp := equalfile.New(nil, equalfile.Options{}) // compare using single mode


Imo, let's not introduce a new dependency library, just for testing. Instead we can work with the helpers that gomega provides.

We can use BeARegularFile (https://onsi.github.io/gomega/#bearegularfile) to check if the file exists. To check if the contents are equal, let's read them as bytes, and use gomega's BeEqualTo (https://onsi.github.io/gomega/#equalexpected-interface).

varshaprasad96 · 2023-07-25T14:53:35Z

test/e2e/alphagenerate/generate_test.go

+	cmp := equalfile.New(nil, equalfile.Options{}) // compare using single mode
+	equal, err := cmp.CompareFile(filepath.Join(kbc.Dir, "testdir2", "grafana/custom-metrics/config.yaml"),
+		filepath.Join(kbc.Dir, "grafana/custom-metrics/config.yaml"))
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())


Do we need ExpectWithOffset while comparing the error here? Can we just directly check the error? That would be easier and cleaner. Wdyt?

Good idea. Acutally I'm still a little confused when should use this one. I think 'ExpectWithOffset' could provide the message about the 'call stack', so it will be used when in a function(maybe a layer of functions). And if not the case, we should use 'Expect'. Is it right?

varshaprasad96 · 2023-07-25T14:53:59Z

test/e2e/alphagenerate/generate_test.go

+	equal, err := cmp.CompareFile(filepath.Join(kbc.Dir, "testdir2", "grafana/custom-metrics/config.yaml"),
+		filepath.Join(kbc.Dir, "grafana/custom-metrics/config.yaml"))
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+	ExpectWithOffset(1, equal).To(BeTrue())


Also, with this one. We may not need ExpectWithOffset.

camilamacedo86 · 2023-07-25T17:55:38Z

/ok-to-test

Kavinjsir · 2023-07-25T18:13:31Z

pkg/rescaffold/migrate.go

+		if err != nil {
+			return fmt.Errorf("Failed to Decode Grafana Plugin: %s. %v", grafanaPluginKey, err)
+		}
+		err = kubebuilderGrafanaEdit()


Question:
Why do we need to run kubebuilderGrafanaEdit in such circumstance where:

the PluginChain does not contain grafana-plugin but the grafana-plugin can loaded from the config?

First, in my understanding, GetPluginChain() method will return the 'layout' field in the PROJECT file, but not plugins field. Correct me if wrong. @camilamacedo86 @varshaprasad96
In this case, if we don't run kubebuilder init --plugins grafana.kubebuilder.io/v1-alpha , but run 'edit' when the project has been inited. The PluginChain will not contain the grafana plugin('layout' field), but it will appears in 'plugins' field. So we need to call 'edit' and 'copy file'.
But if we init the project using two plugins, (or only the grafana plugin itself). The layout field will contain 'grafana plugin', which could be retrieved by 'GetPluginChain()' methond. Since we will also init the new project with all plugins. We don't need to call 'edit', but need to 'copy file'.
Does it make sense?

If you find the Grafana plugin in the PROJECT file then you must to:

When you run kb init use the plugin

After copy the file which has custom metrics config option for it in the new directory where the project was scaffolded

Then, run kb edit with the plugin so that if custom metrics were setup for it then, you will also re-generate them.

I'm thinking if the logic can be something like:

If the origin PROJECT file does not contain the Grafana plugin, then exit

If the origin PROJECT file contains the Grafana plugin, which means the origin project uses it:
2.1 Run edit ...plugin ...grafana to generate the basic grafana dir
2.2 Copy the config.yaml to the new scaffold
2.3 Run edit ...plugin ...grafana again to generate additional files.

I am imagining if we can get free from the 'complicated' logic...

Cool! That's much clearer. I didn't understand 'edit' command before.

+1 Agreed with @Kavinjsir's logic. If the plugin doesn't exist in the origin PROJECT file, then we need to run the edit command. We can exit immediately.

feat: reafactor code due to suggestions feat: revert unused import in mod file fix: add warning and fix test cases fix: golang lint feat: adjust work flow and revert slice dependency

Kavinjsir

LGTM

camilamacedo86

All suggestions are in place!!!

Great work 🥇

So, that seems good to fly

camilamacedo86 · 2023-07-31T19:08:17Z

/lgtm

varshaprasad96

/lgtm

k8s-ci-robot · 2023-07-31T20:21:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir, varshaprasad96, yyy1000

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86,varshaprasad96]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

varshaprasad96 · 2023-07-31T20:21:48Z

/test pull-kubebuilder-e2e-k8s-1-27-1

k8s-ci-robot requested review from everettraven and rashmigottipati July 25, 2023 05:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 25, 2023

varshaprasad96 requested changes Jul 25, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 25, 2023

Kavinjsir reviewed Jul 25, 2023

View reviewed changes

yyy1000 force-pushed the grafana-plugin-2 branch from 4839ab6 to f1ed132 Compare July 26, 2023 12:54

feat: copy grafana config files when rescaffold

7c1ca0d

feat: reafactor code due to suggestions feat: revert unused import in mod file fix: add warning and fix test cases fix: golang lint feat: adjust work flow and revert slice dependency

yyy1000 force-pushed the grafana-plugin-2 branch from f1ed132 to 7c1ca0d Compare July 28, 2023 09:03

Kavinjsir approved these changes Jul 28, 2023

View reviewed changes

camilamacedo86 approved these changes Jul 31, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2023

k8s-ci-robot assigned camilamacedo86 Jul 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2023

varshaprasad96 approved these changes Jul 31, 2023

View reviewed changes

k8s-ci-robot assigned varshaprasad96 Jul 31, 2023

k8s-ci-robot merged commit 32d569c into kubernetes-sigs:master Jul 31, 2023
9 checks passed

yyy1000 deleted the grafana-plugin-2 branch August 5, 2023 03:46

yyy1000 mentioned this pull request Aug 29, 2023

REQUEST: New membership for yyy1000 kubernetes/org#4417

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add Grafana Plugin migration in alpha generate subcommand -- Step 2 #3509

✨ Add Grafana Plugin migration in alpha generate subcommand -- Step 2 #3509

yyy1000 commented Jul 25, 2023

k8s-ci-robot commented Jul 25, 2023

varshaprasad96 Jul 25, 2023

varshaprasad96 Jul 25, 2023

varshaprasad96 Jul 25, 2023

yyy1000 Jul 26, 2023

yyy1000 Jul 26, 2023 •

edited

Loading

varshaprasad96 Jul 25, 2023 •

edited

Loading

varshaprasad96 Jul 25, 2023

yyy1000 Jul 26, 2023

varshaprasad96 Jul 25, 2023

camilamacedo86 commented Jul 25, 2023

Kavinjsir Jul 25, 2023

yyy1000 Jul 26, 2023

camilamacedo86 Jul 27, 2023

Kavinjsir Jul 27, 2023

yyy1000 Jul 28, 2023

varshaprasad96 Jul 28, 2023

Kavinjsir left a comment

camilamacedo86 left a comment

camilamacedo86 commented Jul 31, 2023

varshaprasad96 left a comment

k8s-ci-robot commented Jul 31, 2023

varshaprasad96 commented Jul 31, 2023

	Expect(err).To(HaveOccurred())
	Expect(errors.As(err, &config.PluginKeyNotFoundError{})).To(BeTrue())
	})

	It("DecodePluginConfig should fail to retrieve data from a non-existent plugin", func() {
	var pluginConfig PluginConfig
	err := c1.DecodePluginConfig("plugin-y", &pluginConfig)
	Expect(err).To(HaveOccurred())
	Expect(errors.As(err, &config.PluginKeyNotFoundError{})).To(BeTrue())
	})

✨ Add Grafana Plugin migration in alpha generate subcommand -- Step 2 #3509

✨ Add Grafana Plugin migration in alpha generate subcommand -- Step 2 #3509

Conversation

yyy1000 commented Jul 25, 2023

Description:

k8s-ci-robot commented Jul 25, 2023

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

yyy1000 Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

varshaprasad96 Jul 25, 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

camilamacedo86 commented Jul 25, 2023

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

Kavinjsir left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Jul 31, 2023

varshaprasad96 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 31, 2023

varshaprasad96 commented Jul 31, 2023

yyy1000 Jul 26, 2023 •

edited

Loading

varshaprasad96 Jul 25, 2023 •

edited

Loading