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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Mar 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e0a0fb
fix: XDG_CONFIG_HOME external plugin discovery
em-r Feb 22, 2023
6ccbc0f
test: increase coverage
em-r Feb 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions pkg/cli/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,26 +171,44 @@ func parseExternalPluginArgs() (args []string) {
return args
Copy link
Member

Choose a reason for hiding this comment

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

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

}

// isHostSupported checks whether the host system is supported or not.
func isHostSupported(host string) bool {
for _, platform := range supportedPlatforms {
if host == platform {
return true
}
}
return false
}

// getPluginsRoot detects the host system and gets the plugins root based on the host.
func getPluginsRoot(host string) (pluginsRoot string, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

/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

Copy link
Contributor Author

@em-r em-r Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @camilamacedo86, always happy to help!

if !isHostSupported(host) {
// freebsd, openbsd, windows...
return "", fmt.Errorf("host not supported: %v", host)
}

pluginsRelativePath := filepath.Join("kubebuilder", "plugins")
if xdgHome := os.Getenv("XDG_CONFIG_HOME"); xdgHome != "" {
return filepath.Join(xdgHome, pluginsRelativePath), nil
}

switch host {
case "darwin":
logrus.Debugf("Detected host is macOS.")
pluginsRoot = filepath.Join("Library", "Application Support", "kubebuilder", "plugins")
pluginsRoot = filepath.Join("Library", "Application Support", pluginsRelativePath)
case "linux":
logrus.Debugf("Detected host is Linux.")
pluginsRoot = filepath.Join(".config", "kubebuilder", "plugins")
default:
// freebsd, openbsd, windows...
return "", fmt.Errorf("Host not supported: %v", host)
pluginsRoot = filepath.Join(".config", pluginsRelativePath)
}
userHomeDir, err := getHomeDir()

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

return pluginsRoot, nil
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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

return
}

// DiscoverExternalPlugins discovers the external plugins in the plugins root directory
Expand Down Expand Up @@ -286,16 +304,3 @@ func DiscoverExternalPlugins(fs afero.Fs) (ps []plugin.Plugin, err error) {
func isPluginExectuable(mode fs.FileMode) bool {
return mode&0111 != 0
}

// getHomeDir returns $XDG_CONFIG_HOME if set, otherwise $HOME.
func getHomeDir() (string, error) {
var err error
xdgHome := os.Getenv("XDG_CONFIG_HOME")
if xdgHome == "" {
xdgHome, err = os.UserHomeDir()
if err != nil {
return "", err
}
}
return xdgHome, nil
}
32 changes: 30 additions & 2 deletions pkg/cli/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ var _ = Describe("Discover external plugins", func() {

_, err = getPluginsRoot("random")
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("Host not supported"))
Expect(err.Error()).To(ContainSubstring("host not supported"))
})

It("should skip parsing of directories if plugins root is not a directory", func() {
Expand All @@ -249,7 +249,35 @@ var _ = Describe("Discover external plugins", func() {

_, err = getPluginsRoot("random")
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("Host not supported"))
Expect(err.Error()).To(ContainSubstring("host not supported"))
})

It("should return full path to the external plugins without XDG_CONFIG_HOME", func() {
if _, ok := os.LookupEnv("XDG_CONFIG_HOME"); ok {
err = os.Setenv("XDG_CONFIG_HOME", "")
Expect(err).To(BeNil())
}

home := os.Getenv("HOME")

pluginsRoot, err := getPluginsRoot("darwin")
Expect(err).To(BeNil())
expected := filepath.Join(home, "Library", "Application Support", "kubebuilder", "plugins")
Expect(pluginsRoot).To(Equal(expected))

pluginsRoot, err = getPluginsRoot("linux")
Expect(err).To(BeNil())
expected = filepath.Join(home, ".config", "kubebuilder", "plugins")
Expect(pluginsRoot).To(Equal(expected))
})

It("should return full path to the external plugins with XDG_CONFIG_HOME", func() {
err = os.Setenv("XDG_CONFIG_HOME", "/some/random/path")
Expect(err).To(BeNil())

pluginsRoot, err := getPluginsRoot(runtime.GOOS)
Expect(err).To(BeNil())
Expect(pluginsRoot).To(Equal("/some/random/path/kubebuilder/plugins"))
})

It("should return error when home directory is set to empty", func() {
Expand Down
4 changes: 4 additions & 0 deletions pkg/cli/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const (
projectVersionsHeader = "Supported project versions"
)

var (
supportedPlatforms = []string{"darwin", "linux"}
)

func (c CLI) newRootCmd() *cobra.Command {
cmd := &cobra.Command{
Use: c.commandName,
Expand Down