From c0c511c73d25109d8adb183d9a6eb816f658f1da Mon Sep 17 00:00:00 2001 From: David Simansky Date: Sun, 24 Sep 2023 21:08:51 +0200 Subject: [PATCH 1/3] WIP: Context Sharing POC --- cmd/kn/main.go | 29 +++- cmd/kn/main_test.go | 8 +- pkg/kn/commands/service/describe.go | 16 +- pkg/kn/config/config.go | 5 + pkg/kn/config/config_test.go | 2 +- pkg/kn/config/testing.go | 2 + pkg/kn/config/testing_test.go | 2 + pkg/kn/config/types.go | 10 +- pkg/kn/plugin/context_sharing.go | 225 ++++++++++++++++++++++++++ pkg/kn/plugin/context_sharing_test.go | 185 +++++++++++++++++++++ pkg/kn/plugin/context_types.go | 15 ++ pkg/kn/plugin/manager.go | 24 +-- 12 files changed, 488 insertions(+), 35 deletions(-) create mode 100644 pkg/kn/plugin/context_sharing.go create mode 100644 pkg/kn/plugin/context_sharing_test.go create mode 100644 pkg/kn/plugin/context_types.go diff --git a/cmd/kn/main.go b/cmd/kn/main.go index b5e2bfb44..2b110a18b 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -23,7 +23,7 @@ import ( "github.com/spf13/cobra" "knative.dev/client/pkg/kn/config" - "knative.dev/client/pkg/kn/plugin" + pluginpkg "knative.dev/client/pkg/kn/plugin" "knative.dev/client/pkg/kn/root" ) @@ -60,7 +60,7 @@ func run(args []string) error { return err } - pluginManager := plugin.NewManager(config.GlobalConfig.PluginsDir(), config.GlobalConfig.LookupPluginsInPath()) + pluginManager := pluginpkg.NewManager(config.GlobalConfig.PluginsDir(), config.GlobalConfig.LookupPluginsInPath()) // Create kn root command and all sub-commands rootCmd, err := root.NewRootCommand(pluginManager.HelpTemplateFuncs()) @@ -84,6 +84,29 @@ func run(args []string) error { return err } + // FT: Context Sharing + if config.GlobalConfig.ContextSharing() { + ctxManager, err := pluginpkg.NewContextManager() + if err != nil { + return err + } + + defer func(ctxManager *pluginpkg.ContextDataManager) { + if err := ctxManager.WriteCache(); err != nil { + println("error during write") + } + }(ctxManager) + + err = ctxManager.FetchManifests(pluginManager) + if err != nil { + return err + } + + // Inject shared context data as context.Context + contextData := ctxManager.FetchContextData(pluginManager) + rootCmd.SetContext(contextData) + } + if plugin != nil { // Validate & Execute plugin err = validatePlugin(rootCmd, plugin) @@ -140,7 +163,7 @@ func filterHelpOptions(args []string) []string { } // Check if the plugin collides with any command specified in the root command -func validatePlugin(root *cobra.Command, plugin plugin.Plugin) error { +func validatePlugin(root *cobra.Command, plugin pluginpkg.Plugin) error { // Check if a given plugin can be identified as a command cmd, args, err := root.Find(plugin.CommandParts()) diff --git a/cmd/kn/main_test.go b/cmd/kn/main_test.go index 3435196c0..3b016fc79 100644 --- a/cmd/kn/main_test.go +++ b/cmd/kn/main_test.go @@ -21,7 +21,7 @@ import ( "strings" "testing" - "knative.dev/client/pkg/kn/plugin" + pluginpkg "knative.dev/client/pkg/kn/plugin" "github.com/spf13/cobra" "gotest.tools/v3/assert" @@ -361,7 +361,7 @@ func TestRun(t *testing.T) { args []string expectedOut []string expectedErrOut []string - plugin plugin.Plugin + plugin pluginpkg.Plugin }{ { []string{"kn", "version"}, @@ -425,8 +425,8 @@ func TestRun(t *testing.T) { for _, tc := range testCases { os.Args = tc.args if tc.plugin != nil { - plugin.InternalPlugins = plugin.PluginList{} - plugin.InternalPlugins = append(plugin.InternalPlugins, tc.plugin) + pluginpkg.InternalPlugins = pluginpkg.PluginList{} + pluginpkg.InternalPlugins = append(pluginpkg.InternalPlugins, tc.plugin) } capture := test.CaptureOutput(t) err := run(tc.args[1:]) diff --git a/pkg/kn/commands/service/describe.go b/pkg/kn/commands/service/describe.go index 4b6cbe9f7..06b283c0b 100644 --- a/pkg/kn/commands/service/describe.go +++ b/pkg/kn/commands/service/describe.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "io" + "sort" "strconv" "strings" @@ -99,10 +100,17 @@ func NewServiceDescribeCommand(p *commands.KnParams) *cobra.Command { Example: describe_example, ValidArgsFunction: commands.ResourceNameCompletionFunc(p), RunE: func(cmd *cobra.Command, args []string) error { + var serviceName string if len(args) != 1 { - return errors.New("'service describe' requires the service name given as single argument") + if cmd.Context() != nil && cmd.Context().Value("service") != nil { + serviceName = cmd.Context().Value("service").(string) + } + if serviceName == "" { + return errors.New("'service describe' requires the service name given as single argument") + } + } else { + serviceName = args[0] } - serviceName := args[0] namespace, err := p.GetNamespace(cmd) if err != nil { @@ -182,7 +190,7 @@ func describe(w io.Writer, service *servingv1.Service, revisions []*revisionDesc return nil } -// Write out main service information. Use colors for major items. +// WriteCache out main service information. Use colors for major items. func writeService(dw printers.PrefixWriter, service *servingv1.Service) { commands.WriteMetadata(dw, &service.ObjectMeta, printDetails) dw.WriteAttribute("URL", extractURL(service)) @@ -200,7 +208,7 @@ func writeService(dw printers.PrefixWriter, service *servingv1.Service) { } } -// Write out revisions associated with this service. By default only active +// WriteCache out revisions associated with this service. By default only active // target revisions are printed, but with --verbose also inactive revisions // created by this services are shown func writeRevisions(dw printers.PrefixWriter, revisions []*revisionDesc, printDetails bool) { diff --git a/pkg/kn/config/config.go b/pkg/kn/config/config.go index e1b6340ce..27689a713 100644 --- a/pkg/kn/config/config.go +++ b/pkg/kn/config/config.go @@ -55,6 +55,7 @@ const configContentDefaults = `# Taken from https://github.com/knative/client/bl // config contains the variables for the Kn config type config struct { + // configFile is the config file location configFile string @@ -65,6 +66,10 @@ type config struct { channelTypeMappings []ChannelTypeMapping } +func (c *config) ContextSharing() bool { + return viper.GetBool(keyFeaturesContextSharing) +} + // ConfigFile returns the config file which is either the default XDG conform // config file location or the one set with --config func (c *config) ConfigFile() string { diff --git a/pkg/kn/config/config_test.go b/pkg/kn/config/config_test.go index a21a1dcd1..bc91795d6 100644 --- a/pkg/kn/config/config_test.go +++ b/pkg/kn/config/config_test.go @@ -117,7 +117,7 @@ func setupConfig(t *testing.T, configContent string) (string, func()) { // Save old args backupArgs := os.Args - // Write out a temporary configContent file + // WriteCache out a temporary configContent file var cfgFile string if configContent != "" { cfgFile = filepath.Join(tmpDir, "config.yaml") diff --git a/pkg/kn/config/testing.go b/pkg/kn/config/testing.go index f7b2a0582..7a935a33f 100644 --- a/pkg/kn/config/testing.go +++ b/pkg/kn/config/testing.go @@ -18,6 +18,7 @@ package config // Set an instance of this for config.GlobalConfig to mock // your own configuration setup type TestConfig struct { + TestContextSharing bool TestPluginsDir string TestConfigFile string TestLookupPluginsInPath bool @@ -28,6 +29,7 @@ type TestConfig struct { // Ensure that TestConfig implements the configuration interface var _ Config = &TestConfig{} +func (t TestConfig) ContextSharing() bool { return t.TestContextSharing } func (t TestConfig) PluginsDir() string { return t.TestPluginsDir } func (t TestConfig) ConfigFile() string { return t.TestConfigFile } func (t TestConfig) LookupPluginsInPath() bool { return t.TestLookupPluginsInPath } diff --git a/pkg/kn/config/testing_test.go b/pkg/kn/config/testing_test.go index 093ee7edb..2ec18ae57 100644 --- a/pkg/kn/config/testing_test.go +++ b/pkg/kn/config/testing_test.go @@ -23,6 +23,7 @@ import ( // Test to keep code coverage quality gate happy. func TestTestConfig(t *testing.T) { cfg := TestConfig{ + TestContextSharing: false, TestPluginsDir: "pluginsDir", TestConfigFile: "configFile", TestLookupPluginsInPath: true, @@ -30,6 +31,7 @@ func TestTestConfig(t *testing.T) { TestChannelTypeMappings: nil, } + assert.Equal(t, cfg.ContextSharing(), false) assert.Equal(t, cfg.PluginsDir(), "pluginsDir") assert.Equal(t, cfg.ConfigFile(), "configFile") assert.Assert(t, cfg.LookupPluginsInPath()) diff --git a/pkg/kn/config/types.go b/pkg/kn/config/types.go index 2ef7dc8de..9af5f2bed 100644 --- a/pkg/kn/config/types.go +++ b/pkg/kn/config/types.go @@ -19,6 +19,9 @@ package config type Config interface { + // ContextSharing represents feature flag enabling context sharing + ContextSharing() bool + // ConfigFile returns the location of the configuration file ConfigFile() string @@ -70,9 +73,10 @@ type ChannelTypeMapping struct { // config Keys for looking up in viper const ( - keyPluginsDirectory = "plugins.directory" - keySinkMappings = "eventing.sink-mappings" - keyChannelTypeMappings = "eventing.channel-type-mappings" + keyFeaturesContextSharing = "features.context-sharing" + keyPluginsDirectory = "plugins.directory" + keySinkMappings = "eventing.sink-mappings" + keyChannelTypeMappings = "eventing.channel-type-mappings" ) // legacy config keys, deprecated diff --git a/pkg/kn/plugin/context_sharing.go b/pkg/kn/plugin/context_sharing.go new file mode 100644 index 000000000..85463607f --- /dev/null +++ b/pkg/kn/plugin/context_sharing.go @@ -0,0 +1,225 @@ +// Copyright © 2023 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin + +import ( + "bytes" + "context" + "encoding/json" + "io/fs" + "os" + "os/exec" + "path/filepath" + "strings" + + "knative.dev/client/pkg/kn/config" +) + +//--TYPES-- +//TODO: move types into its own file + +//TODO: let's see if we need this map +//type ContextData map[string]string + +// Manifest represents plugin metadata +type Manifest struct { + // Path to external plugin binary. Always empty for inlined plugins. + Path string `json:"path,omitempty"` + + // Plugin declares its own manifest to be included in Context Sharing feature + HasManifest bool `json:"hasManifest"` + + // ProducesContextDataKeys is a list of keys for the ContextData that + // a plugin can produce. Nil or an empty list declares that this + // plugin is not ContextDataProducer + //TODO: well-known keys could be const, or this can be its own data structure + ProducesContextDataKeys []string `json:"producesKeys,omitempty"` + + // ConsumesContextDataKeys is a list of keys from a ContextData that a + // plugin is interested in to consume. Nil or an empty list declares + // that this plugin is not a ContextDataConsumer + ConsumesContextDataKeys []string `json:"consumesKeys,omitempty"` +} + +// PluginWithManifest represents extended plugin support for Manifest and Context Sharing feature +type PluginWithManifest interface { + // Plugin original interface wrapper + Plugin + + // GetManifest + GetManifest() *Manifest + + // GetContextData + GetContextData() map[string]string + + // ExecuteWithContext + ExecuteWithContext(ctx context.Context, args []string) error +} + +//--TYPES-- + +var CtxManager *ContextDataManager + +type ContextDataManager struct { + //ContextData map[string]ContextData `json:"-"` + Producers map[string][]string `json:"producers"` + Consumers map[string][]string `json:"consumers"` + Manifests map[string]Manifest `json:"manifests"` +} + +func NewContextManager() (*ContextDataManager, error) { + if CtxManager == nil { + //println("opening file...") + //file, err := os.Open(filepath.Join(filepath.Dir(config.GlobalConfig.ConfigFile()), "context.json")) + //if err != nil { + // return nil, err + //} + //decoder := json.NewDecoder(file) + //ctxManager = &ContextDataManager{} + //if err := decoder.Decode(ctxManager); err != nil { + // return nil, err + //} + //out := new(bytes.Buffer) + //enc := json.NewEncoder(out) + //enc.SetIndent("", " ") + //enc.Encode(ctxManager) + //println(out.String()) + CtxManager = &ContextDataManager{ + Producers: map[string][]string{}, + Consumers: map[string][]string{}, + Manifests: map[string]Manifest{}, + } + } + return CtxManager, nil +} + +// GetConsumesKeys returns array of keys consumed by plugin +func (c *ContextDataManager) GetConsumesKeys(pluginName string) []string { + return c.Manifests[pluginName].ConsumesContextDataKeys +} + +// GetProducesKeys returns array of keys produced by plugin +func (c *ContextDataManager) GetProducesKeys(pluginName string) []string { + return c.Manifests[pluginName].ProducesContextDataKeys +} + +func (c *ContextDataManager) FetchContextData(pluginManager *Manager) context.Context { + ctx := context.Background() + for _, p := range pluginManager.GetInternalPlugins() { + if p.Name() == "kn func" { + if pwm, ok := p.(PluginWithManifest); ok { + data := pwm.GetContextData() + + for k, v := range data { + ctx = context.WithValue(ctx, k, v) + } + } + } + } + //ctx = context.WithValue(ctx, "service", "hello") + return ctx + +} + +// FetchManifests it tries to retrieve manifest from both inlined and external plugins +func (c *ContextDataManager) FetchManifests(pluginManager *Manager) error { + for _, plugin := range pluginManager.GetInternalPlugins() { + manifest := &Manifest{} + // For the integrity build the same name format as external plugins + pluginName := "kn-" + strings.Join(plugin.CommandParts(), "-") + if pwm, ok := plugin.(PluginWithManifest); ok { + manifest = pwm.GetManifest() + } + + // Add manifest to map + c.Manifests[pluginName] = *manifest + + if manifest.HasManifest { + c.populateDataKeys(manifest, pluginName) + } + } + plugins, err := pluginManager.ListPlugins() + if err != nil { + return err + } + for _, plugin := range plugins { + // Add new plugins only + if _, exists := c.Manifests[plugin.Name()]; !exists { + + if plugin.Path() != "" { + manifest := &Manifest{ + Path: plugin.Path(), + } + // Fetch from external plugin + if m := fetchExternalManifest(plugin); m != nil { + manifest = m + } + + // Add manifest to map + c.Manifests[plugin.Name()] = *manifest + + if manifest.HasManifest { + c.populateDataKeys(manifest, plugin.Name()) + } + } + } + } + return nil +} + +func (c *ContextDataManager) populateDataKeys(manifest *Manifest, pluginName string) { + // Build producers mapping + for _, key := range manifest.ProducesContextDataKeys { + c.Producers[key] = append(c.Producers[key], pluginName) + } + // Build consumers mapping + for _, key := range manifest.ConsumesContextDataKeys { + c.Consumers[key] = append(c.Consumers[key], pluginName) + } +} + +// TODO: We should cautiously execute external binaries +// fetchExternalManifest returns Manifest from external plugin by exec `$plugin manifest get` +func fetchExternalManifest(p Plugin) *Manifest { + cmd := exec.Command(p.Path(), "manifest") //nolint:gosec + stdOut := new(bytes.Buffer) + cmd.Stdout = stdOut + manifest := &Manifest{ + Path: p.Path(), + } + if err := cmd.Run(); err != nil { + return nil + } + d := json.NewDecoder(stdOut) + if err := d.Decode(manifest); err != nil { + return nil + } + manifest.HasManifest = true + return manifest +} + +// TODO: store to file actually +// WriteCache store data back to cache file +func (c *ContextDataManager) WriteCache() error { + //println("\n====\nContext Data to be stored:") + out := new(bytes.Buffer) + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + if err := enc.Encode(c); err != nil { + return nil + } + //println(out.String()) + return os.WriteFile(filepath.Join(filepath.Dir(config.GlobalConfig.ConfigFile()), "context.json"), out.Bytes(), fs.FileMode(0664)) +} diff --git a/pkg/kn/plugin/context_sharing_test.go b/pkg/kn/plugin/context_sharing_test.go new file mode 100644 index 000000000..852feec8e --- /dev/null +++ b/pkg/kn/plugin/context_sharing_test.go @@ -0,0 +1,185 @@ +// Copyright © 2023 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build !windows + +package plugin + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "gotest.tools/v3/assert" +) + +type testPluginWithManifest struct { + testPlugin + manifest *Manifest + contextData map[string]string +} + +func (t testPluginWithManifest) GetManifest() *Manifest { return t.manifest } +func (t testPluginWithManifest) GetContextData() map[string]string { return t.contextData } +func (t testPluginWithManifest) ExecuteWithContext(ctx context.Context, args []string) error { + return nil +} + +// Verify both interfaces are implemented +var _ Plugin = testPluginWithManifest{} +var _ PluginWithManifest = testPluginWithManifest{} + +func TestFetchManifest(t *testing.T) { + testCases := []struct { + name string + cmdPart []string + hasManifest bool + expectedManifest *Manifest + }{ + { + name: "Inlined with manifest", + cmdPart: []string{"cmd"}, + hasManifest: true, + expectedManifest: &Manifest{ + Path: "", + HasManifest: true, + ProducesContextDataKeys: []string{"service"}, + ConsumesContextDataKeys: []string{"service"}, + }, + }, + { + name: "Inlined no manifest", + cmdPart: []string{"no", "manifest"}, + hasManifest: false, + expectedManifest: &Manifest{ + HasManifest: false, + }, + }, + { + name: "No plugins", + cmdPart: []string{}, + hasManifest: false, + expectedManifest: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + c := setup(t) + t.Cleanup(func() { + cleanup(t, c) + InternalPlugins = PluginList{} + CtxManager = nil + }) + + if len(tc.cmdPart) == 0 { + ctxManager, err := NewContextManager() + assert.NilError(t, err) + + err = ctxManager.FetchManifests(c.pluginManager) + assert.NilError(t, err) + assert.Assert(t, len(ctxManager.Manifests) == 0) + return + } + + if tc.hasManifest { + prepareInternalPlugins(testPluginWithManifest{ + testPlugin: testPlugin{parts: tc.cmdPart}, + manifest: tc.expectedManifest, + contextData: map[string]string{}, + }) + } else { + prepareInternalPlugins(testPlugin{parts: tc.cmdPart}) + } + + ctxManager, err := NewContextManager() + assert.NilError(t, err) + + err = ctxManager.FetchManifests(c.pluginManager) + assert.NilError(t, err) + assert.Assert(t, len(ctxManager.Manifests) == 1) + + expectedKey := "kn-" + strings.Join(tc.cmdPart, "-") + assert.DeepEqual(t, ctxManager.Manifests[expectedKey], *tc.expectedManifest) + }) + } + +} + +func TestContextFetchExternalManifests(t *testing.T) { + testCases := []struct { + name string + testScript string + expectedManifest *Manifest + }{ + { + name: "manifest", + testScript: `#!/bin/bash +echo '{"hasManifest":true,"consumesKeys":["service"]}'\n`, + expectedManifest: &Manifest{ + HasManifest: true, + ConsumesContextDataKeys: []string{"service"}, + }, + }, + { + name: "badjson", + testScript: `#!/bin/bash +echo '{hasManifest:true,"consumesKeys":["service"]}'\n`, + expectedManifest: nil, + }, + { + name: "badscript", + testScript: `#!/bin/bash +exit 1\n`, + expectedManifest: nil, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ct := setup(t) + defer cleanup(t, ct) + + fullPath := createTestPluginInDirectoryFromScript(t, "kn-"+tc.name, ct.pluginsDir, tc.testScript) + // Set expected path + if tc.expectedManifest != nil { + tc.expectedManifest.Path = fullPath + } + + testPlugin, err := ct.pluginManager.FindPlugin([]string{tc.name}) + assert.NilError(t, err) + assert.Assert(t, testPlugin != nil) + + actual := fetchExternalManifest(testPlugin) + assert.DeepEqual(t, actual, tc.expectedManifest) + }) + + } + +} + +// CreateTestPluginInPath with name, path, script, and fileMode and return the tmp random path +func createTestPluginInDirectoryFromScript(t *testing.T, name string, dir string, script string) string { + fullPath := filepath.Join(dir, name) + err := os.WriteFile(fullPath, []byte(script), 0777) + assert.NilError(t, err) + // Some extra files to feed the tests + err = os.WriteFile(filepath.Join(dir, "non-plugin-prefix-"+name), []byte{}, 0555) + assert.NilError(t, err) + _, err = os.CreateTemp(dir, "bogus-dir") + assert.NilError(t, err) + + return fullPath +} diff --git a/pkg/kn/plugin/context_types.go b/pkg/kn/plugin/context_types.go new file mode 100644 index 000000000..fb03b8dce --- /dev/null +++ b/pkg/kn/plugin/context_types.go @@ -0,0 +1,15 @@ +// Copyright © 2023 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin diff --git a/pkg/kn/plugin/manager.go b/pkg/kn/plugin/manager.go index df8d852af..d244ebcb9 100644 --- a/pkg/kn/plugin/manager.go +++ b/pkg/kn/plugin/manager.go @@ -55,26 +55,6 @@ type Plugin interface { Path() string } -type ContextData map[string]string - -type PluginManifest struct { - // ProducesContextDataKeys is a list of keys for the ContextData that - // a plugin can produce. Nil or an empty list declares that this - // plugin is not ContextDataProducer - ProducesContextDataKeys []string - - // ConsumesContextDataKeys is a list of keys from a ContextData that a - // plugin is interested in to consume. Nil or an empty list declares - // that this plugin is not a ContextDataConsumer - ConsumesContextDataKeys []string -} - -type ContextDataConsumer interface { - // ExecuteWithContextData executes the plugin with the given args much like - // Execute() but with an additional argument that holds the ContextData - ExecuteWithContextData(args []string, data ContextData) error -} - type Manager struct { // Dedicated plugin directory as configured pluginsDir string @@ -125,6 +105,10 @@ func (manager *Manager) AppendPlugin(plugin Plugin) { InternalPlugins = append(InternalPlugins, plugin) } +func (manager *Manager) GetInternalPlugins() PluginList { + return InternalPlugins +} + // FindPlugin checks if a plugin for the given parts exist and return it. // The args given must not contain any options and contain only // the commands (like in [ "source", "github" ] for a plugin called 'kn-source-github' From 7e5bef57a92da10076058cbaf180a5bd7158a49a Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 20 Oct 2023 12:22:26 +0200 Subject: [PATCH 2/3] More lazy init --- cmd/kn/main.go | 23 ++++--- pkg/kn/commands/service/describe.go | 10 ++- pkg/kn/plugin/context_sharing.go | 99 ++++++++++++++------------- pkg/kn/plugin/context_sharing_test.go | 11 ++- 4 files changed, 77 insertions(+), 66 deletions(-) diff --git a/cmd/kn/main.go b/cmd/kn/main.go index 2b110a18b..c88980fba 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -85,8 +85,9 @@ func run(args []string) error { } // FT: Context Sharing + var ctxManager *pluginpkg.ContextDataManager if config.GlobalConfig.ContextSharing() { - ctxManager, err := pluginpkg.NewContextManager() + ctxManager, err = pluginpkg.NewContextManager(pluginManager) if err != nil { return err } @@ -96,15 +97,6 @@ func run(args []string) error { println("error during write") } }(ctxManager) - - err = ctxManager.FetchManifests(pluginManager) - if err != nil { - return err - } - - // Inject shared context data as context.Context - contextData := ctxManager.FetchContextData(pluginManager) - rootCmd.SetContext(contextData) } if plugin != nil { @@ -113,7 +105,16 @@ func run(args []string) error { if err != nil { return err } - + if config.GlobalConfig.ContextSharing() { + if pwm, ok := plugin.(pluginpkg.PluginWithManifest); ok { + data, _ := ctxManager.FetchContextData() + err := pwm.ExecuteWithContext(data, argsWithoutCommands(args, plugin.CommandParts())) + if err != nil { + return &runError{err: err} + } + } + return nil + } err := plugin.Execute(argsWithoutCommands(args, plugin.CommandParts())) if err != nil { return &runError{err: err} diff --git a/pkg/kn/commands/service/describe.go b/pkg/kn/commands/service/describe.go index 06b283c0b..8721dc870 100644 --- a/pkg/kn/commands/service/describe.go +++ b/pkg/kn/commands/service/describe.go @@ -19,7 +19,7 @@ import ( "errors" "fmt" "io" - + "knative.dev/client/pkg/kn/plugin" "sort" "strconv" "strings" @@ -102,8 +102,12 @@ func NewServiceDescribeCommand(p *commands.KnParams) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { var serviceName string if len(args) != 1 { - if cmd.Context() != nil && cmd.Context().Value("service") != nil { - serviceName = cmd.Context().Value("service").(string) + if plugin.CtxManager != nil { + data, err := plugin.CtxManager.FetchContextData() + if err != nil { + return err + } + serviceName = data["service"] } if serviceName == "" { return errors.New("'service describe' requires the service name given as single argument") diff --git a/pkg/kn/plugin/context_sharing.go b/pkg/kn/plugin/context_sharing.go index 85463607f..35ea0a166 100644 --- a/pkg/kn/plugin/context_sharing.go +++ b/pkg/kn/plugin/context_sharing.go @@ -16,7 +16,6 @@ package plugin import ( "bytes" - "context" "encoding/json" "io/fs" "os" @@ -28,10 +27,6 @@ import ( ) //--TYPES-- -//TODO: move types into its own file - -//TODO: let's see if we need this map -//type ContextData map[string]string // Manifest represents plugin metadata type Manifest struct { @@ -44,7 +39,6 @@ type Manifest struct { // ProducesContextDataKeys is a list of keys for the ContextData that // a plugin can produce. Nil or an empty list declares that this // plugin is not ContextDataProducer - //TODO: well-known keys could be const, or this can be its own data structure ProducesContextDataKeys []string `json:"producesKeys,omitempty"` // ConsumesContextDataKeys is a list of keys from a ContextData that a @@ -65,7 +59,7 @@ type PluginWithManifest interface { GetContextData() map[string]string // ExecuteWithContext - ExecuteWithContext(ctx context.Context, args []string) error + ExecuteWithContext(ctx map[string]string, args []string) error } //--TYPES-- @@ -74,32 +68,19 @@ var CtxManager *ContextDataManager type ContextDataManager struct { //ContextData map[string]ContextData `json:"-"` - Producers map[string][]string `json:"producers"` - Consumers map[string][]string `json:"consumers"` - Manifests map[string]Manifest `json:"manifests"` + PluginManager *Manager `json:"-"` + Producers map[string][]string `json:"producers"` + Consumers map[string][]string `json:"consumers"` + Manifests map[string]Manifest `json:"manifests"` } -func NewContextManager() (*ContextDataManager, error) { +func NewContextManager(pluginManager *Manager) (*ContextDataManager, error) { if CtxManager == nil { - //println("opening file...") - //file, err := os.Open(filepath.Join(filepath.Dir(config.GlobalConfig.ConfigFile()), "context.json")) - //if err != nil { - // return nil, err - //} - //decoder := json.NewDecoder(file) - //ctxManager = &ContextDataManager{} - //if err := decoder.Decode(ctxManager); err != nil { - // return nil, err - //} - //out := new(bytes.Buffer) - //enc := json.NewEncoder(out) - //enc.SetIndent("", " ") - //enc.Encode(ctxManager) - //println(out.String()) CtxManager = &ContextDataManager{ - Producers: map[string][]string{}, - Consumers: map[string][]string{}, - Manifests: map[string]Manifest{}, + PluginManager: pluginManager, + Producers: map[string][]string{}, + Consumers: map[string][]string{}, + Manifests: map[string]Manifest{}, } } return CtxManager, nil @@ -115,27 +96,31 @@ func (c *ContextDataManager) GetProducesKeys(pluginName string) []string { return c.Manifests[pluginName].ProducesContextDataKeys } -func (c *ContextDataManager) FetchContextData(pluginManager *Manager) context.Context { - ctx := context.Background() - for _, p := range pluginManager.GetInternalPlugins() { +func (c *ContextDataManager) FetchContextData() (map[string]string, error) { + // Load cached data first + if err := c.loadCache(); err != nil { + return nil, err + } + + // Fetch manifests + if err := c.FetchManifests(); err != nil { + return nil, err + } + + // Get context data, limited to func only + for _, p := range c.PluginManager.GetInternalPlugins() { if p.Name() == "kn func" { if pwm, ok := p.(PluginWithManifest); ok { - data := pwm.GetContextData() - - for k, v := range data { - ctx = context.WithValue(ctx, k, v) - } + return pwm.GetContextData(), nil } } } - //ctx = context.WithValue(ctx, "service", "hello") - return ctx - + return map[string]string{}, nil } // FetchManifests it tries to retrieve manifest from both inlined and external plugins -func (c *ContextDataManager) FetchManifests(pluginManager *Manager) error { - for _, plugin := range pluginManager.GetInternalPlugins() { +func (c *ContextDataManager) FetchManifests() error { + for _, plugin := range c.PluginManager.GetInternalPlugins() { manifest := &Manifest{} // For the integrity build the same name format as external plugins pluginName := "kn-" + strings.Join(plugin.CommandParts(), "-") @@ -150,7 +135,7 @@ func (c *ContextDataManager) FetchManifests(pluginManager *Manager) error { c.populateDataKeys(manifest, pluginName) } } - plugins, err := pluginManager.ListPlugins() + plugins, err := c.PluginManager.ListPlugins() if err != nil { return err } @@ -210,16 +195,38 @@ func fetchExternalManifest(p Plugin) *Manifest { return manifest } -// TODO: store to file actually +func (c *ContextDataManager) loadCache() error { + cacheFile := filepath.Join(filepath.Dir(config.GlobalConfig.ConfigFile()), "context.json") + if _, err := os.Stat(cacheFile); err != nil { + if os.IsNotExist(err) { + return nil // No cache file yet + } else { + return err + } + } + + file, err := os.Open(cacheFile) + if err != nil { + return err + } + decoder := json.NewDecoder(file) + ctxManager := &ContextDataManager{} + if err := decoder.Decode(ctxManager); err != nil { + return err + } + c.Manifests = ctxManager.Manifests + c.Producers = ctxManager.Producers + c.Consumers = ctxManager.Consumers + return nil +} + // WriteCache store data back to cache file func (c *ContextDataManager) WriteCache() error { - //println("\n====\nContext Data to be stored:") out := new(bytes.Buffer) enc := json.NewEncoder(out) enc.SetIndent("", " ") if err := enc.Encode(c); err != nil { return nil } - //println(out.String()) return os.WriteFile(filepath.Join(filepath.Dir(config.GlobalConfig.ConfigFile()), "context.json"), out.Bytes(), fs.FileMode(0664)) } diff --git a/pkg/kn/plugin/context_sharing_test.go b/pkg/kn/plugin/context_sharing_test.go index 852feec8e..17e97e9fd 100644 --- a/pkg/kn/plugin/context_sharing_test.go +++ b/pkg/kn/plugin/context_sharing_test.go @@ -17,7 +17,6 @@ package plugin import ( - "context" "os" "path/filepath" "strings" @@ -34,7 +33,7 @@ type testPluginWithManifest struct { func (t testPluginWithManifest) GetManifest() *Manifest { return t.manifest } func (t testPluginWithManifest) GetContextData() map[string]string { return t.contextData } -func (t testPluginWithManifest) ExecuteWithContext(ctx context.Context, args []string) error { +func (t testPluginWithManifest) ExecuteWithContext(ctx map[string]string, args []string) error { return nil } @@ -86,10 +85,10 @@ func TestFetchManifest(t *testing.T) { }) if len(tc.cmdPart) == 0 { - ctxManager, err := NewContextManager() + ctxManager, err := NewContextManager(c.pluginManager) assert.NilError(t, err) - err = ctxManager.FetchManifests(c.pluginManager) + err = ctxManager.FetchManifests() assert.NilError(t, err) assert.Assert(t, len(ctxManager.Manifests) == 0) return @@ -105,10 +104,10 @@ func TestFetchManifest(t *testing.T) { prepareInternalPlugins(testPlugin{parts: tc.cmdPart}) } - ctxManager, err := NewContextManager() + ctxManager, err := NewContextManager(c.pluginManager) assert.NilError(t, err) - err = ctxManager.FetchManifests(c.pluginManager) + err = ctxManager.FetchManifests() assert.NilError(t, err) assert.Assert(t, len(ctxManager.Manifests) == 1) From e2d3a49dbccce3b5915b34f8008fec8264c5f8bb Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 20 Oct 2023 12:32:14 +0200 Subject: [PATCH 3/3] Fix imports --- pkg/kn/commands/service/describe.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kn/commands/service/describe.go b/pkg/kn/commands/service/describe.go index 8721dc870..dc438ef41 100644 --- a/pkg/kn/commands/service/describe.go +++ b/pkg/kn/commands/service/describe.go @@ -19,7 +19,6 @@ import ( "errors" "fmt" "io" - "knative.dev/client/pkg/kn/plugin" "sort" "strconv" "strings" @@ -29,6 +28,7 @@ import ( "knative.dev/serving/pkg/apis/serving" "knative.dev/client/pkg/kn/commands/revision" + "knative.dev/client/pkg/kn/plugin" "knative.dev/client/pkg/printers" clientservingv1 "knative.dev/client/pkg/serving/v1"