-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
feature: implement plugin proposal in #1250 #1290
feature: implement plugin proposal in #1250 #1290
Conversation
@estroz: GitHub didn't allow me to request PR reviews from the following users: Adirio. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
pkg/cli/cli.go
Outdated
func New(opts ...Option) (*CLI, error) { | ||
c := &CLI{ | ||
commandName: "kubebuilder", | ||
defProjVersion: project.Version2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the seeming consensus around name and version, we may need to re-think the --project-version
flag and this defProjVersion
field.
If we care about backward compatibility, we could keep and deprecate the --project-version
flag, and make it automatically choose the "go" plugins. Maybe we don't care since --project-version
is used during project initialization only (I think?).
We probably need a new init
flag, maybe --plugin
, that takes something like <name>/<version>
. So for the existing kubebuilder plugins, it would understand go/v1
and go/v2
.
Lastly, do we want to have a convention to make it easier to distinguish similar plugins from different authors? Consider two different plugins named foo
. Should we include a domain component (e.g. foo.kubebuilder.io
and foo.operator-framework.io
)? If so, this field starts to look a lot like a Kubernetes group version string (maybe more familiar for users) and it would make it more apparent which binary a given KB project would require. One thing I don't like about this is that it requires more typing from users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having name + version for a plugin changes how plugins are accessed, since you can have multiple names for the same plugin version. This is fine and desired, ex. helm/v1
and go/v1
should return different plugins. Name + version needs to be recorded in PROJECT
, so after running init
the --plugin
/--project-version
flag is no longer needed.
@DirectXMan12 @droot thoughts on ^?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda imagined it'd go like this:
kubebuilder init --project-version v{1,2}
are mostly an alias forkubebuilder init --plugin go:v{1,2}
(see below)kubebuilder init --plugin NAME:VERSION
(orNAME/VERSION
) initializes a project with the given plugin, writing that to the project file with the special project version of3x
or something. We might want special handling if you actually used--project-version
to write1
or2
- After
init
, you never specify project version or base plugin again
As for suffixes, might not be a bad idea to encourage it, but I don't expect it'll be a big issue (famous last words) and it is a lot of extra typing. Maybe allow it as a disambiguation form, but don't require it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DirectXMan12 Why do you want to upgrade to v3x
? I think we can acheive this being fully compatible with v1
and v2
so we wouldn't need to upgrade to v3
as the scaffold won't be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was at least that 3x would indicate to aware things that plugins are involved -- if you try to work with a project scaffolded with a particular set of plugins and aren't aware, things could go awry.
We might want to punt on that for later, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial pass. A few questions. Doesn't look too bad. The hacks fancy footwork working around cobra doesn't make me feel warm and fuzzy inside, but that what's hot cocoa is for I guess.
pkg/cli/cli.go
Outdated
// empty string and true. Otherwise, getBaseFlags returns the project version | ||
// and false. | ||
func (c CLI) getBaseFlags() (string, bool) { | ||
fs := pflag.NewFlagSet("base", pflag.ExitOnError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this logic yields "unknown flag: project-version" when we get to the main flag parsing logic. We prob want this defined on the main command too.
pkg/cli/init_project.go
Outdated
|
||
// If only the help flag was set, return the command as is. | ||
if isHelpOnly { | ||
return cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this mean that a plugin doesn't get to modify help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent with this was to handle kubebuilder init --help
, in which case we could print out a general help. In this case, we don't know which plugin to use for help output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have something to do with the use of log.Fatal
where you were using cdmErr
and return
in create api and webhook commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite complete review: I requested some changes, suggested others, and asked some doubts. Hope it helps.
By the way, this needs a rebase as https://travis-ci.org/kubernetes-sigs/kubebuilder/jobs/635475321#L637-L644 was solved as a side effect of #1287 (more info in #1288) a couple weeks ago. The rebase will include multiple new lint checks aside from those that are already yielding errors in the current state (https://travis-ci.org/kubernetes-sigs/kubebuilder/jobs/635475321#L253-L350).
pkg/cli/cli.go
Outdated
func New(opts ...Option) (*CLI, error) { | ||
c := &CLI{ | ||
commandName: "kubebuilder", | ||
defProjVersion: project.Version2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DirectXMan12 Why do you want to upgrade to v3x
? I think we can acheive this being fully compatible with v1
and v2
so we wouldn't need to upgrade to v3
as the scaffold won't be modified.
pkg/cli/cli.go
Outdated
// Run runs the CLI. | ||
func (c CLI) Run() error { | ||
return c.cmd.Execute() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Is Run
a good method name in this case? I would say that the user executes or runs a command but we are building the CLI. Some ideas (brainstorming): Build
, Deploy
, Make
, Create
, Generate
, Finish
, End
. I think I like Deploy
the most.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not Run
, Execute
would make sense. The CLI isn't really deploying anything, ex. resources, it is a program that is running/executing.
pkg/plugin/v1/plugin.go
Outdated
} | ||
|
||
func (_ Plugin) Version() string { | ||
return "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is anyone else concerned about versions as strings and the ability to put anything here?
I guess this could be a feature so you could do init --plugin Foo:Bar
to have a composable set of plugins by "version" but the same overall plugin? If that is the use case maybe this is not a version field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that may rise a lot of issues as we are expecting the default go
plugin to exist for every version and using Bar
as version would break that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version formatting and plugin composition go hand-in-hand, so we should follow up this PR with a discussion on both. For now we could enforce _, err := strconv.Atoi(p.Version()); err != nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. I think for the moment: add a TODO
for this, and we'll track them all down before final merge.
We can do the "move fast and break things" strategy on this branch.
Overall I think that this good some points: I think we need to discuss the version more. Want to see what folks think about the default for help will show the plugins that can be used as well as the default flags and such? Is that possible or is that already working and I just can't see it? |
@estroz merging Master wasn't the best idea IMHO. I think you need to rebase your branch to Master, 257 files changed, and 10k lines modified seem to be a bit too much. |
@Adirio its because the target feature branch isn't up to date with master. It needs to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its because the target feature branch isn't up to date with master. It needs to be updated.
Oh, right, you are not PR-ing against master. Reviewing right now is quite complex. Would it be better if we change the target branch to master until this is ready to ship, then update the current target branch to that point and swap again this PR to point to it?
pkg/cli/alpha.go
Outdated
Example: fmt.Sprintf(` | ||
# scaffolds webhook server | ||
%s alpha webhook <params> | ||
`, c.commandName), | ||
%s alpha webhook <params>`, c.commandName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: From an style point of view I may like more
Example: fmt.Sprintf(
`
# scaffolds webhook server
%s alpha webhook <params>`,
c.commandName,
),
I feel like using backquotes vs \n
helps visualizing how it will look like but messes with the indentation. At least in this case we only mess with the indentation of the text itself, the params are well indented.
P.S.: applies to several other parts of the code.
pkg/plugin/v2/plugin.go
Outdated
"sigs.k8s.io/kubebuilder/pkg/plugin/v1" | ||
v1 "sigs.k8s.io/kubebuilder/pkg/plugin/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is because it expects packages v1
and v2
to be different versions of the pkg/plugin
package. It made me think that we may want to include the plugin name in the package name, either pkg/plugin/govX
or pkg/plugin/go/vX
. The second will still be interpreted as a version but this time of pkg/plugin/go
which would be technichally correct.
@Adirio if we can update the target feature branch with master, we should be ok since we don't need to merge CI fixes going forward. The only following merge will be your changes in #1325. @DirectXMan12 @droot @mengqiy I don't have access to the feature branch. Can you merge it with master? |
I think that #1330 may also be a good idea to merge before, as it targets |
Hey, sorry about that. I'll fast-forward as needed. Feel free to ping me on Slack if I'm not responding here. |
should be up-to-date now |
please do pull out the merge commit though |
approved or almost approved a couple of those PRs from @Adirio. LMK if there's another one blocking, but I think if we rebase on that we can probably merge this as a "here's a working step" and keep iterating. |
975a57e
to
3045ba7
Compare
420b194
to
b35f34f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NITs: a function movement suggestion and some non-needed variables removals.
Otherwise, LGTM.
cmd/main.go
Outdated
func main() { | ||
if err := buildCmdTree().Execute(); err != nil { | ||
log.Fatal(err) | ||
func loadExistingConfig() (*config.Config, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this to internal/config
and calling it MustLoad
?
The subcommands under plugin would also be able to use this function if we move it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must
implies that the function exits if the operation fails. Is this the behavior you intend MustLoad
to have? Otherwise LoadExisting
makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must usually means that errors will be handled with a panic, I guess another name could be better. But moving it there seems like a good thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IHMO: it still has a hall to changes and improvements. However, it will also easier to be done properly during and after the phase2 of its implementation. So, it shows ok for me to attend as first step the proposal made which is the goal of this PR.
@mengqiy @DirectXMan12 wdyt?
/lgtm
These changes are the implementation of concepts outlined in designs/extensible-cli-and-scaffolding-plugins-phase-1.md * cmd/: remove all but 'edit' and 'update' subcommands in favor of plugins that implement removed commands * internal/cmdutil: RunOptions configure Run() to execute a command in a generic manner * pkg/cli: CLI interface and cobra CLI constructors. Most commands from cmd/ were move here and modified to initialize plugins * pkg/plugin: Plugin and PluginGetter interfaces for v1 and v2 scaffolds. These are kubebuilider's base Go plugins consumed by pkg/cli.CLI
a3c93fe
to
358a9f6
Compare
Should not it be an issue in https://github.com/kubernetes-sigs/kubebuilder/projects/7? as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
/ok-to-test |
"sigs.k8s.io/kubebuilder/internal/config" | ||
"sigs.k8s.io/kubebuilder/pkg/scaffold" | ||
"sigs.k8s.io/kubebuilder/pkg/cli" | ||
pluginv1 "sigs.k8s.io/kubebuilder/pkg/plugin/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IHMO: We should not care about V1 since it is deprecated and should no longer be used.
Also, we need to remove the V1 code in order to keep things simple.
However, since it is already done in the PR we can merge with. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing v1 may be easier after merging the feature branch with master. @DirectXMan12 @mengqiy thoughts on this? Regardless, out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to track here: We have a PR for that already: #1355
c.projectVersion = projectConfig.Version | ||
} else { | ||
return fmt.Errorf("failed to read config: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit. Suggestion ( I think make more clear when has an error and not what should be done )
if err != nil {
if os.IsNotExist(err) {
c.configured = false
c.projectVersion = c.defaultProjectVersion
} else {
return fmt.Errorf("failed to read config: %v", err)
}
} else {
c.configured = true
c.projectVersion = projectConfig.Version
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is less clear IMO.
tmpGetter, isGetter := p.(plugin.CreateAPIPluginGetter) | ||
if isGetter { | ||
if hasGetter { | ||
err := fmt.Errorf("duplicate API creation plugins for project version %q: %s, %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not the API be called/used more than once to create different resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each create
command is associated with exactly one versioned plugin, otherwise plugin behavior can conflict. If you want to create more than one resource, call create api
for each resource.
tmpGetter, isGetter := p.(plugin.CreateWebhookPluginGetter) | ||
if isGetter { | ||
if hasGetter { | ||
err := fmt.Errorf("duplicate webhook creation plugins for project version %q: %s, %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use/call the webhook just once as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my create api
comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @estroz,
Just a few questions and nits, otherwise it shows ok for me as phase 1 and I still thinking that will be easier we check more changes and improvements after phase 2 and its implementations.
However, I have not seen the prow jobs called here. So, /lgtm cancel
Could you please check?
/lgtm cancel |
This is being merged in a branch that is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really minor (non-blocking) nits. Fix them in a follow-up.
Are we doing the "layout" field in a follow-up?
/lgtm
/approve
Run: func(_ *cobra.Command, _ []string) { | ||
if err := run(options); err != nil { | ||
Example: fmt.Sprintf(` # Update the vendor dependencies: | ||
kubebuiler update vendor`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this still fmt.Sprintf
return &cobra.Command{ | ||
Use: "alpha", | ||
Short: "Expose commands which are in experimental or early stages of development", | ||
Long: `Command group for commands which are either experimental or in early stages of development`, | ||
Example: fmt.Sprintf(` | ||
# scaffolds webhook server | ||
%s alpha webhook <params>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opposite side of same question here
), | ||
} | ||
|
||
// Lookup the plugin for projectVersion and bind it to the command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment correct any more?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, estroz 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:
Approvers can indicate their approval by writing |
e322071
into
kubernetes-sigs:feature/plugins-part-2-electric-boogaloo
Implements #1250 except for version format enforcement and plugin name resolution (will be done in follow-ups).
TODO:
finalize plugin interface setdiscuss pro's/con's of this implementation, smooth over rough edges/cc @DirectXMan12 @droot @Adirio @joelanford @shawn-hurley @camilamacedo86