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.

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

plugins: abstract base Go plugin for easier version upgrades #1537

Closed

Conversation

estroz
Copy link
Contributor

@estroz estroz commented May 30, 2020

Because major and minor versions will be bumped often, plugin implementations should be abstracted so they're easier to update.

  • pkg/plugin/internal/v0: Init, CreateAPI, and CreateWebhook plugins in the state at which scaffolds/CLI was when plugins were implemented. These "base" plugins can be selectively used by versioned plugins.
  • pkg/plugin/v0: Go plugin implementation v0.0. Used only for project version 2.
  • pkg/plugin/v2: Go plugin implementation v2.x. Used for project versions 3-alpha and beyond.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: estroz
To complete the pull request process, please assign mengqiy
You can assign the PR to them by writing /assign @mengqiy in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 30, 2020
@estroz estroz force-pushed the chore/abstract-base-plugin branch 2 times, most recently from 85aedd5 to 0f88648 Compare May 31, 2020 22:18
…ntations

should be abstracted so they're easier to update.

pkg/plugin/internal/v0: Init, CreateAPI, and CreateWebhook plugins in the
state at which scaffolds/CLI was when plugins were implemented.
These "base" plugins can be selectively used by versioned plugins.

pkg/plugin/v0: Go plugin implementation v0.0. Used only for project
version 2.

pkg/plugin/v2: Go plugin implementation v2.x. Used for project versions
3-alpha and beyond.

pkg/plugin/validation.go: add helpful error types.
@estroz estroz force-pushed the chore/abstract-base-plugin branch from 0f88648 to acb4d76 Compare May 31, 2020 22:21
@estroz estroz changed the title [WIP] plugins: abstract base Go plugin for easier version upgrades plugins: abstract base Go plugin for easier version upgrades May 31, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2020
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @estroz,

Could you please clarify what is the problem that are you trying to solve with this change? Sorry, but it is not clear to me why we need to rename v2.

Also, IMO we should NOT rename or change the plugin v2 for v0 or base. See :

  • v1 -> Project layout v1
  • v2 -> Project layout v2
  • v3 -> Project layout v3 (where the plugins started to be in place )

To keep clear the concept adopted so far:

Note that in the case of the v1 and v2 PROJECT version, a corresponding plugin version is implied and constant -- v1 implies the go.kubebuilder.io/v1 plugin, and similarly for v2. This is for legacy purposes -- no such implication is made with the v3 PROJECT version.
From @DirectXMan12 comment in #1528 (comment).

So, for me replace v2 for V0 will NOT make easier or more intuitive the understatement as described in #1537 (comment)

@estroz
Copy link
Contributor Author

estroz commented Jun 1, 2020

@camilamacedo86 package names reflect plugin version, not project version. v0 is the state of scaffolding/CLI when plugins were created and is the "base" plugin that should be built on top of and the plugin to be used for project version 2 which does not "know" about plugins (no layout key).

Is this package structure not easier to create new plugin versions with than the current structure?

Edit: in the past we had pkg/plugin/{v1,v2} which were related to both project and plugin version. However I don't think linking package name to project version, at least with this naming scheme, makes sense because a plugin can support multiple project versions. Instead, there should either be packages named by plugin version or a single pkg/plugin/go containing all plugin implementations.

@DirectXMan12
Copy link
Contributor

@estroz can you clarify versioning of the go plugin then a bit? I had originally envisioned that project: v1 ~ project: v3, layout: go/v1 and project: v2 ~ project: v3, layout: go/v2 (hand-waving a bit here, but take ~ to mean "equivalent if that were possible")

@k8s-ci-robot
Copy link
Contributor

@estroz: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2020
@estroz
Copy link
Contributor Author

estroz commented Jun 1, 2020

@DirectXMan12 that's how I see the pre-v3 relation as well. My goal here is to enable easy re-use of the frozen project version 2 plugin (v2.0) so that we don't have to duplicate the whole thing for something like #1498. The way I have packages set up might not be the right way to do so.

I also think we should consider not packaging plugins by project version, but given the above relation it might make some sense to continue doing so. We'd eventually have plugins v(3+).Y packaged in pkg/plugin/v3.

@camilamacedo86
Copy link
Member

Hi @estroz,

I understand what you are trying to achieve. See that in #1498 the plugin/v2 are == plugin/v3 however, could it not become an impedivie for customizations into the plugins? For example, with the idea proposed here how can we add a new plugin (SpecialPluginGetter) valid just for V3?

Also, see:

  • plugin/v2 - contains the plugin source code based in the v2 project scaffold
  • plugin/v3 - contains the plugin source code valid for V3 projects (where we will do the bumps Plugin V3+N)

And then one day we will be able to remove v2, however, until there we need to make clear what is or not the code used by v2 projects since we cannot change it.

Also, see that we have other places in the project, for example, the tests which follow the current structure and concepts adopted already:

  • tests/v2/ -> test V2 projects with v2 plugins
  • tests/v3/ -> test V3 projects with v3+N plugins

Screenshot 2020-06-02 at 00 01 33

Regards My goal here is to enable easy re-use of the frozen project version 2 plugin (v2.0) so that we don't have to duplicate

As you said in another comment/pr the V2 plugins is a very special scenario. So, I am OK to "duplicate" it for V3 now since it will not be repeated after that and then, the implementation will likely be different in the future.

So, I still think that we should not move forward with.

@estroz
Copy link
Contributor Author

estroz commented Jun 2, 2020

We can punt on this for now, and re-address when we start changing things after #1498.

@estroz estroz closed this Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants