-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: add GetFullSchema and FillDefaults methods to plugins #114
Conversation
GetFullSchema: returns full schema for plugins (/schemas/plugins/<ID>) FillDefaults: given a schema and a config object, fills all defaults
} | ||
|
||
// PluginService handles Plugins in Kong. | ||
type PluginService service | ||
|
||
// GetSchema retrieves the schema of a plugin | ||
// GetFullSchema retrieves the full schema of a plugin | ||
func (s *PluginService) GetFullSchema(ctx context.Context, |
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.
func (s *PluginService) GetFullSchema(ctx context.Context, | |
func (s *PluginService) GetPluginSchema(ctx context.Context, |
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 should remain as-is: it's more descriptive of the difference between this and GetSchema, and we already know this is plugin stuff since it's part of the plugin service.
kong/plugin_service.go
Outdated
} | ||
|
||
// FillDefaults ingests plugin's defaults from its schema | ||
func (s *PluginService) FillDefaults(ctx context.Context, |
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 doesn't actually need to be on the PluginService since this function doesn't interact with the Admin API.
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.
Moved this under kong/utils.go
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.
What were the differences between this and the original KIC implementation? It looks like the main thing is that you get back a new plugin instance with the default config (which is fine), rather than getting a config and shoving that into your existing plugin, but I may be missing something since a lot of the code is similar.
The main difference with the KIC implementation is that KIC was filling only |
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 43.49% 44.46% +0.96%
==========================================
Files 42 42
Lines 3722 3805 +83
==========================================
+ Hits 1619 1692 +73
- Misses 1786 1793 +7
- Partials 317 320 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
kong/plugin_service.go
Outdated
@@ -29,14 +29,39 @@ type AbstractPluginService interface { | |||
ListAllForRoute(ctx context.Context, routeID *string) ([]*Plugin, error) | |||
// Validate validates a Plugin against its schema | |||
Validate(ctx context.Context, plugin *Plugin) (bool, string, error) | |||
// GetSchema retrieves the schema of a plugin | |||
// GetSchema retrieves the config schema of a 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.
// GetSchema retrieves the config schema of a plugin | |
// GetSchema retrieves the config schema of a plugin. |
kong/plugin_service.go
Outdated
// GetSchema retrieves the schema of a plugin | ||
// GetSchema retrieves the config schema of a plugin | ||
// | ||
// Deprecated: Use GetFullSchema instead |
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.
// Deprecated: Use GetFullSchema instead | |
// Deprecated: Use GetFullSchema instead. |
kong/plugin_service.go
Outdated
GetSchema(ctx context.Context, pluginName *string) (map[string]interface{}, error) | ||
// GetFullSchema retrieves the full schema of a 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.
// GetFullSchema retrieves the full schema of a plugin | |
// GetFullSchema retrieves the full schema of a plugin. | |
// This makes the use of `/schemas` endpoint in Kong. |
kong/plugin_service.go
Outdated
} | ||
|
||
// PluginService handles Plugins in Kong. | ||
type PluginService service | ||
|
||
// GetSchema retrieves the schema of a plugin | ||
// GetFullSchema retrieves the full schema of a 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.
// GetFullSchema retrieves the full schema of a plugin | |
// GetFullSchema retrieves the full schema of a plugin. |
kong/plugin_service_test.go
Outdated
expected *Plugin | ||
}{ | ||
{ | ||
name: "no_config_no_protocols", |
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.
Remove all _
.
go.sum
Outdated
@@ -56,6 +56,8 @@ github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ | |||
github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= | |||
github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU= | |||
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= | |||
github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ= | |||
github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= |
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.
Run go mod tidy
kong/utils.go
Outdated
for key, value := range field.Map() { | ||
if key != "protocols" { | ||
continue | ||
} |
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.
Map() returns a map[string]Result:
https://pkg.go.dev/github.com/tidwall/gjson?utm_source=godoc#Result.Map
Please remove this loop and instead use field.Map()["protocols"]
to do the same thing.
Linear searches are best avoided, especially when an index is already available.
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.
Add an integration test for the GetFulLSchema function that has been added. The test must ensure that this function works against OSS as well as Enterprise.
isn't it already tested on both as part of this here? |
Oh boy. Please add another test that only tests the GetFullSchema method. |
Wanted to add a testcase to check the whole schema response, but couldn't find a plugin whose schema remained fully consistent across the various EE-OSS versions, so I reverted to just test errors and basic response structure. Let me know if that's fine. |
} | ||
|
||
// FillPluginsDefaults ingests plugin's defaults from its schema | ||
func FillPluginsDefaults(plugin *Plugin, schema map[string]interface{}) (*Plugin, error) { |
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.
@GGabriele I made a mistake in the review.
This method interface is misleading and wrong.
The code takes in a plugin struct and then mutates it.
But the method signature also returns a plugin resource.
Either of the following are fine (but together not):
- Return only an error, and document that the plugin is filled and mutated
- Inside the function, make a copy of the input plugin, mutate the copy and return 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.
Doh, my bad, sorry :(
GetFullSchema
: returns full schema for plugins (/schemas/plugins/<ID>
)FillDefaults
: given a schema and a config object, fills all defaultsBased on the discussion happening in here, this PR is introducing 2 new methods for
Plugin
(along few other helper functions, mostly taken from KIC plus few changes).These can be leveraged by
deck
andKIC
to fill defaults values into plugins from their schemas.