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

⚠️ [Rename] Disambiguate plugin by calling subcommand what the plugin getters returns instead of plugin #1748

Merged

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Oct 28, 2020

Description

Rename plugin getters return objects to Subcommand.

Motivation

Currently, both the structs with the getters and those returned by them are called "plugin" (Plugin -> Plugin), leading to missunderstandings. In order to clarify which is which, one of them should be renamed. There are two possibilities:

  1. Rename the struct with the getters to something like PluginFactory: PluginFactory -> Plugin
  2. Rename the returned type to something like Subcommand: Plugin -> Subcommand

The word "plugin" is already extensively used in the code (comments, variable names, ...) to refer to the left term, so I figured that it would be easier to go for the second option.

When deciding which name to use, I saw how the base class for the right term structs was already GenericSubcommand and it basically represents that, with methods to bind flags and execute the command, so I opted for Subcommand.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 28, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Oct 28, 2020

@estroz We talked about this yesterday, what do you think of this approach?

@Adirio Adirio force-pushed the plugin-subcommand-concept branch 2 times, most recently from 0416c10 to 536676c Compare October 28, 2020 09:45
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 @Adirio,

The plugins design whcih was introduced following the concept of a Plug-in Architecture. Then, it means that operations can be extending whcih allows other tools to use 1..N plugins as also they have its own plugins.

Then, the next step that we have been discussed is the plugin phase 2 which has the purpose to allow us to aggregate and/or customize the plugins. For example, we might have a plugin to it the project that will do a scaffold with a layout A and also another one which will create a project with a layout B and both can be used.

See here how the SDK CLI call all plugins currently. Note that in the PROJET file we write all plugins that were used to generate the files. (currently, we have just: go, ansible., helm). For a further understand and follow up this WIP see; #1378. Note that an EP will be raised based on the discussions made on this issue as well.

In this way, I'd like to recommend you first raise an EP and/or at least an issue where you will describe: what is the problem with the current design that you would like to solve? What are the user stories that you would like to address E.g As a <role> I can <capability>, so that <receive benefit>? Describe the technical details with examples of usage as well whcih clarifies your motivations and ideas?

By looking at the PR shows that you are changing the plugins for each one be describe as subcommand then, it shows that goes against the purpose and architecture adopted for it. However, your motivations might not be very clear to me. So, could you please raise this issue and provide the info described above?

@Adirio
Copy link
Contributor Author

Adirio commented Oct 28, 2020

Hi @camilamacedo86,

This PR follows the same design you mention, it is just a rebranding or name change.

To refer to these names I'm going to use the X -> Y notation, where the term in the left (X) is the struct that has the GetXxxxxPlugin methods and the term in the right (Y) is the struct returned by these methods.

Right now, both are called Plugin (Plugin -> Plugin). I was talking with @estroz and having both with the same name leads to missunderstandings, so we thought that one of them should be renamed (just changing the name, the implementation and design behind it stays the same).

There are two options:

  1. Changing the left term: SomethingElse -> Plugin
  2. Changing the right term: Plugin -> SomethingElse

The word "plugin" is already extensively used in the code (comments, variable names, ...) to refer to the left term, so I figured that it would be easier to go for the second option (Plugin -> SomethingElse).

When deciding which name to use, I saw how the base class for the right term structs was already GenericSubcommand and it basically represents that, with methods to bind flags and execute the command, so I opted for Subcommand (Plugin -> Subcommand).

Is the motivation of these change (just renaming) clearer now?

P.S.: during the offline discussion with @estroz, the possibility of using PluginFactory -> Plugin was also mentioned.

Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

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

These name changes make sense to me on a high level. Haven't thoroughly gone through this PR yet.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 28, 2020

Hi @Adirio,

So, by reading your comment in #1748 (comment) I understand that you are doing a "cleanup" with some renames then, could you please update the first comment to clarifies this PR? Note that, Add the subcommand concept to represent the actual operations returned by plugins. lead to belive it introduces a new design to the cli when you are doing renames mainly only. I mean, what means Add the subcommand concept? May something such as;

Description
// Add a summary of what was done here specifically. E.g

  • Rename .. to ...
  • add validation for ...

Motivation
// Why this change is required? What are the problems that are solved with it? User story usually is helpful as well to get the motivation.

@camilamacedo86 camilamacedo86 dismissed their stale review October 28, 2020 17:51

shows that it mainly renames so I need to check it further now that the its motivation is clarified

@Adirio Adirio changed the title ✨ Add the subcommand concept ✨ [Rename] Disambiguate plugin by calling subcommand what the plugin getters returns instead of plugin Oct 29, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Oct 29, 2020

@camilamacedo86 Title, description and motivation updated.

@Adirio Adirio changed the title ✨ [Rename] Disambiguate plugin by calling subcommand what the plugin getters returns instead of plugin ⚠️ [Rename] Disambiguate plugin by calling subcommand what the plugin getters returns instead of plugin Oct 30, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2020
@Adirio Adirio closed this Nov 3, 2020
@Adirio Adirio reopened this Nov 3, 2020
@Adirio Adirio closed this Nov 3, 2020
@Adirio Adirio reopened this Nov 3, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Nov 4, 2020

Ping @camilamacedo86 and @estroz

pkg/plugin/interfaces.go Outdated Show resolved Hide resolved
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Looks good, just one nit about naming.

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.

It shows great for me now either 👍
just address the @estroz nit #1748 (review) for we are able to get it merged

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2020
Previously, both the getters and the execution objects were called plugins, leading to missunderstandings

Signed-off-by: Adrian Orive <[email protected]>
@estroz
Copy link
Contributor

estroz commented Nov 4, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020
@Adirio Adirio closed this Nov 5, 2020
@Adirio Adirio reopened this Nov 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit a3e0baf into kubernetes-sigs:master Nov 5, 2020
@Adirio Adirio deleted the plugin-subcommand-concept branch November 5, 2020 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants