Reimplement script commands against hive script #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
|
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
from
0d30ab4
to
1724458
Compare
joamaki
commented
Oct 2, 2024
joamaki
commented
Oct 2, 2024
joamaki
commented
Oct 2, 2024
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
11 times, most recently
from
dea1492
to
d1d27a4
Compare
Bring in the new hive version that includes the script support. Fix the API usage. Signed-off-by: Jussi Maki <[email protected]>
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
2 times, most recently
from
35f2d78
to
27423b5
Compare
joamaki
changed the title
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
from
27423b5
to
f01b461
Compare
joamaki
commented
Oct 9, 2024
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
3 times, most recently
from
d73ca35
to
950e499
Compare
joamaki
commented
Oct 9, 2024
| @@ -12,6 +12,10 @@ func String(s string) Key { | |||
| return []byte(s) | |||
| } | |||
|
|
|||
| func FromString(s string) (Key, error) { | |||
There was a problem hiding this comment.
I think it makes sense to have also these utility functions here for the FromString index field. Not sure what the naming convention should be. Maybe index.Int32String...
This also makes me wonder how should we think about e.g. prefix searching index that has index.Int32 as the key (e.g. big-endian encoded). If one calls index.Int32String with 65536 (0x0001_0000), should it return []byte{0x00, 0x01} and you'd get all objects with key 0x0001_0000 to 0x0001_FFFF...
This sort of thing isn't currently supported with the "fixed-sized" keys created with index.Int* etc.
If we would like to support it we'd have to have a different method for constructing prefix search keys.
I guess this is something we can visit in the future when a use-case arises for this.
There was a problem hiding this comment.
Added the *String variants in the last commit, e.g. index.Int32String and so on. Kept index.FromString as index.StringString would've been weird. Or should I add it for consistency?
There was a problem hiding this comment.
Or should I add it for consistency?
As a personal preference I'd go for index.StringString to be consistent, but agree that it is not a nice name 😓
No strong opinion on this, in the end both are fine for me.
joamaki
commented
Oct 9, 2024
| @@ -208,6 +211,32 @@ func (s *Set[T]) UnmarshalJSON(data []byte) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (s Set[T]) MarshalYAML() (any, error) { | |||
There was a problem hiding this comment.
Added this for use in testdata/db.txtar as the test objects used Set[string] for tags.
Realized I need the yaml support for part.Map as well. And these need tests.
There was a problem hiding this comment.
Done and moved to its own commit.
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
from
950e499
to
204b1f3
Compare
joamaki
commented
Oct 9, 2024
|
|
||
| // FromString is an optional conversion from string to a raw key. | ||
| // If implemented allows script commands to query with this index. | ||
| FromString func(key string) (index.Key, error) |
There was a problem hiding this comment.
I wonder if we want to make this mandatory or not. Would be a breaking change if we require it.
Might also make sense to have constructors for Index instead of having to fill in the struct.
I'm inclined to not make this mandatory as of yet as I want these changes to be non-breaking for now.
joamaki
commented
Oct 9, 2024
| return fmt.Errorf("-columns not supported with non-table formats") | ||
| } | ||
| switch format { | ||
| case "yaml": |
There was a problem hiding this comment.
Kind of tempted to pull in https://github.com/itchyny/gojq into this to allow assertions with a jq query. Otoh, it's a pretty big dependency so perhaps best to keep this minimal until there's a real need.
joamaki
commented
Oct 9, 2024
| case "table": | ||
| header := tbl.TableHeader() | ||
| if header == nil { | ||
| return fmt.Errorf("objects in table %q not TableWritable", tbl.Meta.Name()) |
There was a problem hiding this comment.
Here's perhaps a good reason to change NewTable[Obj any] into NewTable[Obj TableWritable] (or maybe make a TableObject constraint). Again don't want to make this into a breaking change yet, but maybe for statedb v0.4...
To allow using part.Set[] and part.Map[] types with the YAML marshalling/unmarshalling in the StateDB script commands, add the custom YAML marshalling and unmarshalling methods for these types. Signed-off-by: Jussi Maki <[email protected]>
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
2 times, most recently
from
06dad38
to
58f01e1
Compare
pippolo84
reviewed
Oct 10, 2024
Signed-off-by: Jussi Maki <[email protected]>
Reimplement the reconciler tests using scripttest. This significantly simplifies the test-suite and allows easier verification of more complex scenarios. To allow for Status JSON and YAML marshalling, define custom UnmarshalJSON and UnmarshalYAML that also fill in the 'id'. The 'id' is used with StatusSet to efficiently allow multiple reconcilers to manipulate the status without conflicts, e.g. if the object status id is the same it can still be updated with the reconciliation result even if the object conflicted due to other reconciler's update to its status. Signed-off-by: Jussi Maki <[email protected]>
Add *String counterparts (Int32 => Int32String) for implementing FromString in Index[]. Signed-off-by: Jussi Maki <[email protected]>
joamaki
force-pushed
the
pr/joamaki/hivescript
branch
from
58f01e1
to
4a2e8f4
Compare
pippolo84
approved these changes
Oct 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reimplement the script commands with the hive/script library. And with these reimplement the reconciler tests. I tried to be faithful to the original test cases, but please double-check.
See testdata/db.txtar for an overview of the commands.
I'll do a separate pass to add documentation to the README here and to the cilium docs once we've gotten a bit more experience with this.