Skip to content
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

Context Sharing POC #1855

Merged
merged 3 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions cmd/kn/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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())
Expand All @@ -84,13 +84,37 @@ func run(args []string) error {
return err
}

// FT: Context Sharing
var ctxManager *pluginpkg.ContextDataManager
if config.GlobalConfig.ContextSharing() {
ctxManager, err = pluginpkg.NewContextManager(pluginManager)
if err != nil {
return err
}

defer func(ctxManager *pluginpkg.ContextDataManager) {
if err := ctxManager.WriteCache(); err != nil {
println("error during write")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message should contain more context (pun intended :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, you propbably want to write this to standard error.

}
}(ctxManager)
}

if plugin != nil {
// Validate & Execute plugin
err = validatePlugin(rootCmd, plugin)
if err != nil {
return err
}

if config.GlobalConfig.ContextSharing() {
if pwm, ok := plugin.(pluginpkg.PluginWithManifest); ok {
data, _ := ctxManager.FetchContextData()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably should not ignore the error here. Either remove the error part of the FectContextData() signature or return the error (I hope, the second arg is an error ;-), nevertheless ignore this here has some smell)

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}
Expand Down Expand Up @@ -140,7 +164,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())

Expand Down
8 changes: 4 additions & 4 deletions cmd/kn/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -361,7 +361,7 @@ func TestRun(t *testing.T) {
args []string
expectedOut []string
expectedErrOut []string
plugin plugin.Plugin
plugin pluginpkg.Plugin
}{
{
[]string{"kn", "version"},
Expand Down Expand Up @@ -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:])
Expand Down
20 changes: 16 additions & 4 deletions pkg/kn/commands/service/describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,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"

Expand Down Expand Up @@ -99,10 +100,21 @@ 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 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")
}
} else {
serviceName = args[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I have the feeling that his access pattern will be repeated in several commands (all that require a service name), so maybe we could extract this handling in a utility method. Usually, I wouldn't opt for this, but this looks complex enough to avoid a C&P approach.

}
serviceName := args[0]

namespace, err := p.GetNamespace(cmd)
if err != nil {
Expand Down Expand Up @@ -182,7 +194,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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be a wrong search/replace thingy. Same as in the comment below.

func writeService(dw printers.PrefixWriter, service *servingv1.Service) {
commands.WriteMetadata(dw, &service.ObjectMeta, printDetails)
dw.WriteAttribute("URL", extractURL(service))
Expand All @@ -200,7 +212,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) {
Expand Down
5 changes: 5 additions & 0 deletions pkg/kn/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/kn/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong comment

var cfgFile string
if configContent != "" {
cfgFile = filepath.Join(tmpDir, "config.yaml")
Expand Down
2 changes: 2 additions & 0 deletions pkg/kn/config/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 }
Expand Down
2 changes: 2 additions & 0 deletions pkg/kn/config/testing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ import (
// Test to keep code coverage quality gate happy.
func TestTestConfig(t *testing.T) {
cfg := TestConfig{
TestContextSharing: false,
TestPluginsDir: "pluginsDir",
TestConfigFile: "configFile",
TestLookupPluginsInPath: true,
TestSinkMappings: nil,
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())
Expand Down
10 changes: 7 additions & 3 deletions pkg/kn/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
Loading
Loading