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

✨ external plugins(plugin phase 2): add support to pass CLI flags #2693

Merged
merged 1 commit into from
Jun 21, 2022
Merged

✨ external plugins(plugin phase 2): add support to pass CLI flags #2693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions pkg/cli/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,18 @@ func WithCompletion() Option {

everettraven marked this conversation as resolved.
Show resolved Hide resolved
// parseExternalPluginArgs returns the program arguments.
func parseExternalPluginArgs() (args []string) {
args = make([]string, len(os.Args)-1)
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see this scenarios tested with e2e. Example:

By having the sample plugin here and calling them as we do in the others e2e tests.
So that we can ensure the expected results, which would make it simple, we review the changes.

could we try to work on that ? WDYT @rashmigottipati @everettraven ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@camilamacedo86 I'll work on a separate PR to add e2e test with the python external plugin I've built. But I wouldn't make e2e tests as a part of this PR, as the goal of this PR is to enable CLI flags for external plugins.

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 have the e2e tests, we would add a new test/check to ensure that it works.

That would be adding a new call passing the flags and checking the result that value was used in the mock plugin. All prs must be covered with tests and any new feature that we add will need to be covered on the e2e afterwords. ( in this another PR ) So, the best would be if we have it now.

However, to move forward with this one ( without the e2e tests ) could we add in the description of this PR the tests done locally with the results showing that it works as well? Would that be possible? WDYT @rashmigottipati @everettraven ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to go through and add as a comment to this thread the reasoning for this change with proof when running it locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using the original method (adding a print statement to print the args) with the changes that pass the CLI flags to an external plugin, running kubebuilder init --plugins myexternalplugin/v1 --domain example.com results in this error output:

[init --plugins myexternalplugin/v1 --domain example.com]
INFO[0000] Adding external plugin: myexternalplugin     
usage: myexternalplugin.py [-h] [--domain DOMAIN] [--license LICENSE]
                           [--boolean | --no-boolean]
myexternalplugin.py: error: unrecognized arguments: init --plugins myexternalplugin/v1
Error: failed to initialize project: unable to scaffold with "myexternalplugin/v1": exit status 2
Usage:
  kubebuilder init [flags]

Examples:
  # Help for initializing a project with version "2"
  kubebuilder init --project-version="2" -h

  # Help for initializing a project with version "3"
  kubebuilder init --project-version="3" -h

Flags:
      --boolean                  flag to test boolean logic
      --domain string            Project domain (default "my.domain")
      --float float              flag to test float logic (default 123.45)
  -h, --help                     help for init
      --integer int              flag to test integer logic (default 123)
      --license string           Project license (default "apache2")
      --project-version string   project version (default "3")

Global Flags:
      --plugins strings   plugin keys to be used for this subcommand execution

2022/06/01 09:18:05 failed to initialize project: unable to scaffold with "myexternalplugin/v1": exit status 2

The important line to note in that error message is:

myexternalplugin.py: error: unrecognized arguments: init --plugins myexternalplugin/v1

This tells us that we are parsing all arguments after the program name itself, including subcommands and flags that are likely only to be used with Kubebuilder (--plugins flag specifically). We can see this more so from the print statement that I added for testing here:

[init --plugins myexternalplugin/v1 --domain example.com]

The modified function ensures that we only parse flags and the corresponding flag values. It also ensures that we do not parse the --plugins flag since it is likely only used with Kubebuilder.

With the new changes the output of the same command is:

[--domain example.com]
INFO[0000] Adding external plugin: myexternalplugin

There is no error since the proper flags are passed to the external plugin.

@camilamacedo86 @rashmigottipati I hope this helps demonstrate the reason for the change!

copy(args, os.Args[1:])
// Loop through os.Args and only get flags and their values that should be passed to the plugins
// this also removes the --plugins flag and its values from the list passed to the external plugin
for i := range os.Args {
if strings.Contains(os.Args[i], "--") && !strings.Contains(os.Args[i], "--plugins") {
args = append(args, os.Args[i])

// Don't go out of bounds and don't append the next value if it is a flag
if i+1 < len(os.Args) && !strings.Contains(os.Args[i+1], "--") {
args = append(args, os.Args[i+1])
}
}
}

return args
}
Expand Down
37 changes: 37 additions & 0 deletions pkg/cli/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,43 @@ var _ = Describe("Discover external plugins", func() {

})
})

