🐛 (API - External Plugins): Fix external plugin discovery on Linux #3247

em-r · 2023-02-23T02:12:55Z

This PR aims to fix a bug that's causing external plugin discovery to fail in Linux systems when XDG_CONFIG_HOME environment variable is set.

fixes: #3224

k8s-ci-robot · 2023-02-23T02:13:04Z

Hi @em-r. 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.

em-r · 2023-02-23T02:13:44Z

/cc @camilamacedo86

camilamacedo86 · 2023-02-23T11:29:43Z

pkg/cli/options.go

 	if err != nil {
 		return "", fmt.Errorf("error retrieving home dir: %v", err)
 	}
 	pluginsRoot = filepath.Join(userHomeDir, pluginsRoot)

-	return pluginsRoot, nil


This method expects a return (pluginsRoot string, err error) I think it is missing return it

the reason I have replaced return pluginsRoot, nil with just return is because the function uses named returns so I think specifying the variables in the return is redundant, but I can add it back (maybe because of a style convention?) just let me know.
thank you!

If we do not return this values then we should remove them from the signature.
Could you please change that?

camilamacedo86

/ok-to-test

camilamacedo86 · 2023-02-23T11:32:10Z

pkg/cli/options.go

@@ -171,26 +171,44 @@ func parseExternalPluginArgs() (args []string) {
 	return args


@evansheng @rashmigottipati could you please give a hand to review this one?

em-r · 2023-02-23T18:04:46Z

I can see the coverage check failed, I'll add more tests to fix this as well.
edit: added a test on f883680 which should hopefully increase the coverage.
edit 2: nope, the test actually failed in the ci somehow, will look into it and prepare a fix.

em-r · 2023-02-27T02:46:45Z

coverage drop has been fixed. I'm seeing a failure in k8s-1-24-7 build, however, based on its logs I don't think it's related to the changes in this PR, it seems like some sort of missing file issue in the machine where the build ran. will keep looking into it.
cc @camilamacedo86

everettraven

Thanks for this contribution @em-r , these changes look great!

/lgtm

everettraven · 2023-03-03T18:15:02Z

/retest

everettraven · 2023-03-03T19:02:19Z

The test failure seems like some weird kustomize errors. I'm not sure off the top of my head what the problem here is

em-r · 2023-03-03T22:34:57Z

thanks @everettraven for the review!
I'll try to look into the build errors over the weekend, and hopefully, figure out a way to resolve them. I'll keep you posted.

camilamacedo86 · 2023-03-06T09:29:04Z

/test pull-kubebuilder-e2e-k8s-1-24-7

em-r · 2023-03-06T16:03:03Z

thanks @camilamacedo86 for re-running the failing build.
seems to be passing now @everettraven

camilamacedo86 · 2023-03-06T16:55:45Z

pkg/cli/options.go

+	}
+	return false
+}
+
 // getPluginsRoot detects the host system and gets the plugins root based on the host.
 func getPluginsRoot(host string) (pluginsRoot string, err error) {


/hold

Suggested change

func getPluginsRoot(host string) (pluginsRoot string, err error) {

func getPluginsRoot(host string) {

Can we first remove the return since it is not used?
https://github.com/kubernetes-sigs/kubebuilder/pull/3247/files#r1126146318

@camilamacedo86 this function is assigned to retrievePluginsRoot, and then called by DiscoverExternalPlugins. the output of the function is indeed used then to lookup external plugins in pluginsRoot.
to elaborate as to why I've changed return pluginsRoot, nil to just return: it's because the function uses named returns, so while using return pluginsRoot, nil as a return expression would work, specifying the variables would be redundant, so essentially, just using a "void" return will have the same outcome (because of the named return in the signature).
but either way, I can revert it back to the old style as it would work fine as well. what do you think?
thank you!

I think that is fine as it is . We can re-check in a follow up either.
Thank you a lot for your help to address those ones.

thank you @camilamacedo86, always happy to help!

camilamacedo86

/hold cancel

/lgtm

camilamacedo86 · 2023-03-06T19:06:16Z

/approved

rashmigottipati

/lgtm

k8s-ci-robot · 2023-03-06T19:06:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, em-r, everettraven, rashmigottipati

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]

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

camilamacedo86 · 2023-03-06T20:10:35Z

/test pull-kubebuilder-e2e-k8s-1-26-0

fix: XDG_CONFIG_HOME external plugin discovery

2e0a0fb

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 Feb 23, 2023

k8s-ci-robot requested review from everettraven and jmrodri February 23, 2023 02:13

k8s-ci-robot requested a review from camilamacedo86 February 23, 2023 02:13

camilamacedo86 reviewed Feb 23, 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 Feb 23, 2023

camilamacedo86 changed the title ~~🐛 Fix external plugin discovery on Linux~~ 🐛 (API - External Plugins): Fix external plugin discovery on Linux Feb 23, 2023

camilamacedo86 reviewed Feb 23, 2023

View reviewed changes

test: increase coverage

6ccbc0f

em-r force-pushed the linux-plugin-discovery-fix branch from f883680 to 6ccbc0f Compare February 27, 2023 02:08

everettraven approved these changes Mar 3, 2023

View reviewed changes

k8s-ci-robot assigned everettraven Mar 3, 2023

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

rashmigottipati approved these changes Mar 6, 2023

View reviewed changes

camilamacedo86 reviewed Mar 6, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2023

camilamacedo86 approved these changes Mar 6, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2023

k8s-ci-robot assigned camilamacedo86 Mar 6, 2023

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

rashmigottipati approved these changes Mar 6, 2023

View reviewed changes

k8s-ci-robot assigned rashmigottipati Mar 6, 2023

k8s-ci-robot merged commit 6adce22 into kubernetes-sigs:master Mar 6, 2023

varshaprasad96 mentioned this pull request Apr 6, 2023

[Doc] - Create doc with common suggestions and overview #3203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 (API - External Plugins): Fix external plugin discovery on Linux #3247

🐛 (API - External Plugins): Fix external plugin discovery on Linux #3247

em-r commented Feb 23, 2023

k8s-ci-robot commented Feb 23, 2023

em-r commented Feb 23, 2023

camilamacedo86 Feb 23, 2023

em-r Feb 23, 2023

camilamacedo86 Mar 6, 2023

camilamacedo86 left a comment

camilamacedo86 Feb 23, 2023

em-r commented Feb 23, 2023 •

edited

Loading

em-r commented Feb 27, 2023

everettraven left a comment

everettraven commented Mar 3, 2023

everettraven commented Mar 3, 2023

em-r commented Mar 3, 2023

camilamacedo86 commented Mar 6, 2023

em-r commented Mar 6, 2023

camilamacedo86 Mar 6, 2023

em-r Mar 6, 2023 •

edited

Loading

camilamacedo86 Mar 6, 2023

em-r Mar 7, 2023

camilamacedo86 left a comment

camilamacedo86 commented Mar 6, 2023

rashmigottipati left a comment

k8s-ci-robot commented Mar 6, 2023

camilamacedo86 commented Mar 6, 2023

		@@ -171,26 +171,44 @@ func parseExternalPluginArgs() (args []string) {
		return args

	func getPluginsRoot(host string) (pluginsRoot string, err error) {
	func getPluginsRoot(host string) {

🐛 (API - External Plugins): Fix external plugin discovery on Linux #3247

🐛 (API - External Plugins): Fix external plugin discovery on Linux #3247

Conversation

em-r commented Feb 23, 2023

k8s-ci-robot commented Feb 23, 2023

em-r commented Feb 23, 2023

camilamacedo86 Feb 23, 2023

Choose a reason for hiding this comment

em-r Feb 23, 2023

Choose a reason for hiding this comment

camilamacedo86 Mar 6, 2023

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 Feb 23, 2023

Choose a reason for hiding this comment

em-r commented Feb 23, 2023 • edited Loading

em-r commented Feb 27, 2023

everettraven left a comment

Choose a reason for hiding this comment

everettraven commented Mar 3, 2023

everettraven commented Mar 3, 2023

em-r commented Mar 3, 2023

camilamacedo86 commented Mar 6, 2023

em-r commented Mar 6, 2023

camilamacedo86 Mar 6, 2023

Choose a reason for hiding this comment

em-r Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Mar 6, 2023

Choose a reason for hiding this comment

em-r Mar 7, 2023

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Mar 6, 2023

rashmigottipati left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 6, 2023

camilamacedo86 commented Mar 6, 2023

em-r commented Feb 23, 2023 •

edited

Loading

em-r Mar 6, 2023 •

edited

Loading