enable saving config between steps in a single feature #103

n3wscott · 2021-02-24T22:35:55Z

Changes

Looking for feedback on a method to pass data between steps in a feature.

Note that the interface introduced here is aligned with the implementation in pkg: https://github.com/knative/pkg/tree/master/kvstore

/kind enhancement

Relates to #66

vaikas · 2021-02-24T23:13:43Z

pkg/feature/feature.go

@@ -52,6 +55,22 @@ func (s *Step) TestName() string {
 	}
 }

+// Save a value for use in a later feature step, or error on issue.
+func (f *Feature) Save(ctx context.Context, t *testing.T, key string, value interface{}) {


Just curious what the reasoning is for Save/Load vs. Set/Get? I found it a bit odd that we translate Save/Load into Get/Set underneath.

I guess my thinking is that to me Save/Load semantically means save everything or load everything.

I was trying to make a helper method for getting and setting the keys tied up with failing the test, so maybe you have a better name for the method? Maybe I also call it Get and Set, note it is on the top level object, not the Store object.

vaikas · 2021-02-24T23:16:24Z

pkg/state/interfaces.go

+import "context"
+
+// This is a subset of knative.dev/pkg/kvstore.Interface
+type Store interface {


Instead of defining a separate interface, would it be bad if we did not define this new interface, but instead in the InMemory implementation, we return NonImplemented error instead?

I was attempting to allow to use one of those impls, but not vendor it in directly because the Load and Init and Save did not make sense for Features

slinkydeveloper

I like the approach overrall, but i don't like that i need to use feature to Load and Store. This approach with feature is not really modular, because you can't provide a stepfn that wants to push things inside the store without without knowing in advance the feature, breaking the encapsulation between feature and stepfn (for example, think about one of the recorder features pushing useful data inside the kv store when installing for later usage)

What I had in mind was something using the context, where the kv store lives in the context and this context is passed through the various phases of the execution:

	f.Setup("install sender", func(ctx context.Context, t *testing.T) {
		t.Helper()
		var to string
		var event cloudevents.Event
		storepkg.Load(ctx, t, "to", &to)
		storepkg.Load(ctx, t, "event", &event)
		from := feature.MakeRandomK8sName("sender")
		eventshub.Install(from, eventshub.StartSender(to), eventshub.InputEvent(event))
	})

(storepkg is the module where kvstore impl lives)

Also you should provide a Get method that returns interface{} too, so user can choose between the two "styles"

n3wscott · 2021-03-01T18:24:59Z

having the store be context based will not work because of how passing and threading work, it would have to be injected into the context higher up and then used as a pointer to another kvstore, which we can do as a followup that is feature based. So I say we start with this when I fix it up with the comments and we can look at fancy hooks to access the kvstore later? or maybe ... meh YOLO, brb.

n3wscott · 2021-03-01T19:19:54Z

ok please take another look, I took the request from @vaikas and @slinkydeveloper and made the feature integration look like this:

setup,

		state.SetOrFail(ctx, t, "event", FullEvent())
...
		state.SetOrFail(ctx, t, "to", to)

using,

		to := state.GetStringOrFail(ctx, t, "to")
		var event cloudevents.Event
		state.GetOrFail(ctx, t, "event", &event)

but nothing is stopping you from loading the KVStore directly:

s := state.FromContext(ctx)
s.Get/Set ...

…ccess.

vaikas · 2021-03-01T19:48:10Z

/hold
Just holding in case @slinkydeveloper has more thoughts.
But otherwise, LGTM. Thanks!

pkg/state/context.go

slinkydeveloper · 2021-03-02T08:02:55Z

pkg/state/kvstore.go

+	if !ok {
+		return fmt.Errorf("key %s does not exist", key)
+	}
+	err := json.Unmarshal([]byte(v), value)


Ah wait i didn't saw it, why storing json? Can't we store just void pointers to things and then cast them when retrieving them? The state to store doesn't necessarily define conversion back and forth to json, and for some reason the user might even not be able to define such conversion (e.g. when you're using a struct from an external library as state)

I avoided this way to prevent mutation of the thing that is stored in the kvstore. Set, Get, mutate get'ed object, Get updated object?? is unexpected to me. cheap and cheerful way is json.

I see your point, but maybe what I want to store inside the kvstore is exactly a mutable object, like a list.

In any case, my problem with json is that you might end up storing objects which are not very json compatible but where marshalling doesn't fail, and you will lose some information, and together with it debugging time.

the impl can evolve over time, this is more about the interface and the current problemset is really "how do I pass that url to the next phase"

if you start working out ways of passing callbacks I will burn the building down.

net.Url is a too simplistic example (and even with it, sometimes encoding and decoding it back and forth to json has weird behaviours). Any more complex state where json encoding/decoding is not defined won't work with this implementation.

if you start working out ways of passing callbacks I will burn the building down.

That's state of the test too 😄

it is a kv store, if you can't save it to a config map, it will not work. That is the bar.

But why are we using the kv store model for something which is always in memory by design? It's an in memory map, it's the state of the test, it's not something that has to be read/write to a config map.

it might let us do some really interesting things later, like retries or test startup in the middle, or capture test state.

n3wscott · 2021-03-02T16:36:26Z

Ready.

/unhold

slinkydeveloper · 2021-03-02T16:46:50Z

pkg/state/context.go

 func ContextWith(ctx context.Context, s Store) context.Context {
 	return context.WithValue(ctx, envKey{}, s)
 }

+// FromContext returns the Store from Context, if not found FromContext will
+// panic.
+// TODO: revisit if we really want to panic here... likely not.


IMO panic is absolutely fine here, the Store must always exists in the context passed to StepFn. If it's not there, it's a reconciler-test bug

slinkydeveloper · 2021-03-02T16:54:42Z

pkg/state/kvstore.go

+	if !ok {
+		return fmt.Errorf("key %s does not exist", key)
+	}
+	err := json.Unmarshal([]byte(v), value)


I see your point, but maybe what I want to store inside the kvstore is exactly a mutable object, like a list.

In any case, my problem with json is that you might end up storing objects which are not very json compatible but where marshalling doesn't fail, and you will lose some information, and together with it debugging time.

slinkydeveloper · 2021-03-02T16:55:18Z

pkg/state/kvstore.go

+}
+
+// Set marshals and sets the value given under specified key.
+func (cs *KVStore) Set(_ context.Context, key string, value interface{}) error {


Do we expect users to check the error everytime and fail the StepFn if the Set/Get returns an error? Isn't that too much and makes the usage unnecessary complicated? Can't we just panic in case of an error? These Get/Set errors are not recoverable anyway.

I am matching a subset of this interface: https://github.com/knative/pkg/blob/master/kvstore/kvstore.go#L23

Since it's an implementation, i guess you can always have the GetOrFail and SetOrFail

We can't do that because KVStore is not used, it is Store, and Store is the pkg kvstore interface.

I have added methods like this to the context.

n3wscott · 2021-03-02T17:11:06Z

pkg/state/interfaces.go

+
+import "context"
+
+// This is a subset of knative.dev/pkg/kvstore.Interface


note: @slinkydeveloper ^^

Any particular reason why we're trying to match that particular interface about config maps?

So that we can swap out this impl for the same that is used in pkg.

We are trying to make synergy here, not reboil the world. I made the interface a subset because some of those things did not make sense in the context of features, but I want to use that impl in pkg at some point later.

knative-prow-robot · 2021-03-02T22:34:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enable saving config between steps in a single feature

e632ecd

knative-prow-robot added the kind/enhancement label Feb 24, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 24, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 24, 2021

lint

90b8bb3

vaikas reviewed Feb 24, 2021

View reviewed changes

slinkydeveloper reviewed Mar 1, 2021

View reviewed changes

n3wscott force-pushed the kvstore branch 2 times, most recently from 126e09c to 2a10dfe Compare March 1, 2021 19:12

based on feedback, make methods consistant and enable context based a…

488e224

…ccess.

n3wscott force-pushed the kvstore branch from 2a10dfe to 488e224 Compare March 1, 2021 19:21

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2021

vaikas previously approved these changes Mar 1, 2021

View reviewed changes

knative-prow-robot assigned vaikas Mar 1, 2021

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 1, 2021

slinkydeveloper reviewed Mar 2, 2021

View reviewed changes

Scott Nichols added 2 commits March 2, 2021 08:13

Merge branch 'master' into kvstore

34b6c82

adding more comments around assumptions

55d330f

n3wscott dismissed vaikas’s stale review via 55d330f March 2, 2021 16:33

knative-prow-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 2, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2021

slinkydeveloper reviewed Mar 2, 2021

View reviewed changes

n3wscott commented Mar 2, 2021

View reviewed changes

Merge branch 'master' into kvstore

432aab4

update to use feature.T

469acac

vaikas approved these changes Mar 2, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2021

knative-prow-robot merged commit c687b5f into knative-extensions:master Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable saving config between steps in a single feature #103

enable saving config between steps in a single feature #103

n3wscott commented Feb 24, 2021 •

edited

Loading

vaikas Feb 24, 2021

vaikas Feb 24, 2021

n3wscott Feb 25, 2021

vaikas Feb 24, 2021

n3wscott Feb 25, 2021

slinkydeveloper left a comment

n3wscott commented Mar 1, 2021

n3wscott commented Mar 1, 2021

vaikas commented Mar 1, 2021

slinkydeveloper Mar 2, 2021

n3wscott Mar 2, 2021

slinkydeveloper Mar 2, 2021

n3wscott Mar 2, 2021

slinkydeveloper Mar 2, 2021

n3wscott Mar 2, 2021

slinkydeveloper Mar 2, 2021 •

edited

Loading

n3wscott Mar 2, 2021

n3wscott commented Mar 2, 2021

slinkydeveloper Mar 2, 2021

slinkydeveloper Mar 2, 2021

slinkydeveloper Mar 2, 2021

n3wscott Mar 2, 2021

slinkydeveloper Mar 2, 2021 •

edited

Loading

n3wscott Mar 2, 2021

n3wscott Mar 2, 2021

slinkydeveloper Mar 2, 2021

n3wscott Mar 2, 2021

n3wscott Mar 2, 2021

knative-prow-robot commented Mar 2, 2021


		import "context"

		// This is a subset of knative.dev/pkg/kvstore.Interface

enable saving config between steps in a single feature #103

enable saving config between steps in a single feature #103

Conversation

n3wscott commented Feb 24, 2021 • edited Loading

Changes

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

slinkydeveloper left a comment

Choose a reason for hiding this comment

n3wscott commented Mar 1, 2021

n3wscott commented Mar 1, 2021

vaikas commented Mar 1, 2021

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

slinkydeveloper Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3wscott commented Mar 2, 2021

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

slinkydeveloper Mar 2, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Mar 2, 2021

n3wscott commented Feb 24, 2021 •

edited

Loading

slinkydeveloper Mar 2, 2021 •

edited

Loading

slinkydeveloper Mar 2, 2021 •

edited

Loading