feat: add GetFullSchema and FillDefaults methods to plugins #114

GGabriele · 2022-01-18T10:27:52Z

GetFullSchema: returns full schema for plugins (/schemas/plugins/<ID>)
FillDefaults: given a schema and a config object, fills all defaults

Based 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 and KIC to fill defaults values into plugins from their schemas.

GetFullSchema: returns full schema for plugins (/schemas/plugins/<ID>) FillDefaults: given a schema and a config object, fills all defaults

CLAassistant · 2022-01-18T10:27:59Z

All committers have signed the CLA.

kong/plugin_service.go

hbagdi · 2022-01-18T18:19:35Z

kong/plugin_service.go

 }

 // 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,


Suggested change

func (s *PluginService) GetFullSchema(ctx context.Context,

func (s *PluginService) GetPluginSchema(ctx context.Context,

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.

hbagdi · 2022-01-18T18:20:06Z

kong/plugin_service.go

+}
+
+// FillDefaults ingests plugin's defaults from its schema
+func (s *PluginService) FillDefaults(ctx context.Context,


This doesn't actually need to be on the PluginService since this function doesn't interact with the Admin API.

Moved this under kong/utils.go

rainest

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.

GGabriele · 2022-01-19T08:41:11Z

The main difference with the KIC implementation is that KIC was filling only Config defaults, while here we are filling both Config and Protocols, as well as setting Enabled to true as default. This is done by leveraging the "newer" /schemas/plugins/$ID endpoint, so most of the code differences in the fillConfigRecord function are due to handling a slightly different output from the one used in KIC.

codecov-commenter · 2022-01-19T08:45:10Z

Codecov Report

Merging #114 (36bf996) into main (8d08c5d) will increase coverage by 0.96%.
The diff coverage is 87.95%.

@@            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

Flag	Coverage Δ
2.0.5	`39.13% <87.95%> (+1.08%)`	⬆️
2.1.4	`38.94% <87.95%> (+1.09%)`	⬆️
2.2.2	`38.94% <87.95%> (+1.09%)`	⬆️
2.3.3	`38.94% <87.95%> (+1.09%)`	⬆️
2.4.0	`38.94% <87.95%> (+1.09%)`	⬆️
community	`39.13% <87.95%> (+1.08%)`	⬆️
enterprise	`44.15% <87.95%> (+0.97%)`	⬆️
enterprise-1.5.0.11	`44.15% <87.95%> (+0.97%)`	⬆️
enterprise-2.1.4.6	`43.02% <87.95%> (+1.00%)`	⬆️
enterprise-2.2.1.3	`43.02% <87.95%> (?)`
enterprise-2.3.3.2	`43.02% <87.95%> (?)`
integration	`44.46% <87.95%> (+0.96%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
kong/plugin_service.go	`56.79% <60.00%> (+0.32%)`	⬆️
kong/utils.go	`86.00% <94.11%> (+6.73%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d08c5d...36bf996. Read the comment docs.

hbagdi · 2022-01-20T18:06:18Z

kong/plugin_service.go

@@ -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


Suggested change

// GetSchema retrieves the config schema of a plugin

// GetSchema retrieves the config schema of a plugin.

hbagdi · 2022-01-20T18:06:26Z

kong/plugin_service.go

-	// GetSchema retrieves the schema of a plugin
+	// GetSchema retrieves the config schema of a plugin
+	//
+	// Deprecated: Use GetFullSchema instead


Suggested change

// Deprecated: Use GetFullSchema instead

// Deprecated: Use GetFullSchema instead.

hbagdi · 2022-01-20T18:07:23Z

kong/plugin_service.go

 	GetSchema(ctx context.Context, pluginName *string) (map[string]interface{}, error)
+	// GetFullSchema retrieves the full schema of a plugin


Suggested change

// 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.

hbagdi · 2022-01-20T18:07:41Z

kong/plugin_service.go

 }

 // PluginService handles Plugins in Kong.
 type PluginService service

-// GetSchema retrieves the schema of a plugin
+// GetFullSchema retrieves the full schema of a plugin


Suggested change

// GetFullSchema retrieves the full schema of a plugin

// GetFullSchema retrieves the full schema of a plugin.

hbagdi · 2022-01-20T18:09:19Z

kong/plugin_service_test.go

+		expected *Plugin
+	}{
+		{
+			name: "no_config_no_protocols",


Remove all _.

hbagdi · 2022-01-20T18:10:19Z

go.sum

@@ -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=


Run go mod tidy

hbagdi · 2022-01-20T18:14:43Z

kong/utils.go

+		for key, value := range field.Map() {
+			if key != "protocols" {
+				continue
+			}


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.

hbagdi

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.

GGabriele · 2022-01-20T18:43:31Z

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?

hbagdi · 2022-01-20T19:52:17Z

isn't it already tested on both as part of this here?

Oh boy.
That test is for another function. If that function is removed, we would be removing the call to the GetFullSchema function.

Please add another test that only tests the GetFullSchema method.
Assert the response code, and check for a few elements in the body to make sure the response is as expected.
Also, add a negative test for the "noexists" plugin.

GGabriele · 2022-01-21T11:56:27Z

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.

hbagdi · 2022-01-21T22:17:10Z

kong/utils.go

+}
+
+// FillPluginsDefaults ingests plugin's defaults from its schema
+func FillPluginsDefaults(plugin *Plugin, schema map[string]interface{}) (*Plugin, error) {


@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

Doh, my bad, sorry :(

#116

feat: add GetFullSchema and FillDefaults methods to plugins

91478a1

GetFullSchema: returns full schema for plugins (/schemas/plugins/<ID>) FillDefaults: given a schema and a config object, fills all defaults

GGabriele requested a review from a team as a code owner January 18, 2022 10:27

GGabriele mentioned this pull request Jan 18, 2022

feat: populate plugin defaults from their schemas Kong/deck#562

Merged

hbagdi reviewed Jan 18, 2022

View reviewed changes

rainest reviewed Jan 18, 2022

View reviewed changes

Mark method as deprecated

a9cf627

Move FillDefaults to utils

2caa2cf

rainest self-requested a review January 20, 2022 17:59

rainest approved these changes Jan 20, 2022

View reviewed changes

hbagdi reviewed Jan 20, 2022

View reviewed changes

hbagdi requested changes Jan 20, 2022

View reviewed changes

Addressing comments

1fe520f

GGabriele added 4 commits January 21, 2022 11:32

Add testcase for GetFullSchema

5742167

Use prom plugin for testing

7e14314

Use key-auth plugin for testing

029bce9

simplify tests

36bf996

hbagdi approved these changes Jan 21, 2022

View reviewed changes

hbagdi merged commit bfe90b4 into main Jan 21, 2022

hbagdi deleted the DECK-8 branch January 21, 2022 22:00

hbagdi reviewed Jan 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add GetFullSchema and FillDefaults methods to plugins #114

feat: add GetFullSchema and FillDefaults methods to plugins #114

GGabriele commented Jan 18, 2022

CLAassistant commented Jan 18, 2022 •

edited

Loading

hbagdi Jan 18, 2022

rainest Jan 18, 2022

hbagdi Jan 18, 2022

GGabriele Jan 19, 2022

rainest left a comment

GGabriele commented Jan 19, 2022

codecov-commenter commented Jan 19, 2022 •

edited

Loading

hbagdi Jan 20, 2022

hbagdi Jan 20, 2022

hbagdi Jan 20, 2022

hbagdi Jan 20, 2022

hbagdi Jan 20, 2022

hbagdi Jan 20, 2022

hbagdi Jan 20, 2022

hbagdi left a comment

GGabriele commented Jan 20, 2022

hbagdi commented Jan 20, 2022

GGabriele commented Jan 21, 2022

hbagdi Jan 21, 2022

GGabriele Jan 22, 2022

	func (s *PluginService) GetFullSchema(ctx context.Context,
	func (s *PluginService) GetPluginSchema(ctx context.Context,

	// GetSchema retrieves the config schema of a plugin
	// GetSchema retrieves the config schema of a plugin.

	// Deprecated: Use GetFullSchema instead
	// Deprecated: Use GetFullSchema instead.

		GetSchema(ctx context.Context, pluginName *string) (map[string]interface{}, error)
		// GetFullSchema retrieves the full schema of a plugin

	// 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.

feat: add GetFullSchema and FillDefaults methods to plugins #114

feat: add GetFullSchema and FillDefaults methods to plugins #114

Conversation

GGabriele commented Jan 18, 2022

CLAassistant commented Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainest left a comment

Choose a reason for hiding this comment

GGabriele commented Jan 19, 2022

codecov-commenter commented Jan 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

GGabriele commented Jan 20, 2022

hbagdi commented Jan 20, 2022

GGabriele commented Jan 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jan 18, 2022 •

edited

Loading

codecov-commenter commented Jan 19, 2022 •

edited

Loading