-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add plugin registry #5467
Add plugin registry #5467
Conversation
Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5467 +/- ##
==========================================
+ Coverage 26.15% 26.19% +0.04%
==========================================
Files 457 458 +1
Lines 49111 49169 +58
==========================================
+ Hits 12843 12881 +38
- Misses 35246 35266 +20
Partials 1022 1022 ☔ View full report in Codecov by Sentry. |
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
[IMO]
We have 2 kinds of packages under pkgs/plugin
, but it's confusing. So we may have to separate them.
- packages mainly used by the plugins
- packages mainly used by the piped, and it operates about the plugins
Let's discuss and do it on another PR.
plugin, ok := pr.stageBasedPlugins[stage.Name.String()] | ||
if ok { | ||
plugins = append(plugins, 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.
[ASK]
What if not ok
? I think it should return an error.
} | |
}else{ | |
return nil, fmt.Errorf("no plugin found for the stage %s", stage.Name.String()) | |
} |
instead of the following:
if len(plugins) == 0 {
return nil, fmt.Errorf("no plugin found for each stages")
}
The same in getPluginClientsByNames()
.
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 see, thanks.
I fixed to return when the plugin couldn't find by the given identity (such as stage name or plugin name)
58f6e2a
} | ||
|
||
func TestPluginRegistry_GetPluginClientsByAppConfig(t *testing.T) { | ||
tests := []struct { |
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.
[ASK] Is it possible to add t.Parallel()
?
Also in t.Run(tt.name, func(t *testing.T) {
.
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.
Thanks, added on 1e1bd50
Signed-off-by: Yoshiki Fujikane <[email protected]>
…y (stage name and plugin name) Signed-off-by: Yoshiki Fujikane <[email protected]>
@Warashi Sorry, plz re-approve 🙏 |
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.
Thank you!
What this PR does:
I added the package
pkg/plugin/registry
.This package is responsible for finding which plugin is used to deploy the app.
I implemented the two use-cases as methods for now.
GetPluginClientsByAppConfig
: Get the plugin client based on the app configGetPluginClientByStageName
: Get the plugin client based on the stage nameThese will be used in the controller, scheduler, planner like this↓ (draft implementation)
add-plugin-registry...use-plugin-registry
Why we need it:
We need to find the plugin for the app to deploy, detect the diff, and get the live state from the plugin.
At first, we added the plugin name to the
model.Application
on the PR #5457.However, users should update the app to change its available plugins on the Web UI when they want to use another plugin. It takes more time for users.
So, we decided to determine the plugin used to deploy an app from the app config.
Which issue(s) this PR fixes:
Part of #5259
Does this PR introduce a user-facing change?: