-
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
pkg/cli: resolve --plugins
/layout
value by semantic version
#1536
pkg/cli: resolve --plugins
/layout
value by semantic version
#1536
Conversation
7b2a309
to
eb9b0ee
Compare
d008f68
to
74c039d
Compare
9eada89
to
a94b488
Compare
669b702
to
899d142
Compare
--plugins
/layout
value by semantic version
899d142
to
7188ed1
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.
You are doing many changes. Its ONLY possible check and ensure that the plugin is working well with 2..N plugin versions implemented in the project when we have more than one version here which is ONLY done in the commit e38b06b from the PR #1498. Otherwise, we might be introducing new issues scenarios in this design/solution which is happening in this case. See:
Starting to generate projects with version 3-alpha
Generating project-v3
go: creating new go.mod: module sigs.k8s.io/kubebuilder/testdata/project-v3
initializing project-v3 ...
Error: unknown flag: --domain
2020/06/01 13:00:28 unknown flag: --domain
make[1]: *** [Makefile:63: generate-testdata] Error 1
make[1]: Leaving directory '/Users/camilamacedo/go/src/sigs.k8s.io/kubebuilder'
make: *** [Makefile:58: generate] Error 2
IMO we should move forward with #1498 and #1538 then, you would be able to work in improvements and cleanups as you wish and then, this scenario will be verified tested by the CI to avoid new issues be introduced as well. (kb working with more than one plugin version)
7188ed1
to
4a5d821
Compare
4a5d821
to
1afe32c
Compare
containing multiple plugins of the same name (different versions) or same short name (different fully-qualified names). For example, when a CLI containing plugins "go.kubebuilder.io/v2.0" and "go.kubebuilder.io/v2.1" with a default of the latter is passed a either config with "layout: go/v2.0", a config with "layout: go/v2.1", or "--plugins go", it should resolve each successfully. pkg/cli: resolve default plugins first so that, if a key matches a default plugin, the correct plugin is always returned. If a user wants non-default behavior they must specify the exact plugin they want.
1afe32c
to
cb81777
Compare
@@ -218,22 +216,32 @@ func (c *cli) initialize() error { | |||
// layout and --plugins values can be short (ex. "go/v2.0.0") or unversioned |
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.
// layout and --plugins values can be short (ex. "go/v2.0.0") or unversioned | |
// layout and --plugins values can be short (ex. "go/v2.0") or unversioned |
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.
We haven't yet removed patch versions yet, PR to come.
|
||
"sigs.k8s.io/kubebuilder/pkg/plugin" | ||
) | ||
|
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.
Coud we add here :
func TestPlugin(t *testing.T) { | |
RegisterFailHandler(Fail) | |
RunSpecs(t, "Plugin test") | |
} |
The reason is that it makes easier we work with since we can run just this file.
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 case is part of the greater CLI suite. I don't see a good reason to separate them right now.
By("resolving foo/v2.0") | ||
resolvedPlugins, err = resolvePluginsByKey(plugins, "foo/v2.0") | ||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(makePluginKeySlice(resolvedPlugins...)).To(Equal([]string{"foo.kubebuilder.io/v2.0"})) |
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 following scenario is not working:
It("should return the correct version when just the name of the plugin is informed", func() {
var (
plugins = makePluginsForKeys(
"foo.example.com/v1.0",
"foo.example.com/v2.0",
)
resolvedPlugins []plugin.Base
err error
)
By("resolving foo.example.com")
resolvedPlugins, err = resolvePluginsByKey(plugins, "foo.example.com")
Expect(err).NotTo(HaveOccurred())
Expect(makePluginKeySlice(resolvedPlugins...)).To(Equal([]string{"foo.example.com/v2.0"}))
})
should we not return the upper version in this case since we are allowing pass just the name?
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 key would be ambiguous because there are multiple major versions for key foo.example.com
. See this case.
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.
Agreed that it would be a RFE
err error | ||
) | ||
|
||
It("should resolve keys correctly", func() { |
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 it would be easier to understand if we have 1 IT for scenario such as:
It("should work when the plugin informed exist", func() {
By("resolving foo.example.com/v1.0")
resolvedPlugins, err = resolvePluginsByKey(plugins, "foo.example.com/v1.0")
Expect(err).NotTo(HaveOccurred())
Expect(makePluginKeySlice(resolvedPlugins...)).To(Equal([]string{"foo.example.com/v1.0"}))
})
- should work when just the name is informed
- should work when short name is informed
etc ...
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.
Using multiple By()
's within an It()
is idiomatic, check out the docs.
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 am not sure if here, it is so clear as the examples in the doc but ok.
_, err = resolvePluginsByKey(plugins, "foo") | ||
Expect(err).To(MatchError(errAmbiguousPlugin{ | ||
key: "foo", | ||
msg: `possible keys: ["foo.example.com/v1.0" "foo.kubebuilder.io/v1.0" "foo.kubebuilder.io/v2.0"]`, |
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.
IMO this scenario make is better covered with the currently duplicated issue for plugins.
I mean the plugin/v1.0.0 is duplicated in my POV is a more intuitive error for 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.
The plugins known to the CLI
are not duplicated, i.e. there is no duplicate key in plugins
here. The user-defined key foo
is ambiguous because it can match multiple known plugins.
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, 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 |
Multiple plugins with the same name but different versions are now resolved correctly by
resolvePluginsByKey()
. For example, a config withlayout: go/v2.0
and another withlayout: go/v2.1
should both be resolvable by a particular CLI.