📖 Plugin phase 1.5 EP #1990

Adirio · 2021-02-04T09:13:15Z

This PR proposes a plugin phase 1.5 by submiting an EP.
Phase 1.5 should provide some of the goals defined for phase 2 in a shorter date.

You can read the EP with a nicer UX that in the chenged files here.

DirectXMan12

seems pretty reasonable to me. Might want to elaborate on which each step gets each guarantee for posterity's sake, but generally looks good :-)

designs/extensible-cli-and-scaffolding-plugins-phase-1-5.md

DirectXMan12 · 2021-02-08T22:48:18Z

designs/extensible-cli-and-scaffolding-plugins-phase-1-5.md

+
+#### Post-scaffold
+This method will be used to take actions after the main scaffolding is performed, e.g. cleanup.
+NOTE: a filesystem abstraction will **NOT** be passed to this method, it should interact directly.


is the recommendation that you not interact with the filesystem here then, or is this just because naturally this takes place after KB writes stuff to disk, or both?

It was because the scaffolding is expected to be done in the previous step, post-scaffold was designed to do tasks like code generation, make targets and things like that that do not require a filesystem. Even though the current design and implementation of plugin phase 1.5 do not pass a filesystem, maybe we want to leave the option to pass a filesystem in the future if the need arises.

I updated the note saying that we may need to pass a filesystenm abstraction in a future. It is more a "We don't need it now" that a "We should not have a filesystem abstraction here".

estroz

Looks great, just one question/suggestion.

estroz · 2021-02-11T21:13:44Z

designs/extensible-cli-and-scaffolding-plugins-phase-1-5.md

+	InjectResource(*resource.Resource) error
+}
+
+type HasPreScaffold interface {


I don't see an immediate need for pre (and post, but that is covered above) scaffolding, since these can be done within Scaffold(). Perhaps remove these to simplify things, and add them back if we find a valid use case for them?

They are optional, so there is no need to use them if you want to develop a simple plugin (you can check the edit subcommands in the implementation PR), but they do offer a great flexibility. All plugins will run their pre-scaffold logic and can return an error (stoping the subcomman execution) before any of the plugins start to scaffold the files. This allows us to validate all the plugins before scaffolding. If we only had one hook (Scaffold) then every plugin will run its validation, scaffold its files and do the post-scaffolding tasks before the rest of the plugin can validate theirselves. Same applies for post-scaffolding tasks.

Let me illustrate this with a couple examples:

We have a plugin that allows us to define the fields a type will have in some yaml format that must be present before we run kb create api. The cli has the base go/v3 and this plugins configured for this project. One of the validations of this plugins is to check that the file is present and that it can be parsed. If we only have one hook, it will check this conditions after go/v3 has scaffolded its files, when it shouldn't because it failed to fulfill the second plugin requirements.

We have a plugin that adds some fields to the generated types. Again, the cli has the base go/v3 plugin and this one that edits its fields. go/v3 runs make generate at the end, but if we don't have a post-scaffold hook, it will be run before the second plugin performs the changes to the types file, and therefore be obsolete by the time the command finishes.

Thanks for the explanation, that makes sense.

answers my question as well.

estroz · 2021-02-11T21:15:23Z

Remove this hold when ready to merge.

/lgtm
/hold

estroz · 2021-02-11T22:56:29Z

/hold cancel

jmrodri

/lgtm

designs/extensible-cli-and-scaffolding-plugins-phase-1-5.md

jmrodri · 2021-02-11T23:04:56Z

designs/extensible-cli-and-scaffolding-plugins-phase-1-5.md

+
+Execution methods will be able to return an error. A specific error can be returned to specify that
+no further methods of this plugin should be called, but that the scaffold process should be continued.
+This enables plugins to exit early, e.g., a plugin that scaffolds some files only for cluster-scoped


The exit early but continue is nice.

jmrodri · 2021-02-11T23:08:14Z

designs/extensible-cli-and-scaffolding-plugins-phase-1-5.md

+	InjectResource(*resource.Resource) error
+}
+
+type HasPreScaffold interface {


answers my question as well.

k8s-ci-robot · 2021-02-11T23:08:28Z

@jmrodri: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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 · 2021-02-11T23:08:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, DirectXMan12, estroz, jmrodri

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~~ [DirectXMan12,estroz]

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

Adirio · 2021-02-11T23:13:13Z

/hold
Unitl I resolve the raised comments

Signed-off-by: Adrian Orive <[email protected]>

Adirio · 2021-02-11T23:19:04Z

/hold cancel

jmrodri · 2021-02-11T23:19:58Z

/lgtm

k8s-ci-robot · 2021-02-11T23:20:03Z

@jmrodri: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

estroz · 2021-02-11T23:20:15Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2021

k8s-ci-robot requested review from camilamacedo86 and mengqiy February 4, 2021 09:13

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 4, 2021

Adirio mentioned this pull request Feb 4, 2021

🌱 Filesystem and config persistence cleanup #1980

Closed

Adirio force-pushed the plugin-phase-1-5 branch from 95403df to 8bc3276 Compare February 4, 2021 14:49

Adirio mentioned this pull request Feb 4, 2021

⚠️ Plugin chaining #1991

Closed

DirectXMan12 approved these changes Feb 8, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2021

Adirio force-pushed the plugin-phase-1-5 branch 2 times, most recently from 467d087 to 03b7afe Compare February 9, 2021 07:08

estroz approved these changes Feb 11, 2021

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 Feb 11, 2021

k8s-ci-robot assigned estroz Feb 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021

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

jmrodri approved these changes Feb 11, 2021

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 11, 2021

Plugin phase 1.5 EP

3948289

Signed-off-by: Adrian Orive <[email protected]>

Adirio force-pushed the plugin-phase-1-5 branch from aa0b71a to 3948289 Compare February 11, 2021 23:18

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2021

k8s-ci-robot merged commit 757cc8a into kubernetes-sigs:master Feb 12, 2021

Adirio deleted the plugin-phase-1-5 branch February 12, 2021 06:12

Adirio mentioned this pull request Feb 13, 2021

Feature Request: Finalizer handler scaffolded by default for go/v3 #2010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

📖 Plugin phase 1.5 EP #1990

📖 Plugin phase 1.5 EP #1990

Adirio commented Feb 4, 2021 •

edited

Loading

DirectXMan12 left a comment

DirectXMan12 Feb 8, 2021

Adirio Feb 8, 2021

Adirio Feb 11, 2021

estroz left a comment

estroz Feb 11, 2021 •

edited

Loading

Adirio Feb 11, 2021

estroz Feb 11, 2021

jmrodri Feb 11, 2021

estroz commented Feb 11, 2021

estroz commented Feb 11, 2021

jmrodri left a comment

jmrodri Feb 11, 2021

jmrodri Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

Adirio commented Feb 11, 2021

Adirio commented Feb 11, 2021

jmrodri commented Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

estroz commented Feb 11, 2021

📖 Plugin phase 1.5 EP #1990

📖 Plugin phase 1.5 EP #1990

Conversation

Adirio commented Feb 4, 2021 • edited Loading

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

estroz Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Feb 11, 2021

estroz commented Feb 11, 2021

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

Adirio commented Feb 11, 2021

Adirio commented Feb 11, 2021

jmrodri commented Feb 11, 2021

k8s-ci-robot commented Feb 11, 2021

estroz commented Feb 11, 2021

Adirio commented Feb 4, 2021 •

edited

Loading

estroz Feb 11, 2021 •

edited

Loading