Context("parsing flags for external plugins", func() {
It("should only parse flags excluding the `--plugins` flag", func() {
// change the os.Args for this test and set them back after
oldArgs := os.Args
defer func() { os.Args = oldArgs }()
os.Args = []string{
"kubebuilder",
"init",
"--plugins",
"myexternalplugin/v1",
"--domain",
"example.com",
"--binary-flag",
"--license",
"apache2",
"--another-binary",
}

args := parseExternalPluginArgs()
Expect(args).Should(ContainElements(
"--domain",
"example.com",
"--binary-flag",
"--license",
"apache2",
"--another-binary",
))

Expect(args).ShouldNot(ContainElements(
"kubebuilder",
"init",
"--plugins",
"myexternalplugin/v1",
))
})
})
})

var _ = Describe("CLI options", func() {
Expand Down
25 changes: 25 additions & 0 deletions pkg/plugin/external/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,29 @@ type PluginResponse struct {

// ErrorMsgs contains the specific error messages of the plugin failures.
ErrorMsgs []string `json:"errorMsgs,omitempty"`

// Flags contains the plugin specific flags that the plugin returns to Kubebuilder when it receives
// a request for a list of supported flags from Kubebuilder
Flags []Flag `json:"flags,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Would possible here to extend the cobra dep Flag so that we would allow the same options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a look at doing this and it seems like the way to determine the type of the flag is an interface. I think that structure would make it difficult for external plugin authors to be able to specify the type of the flag, and the new Flag struct introduced below makes it a bit easier IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For more details on it, here is the godocs for the pflag library that Kubebuilder uses for flags: https://pkg.go.dev/github.com/spf13/pflag

}

// Flag is meant to represent a CLI flag that is used by Kubebuilder to define flags that are parsed
// for use with an external plugin
type Flag struct {
// Name is the name that should be used when creating the flag.
// i.e a name of "domain" would become the CLI flag "--domain"
Name string

// Type is the type of flag that should be created. The types that
// Kubebuilder supports are: string, bool, int, and float.
// any value other than the supported will be defaulted to be a string
Type string

// Default is the default value that should be used for a flag.
// Kubebuilder will attempt to convert this value to the defined
// type for this flag.
Default string

// Usage is a description of the flag and when/why/what it is used for.
Usage string
}
6 changes: 6 additions & 0 deletions pkg/plugins/external/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package external

import (
"github.com/spf13/pflag"

"sigs.k8s.io/kubebuilder/v3/pkg/machinery"
everettraven marked this conversation as resolved.
Show resolved Hide resolved
"sigs.k8s.io/kubebuilder/v3/pkg/model/resource"
"sigs.k8s.io/kubebuilder/v3/pkg/plugin"
Expand All @@ -39,6 +41,10 @@ func (p *createAPISubcommand) InjectResource(*resource.Resource) error {
return nil
}

func (p *createAPISubcommand) BindFlags(fs *pflag.FlagSet) {
bindExternalPluginFlags(fs, "api", p.Path, p.Args)
}

func (p *createAPISubcommand) Scaffold(fs machinery.Filesystem) error {
req := external.PluginRequest{
APIVersion: defaultAPIVersion,
Expand Down
6 changes: 6 additions & 0 deletions pkg/plugins/external/edit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package external

import (
"github.com/spf13/pflag"

everettraven marked this conversation as resolved.
Show resolved Hide resolved
"sigs.k8s.io/kubebuilder/v3/pkg/machinery"
"sigs.k8s.io/kubebuilder/v3/pkg/plugin"
"sigs.k8s.io/kubebuilder/v3/pkg/plugin/external"
Expand All @@ -29,6 +31,10 @@ type editSubcommand struct {
Args []string
}

func (p *editSubcommand) BindFlags(fs *pflag.FlagSet) {
bindExternalPluginFlags(fs, "edit", p.Path, p.Args)
}

func (p *editSubcommand) Scaffold(fs machinery.Filesystem) error {
req := external.PluginRequest{
APIVersion: defaultAPIVersion,
Expand Down
Loading