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

🌱 Plugin resolution enhancement proposal #1942

Closed

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Jan 13, 2021

Enhance the plugin resolution algorithm to simplify the user experience and allow plugins to be defined by command call.

Implementation: #2019

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Adirio
To complete the pull request process, please assign estroz after the PR has been reviewed.
You can assign the PR to them by writing /assign @estroz 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

@Adirio Adirio marked this pull request as draft January 13, 2021 14:06
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2021

1. [All commands but Init] Obtain the project version and plugin keys from the project configuration if available.
* If they are available, plugin keys will be fully qualified.
2. [Init command only] Obtain the project version from `--project-version` flag.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the advantages implementation-wise of this new approach is that --project-version stops being a global variable for all commands that need to be special cased. It will just be a local variable of the Init command.

@Adirio Adirio mentioned this pull request Jan 19, 2021
15 tasks
@Adirio Adirio mentioned this pull request Feb 16, 2021
1 task
@Adirio Adirio marked this pull request as ready for review February 17, 2021 08:42
@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 Feb 17, 2021
4. Qualify the unqualified keys from step 3.
* If we know the project version, use this info to filter the available plugins so that we can be more accurate.
5. Resolve the plugins with the list of qualified keys above.
6. [Init command only] Resolve the project version in case it wasn't provided and there is only one project version
Copy link
Contributor

@estroz estroz Mar 11, 2021

Choose a reason for hiding this comment

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

What if two default plugins support the same project version? Which will be chosen?

Copy link
Contributor

@estroz estroz Mar 11, 2021

Choose a reason for hiding this comment

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

Actually this doesn't seem to be a problem if default plugins are forced to have a disjoint set of supported project versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, I may want to specify a plugin for a particular project version, not just the project versions it supports. These changes take my ability to do that away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if two default plugins support the same project version? Which will be chosen?

This proposal takes into account that we will support plugin chains. Having multiple plugins set as default means that it is a default chain. This proposal removes the ability to provide different plugin/plugin-chains for each project version, but I guess we could try to keep this behavior.

1. [All commands but Init] Obtain the project version and plugin keys from the project configuration if available.
* If they are available, plugin keys will be fully qualified.
2. [Init command only] Obtain the project version from `--project-version` flag.
* The flag is optional, we will try to resolve project version later in case it is not provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

There will always be a default project version. If no default plugins support the default project version, then an error should be returned.

Copy link
Contributor Author

@Adirio Adirio Mar 11, 2021

Choose a reason for hiding this comment

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

The idea here is, in case they provide a project version, make decissions knowing it, but in case it wasn't provide, just make decissions as if it was unknown and try to resolve it later.

Imagine I have the following plugins resolved:

Plugin Supported project version
go/v2 2, 3
go/v3 3
declarative/v1 2, 3

If the user provides a --plugins=go/v3,declarative flag, we would resolve to the last two plugins, and the only supported project version by both of them is 3 so we can implicitly resolve the project version.
If the user provides a --plugins=go/v2,declarative flag, we would resolve to the first and last plugin, and two different project versions are supported. If any of those is the default project version configured for the CLI, that will be used.

So we will have 3 ways of resolving the project version:

  • Explicitly by providing --project-version flag or having an already present config file (version field).
  • Implicitly by providing a set of plugins that only support a single common project version.
  • Default in case all resolved plugins support the default project version defined in the CLI.

In case none of the three above cases applies, an error ambiguous-project-version will be returned.

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants