Remove LookupResources v1 implementation #2099
Conversation
github-actions
bot
added
area/CLI
| }) | ||
| } | ||
| for _, chunkSize := range []uint16{5, 10} { | ||
| t.Run(fmt.Sprintf("chunk-size-%t", chunkSize), func(t *testing.T) { |
There was a problem hiding this comment.
I think this is what should be reported here but I'm not 100% sure.
internal/services/v1/permissions.go
Outdated
| if req.OptionalCursor != nil { | ||
| _, ok, err := cursor.GetCursorFlag(req.OptionalCursor, lrv2CursorFlag) | ||
| if err != nil { | ||
| return ps.rewriteError(resp.Context(), err) | ||
| } | ||
|
|
||
| if ok { | ||
| return ps.lookupResources2(req, resp) | ||
| } | ||
| } |
There was a problem hiding this comment.
I figured this was no longer necessary since the flag itself isn't really necessary anymore.
| @@ -641,7 +641,6 @@ func TestLookupResources(t *testing.T) { | |||
| MaxPreconditionsCount: 1000, | |||
| StreamingAPITimeout: 30 * time.Second, | |||
| MaxRelationshipContextSize: 25000, | |||
| UseExperimentalLookupResources2: useV2, | |||
There was a problem hiding this comment.
There's a bunch of changes like this that are just propagating the option removal.
pkg/proto/core/v1/util.go
Outdated
There was a problem hiding this comment.
I ran deadcode against the project and this file was called out as being entirely unused. I'm assuming that means it can be deleted.
| @@ -393,158 +393,7 @@ func TranslateExpansionTree(node *core.RelationTupleTreeNode) *v1.PermissionRela | |||
| } | |||
| } | |||
|
|
|||
| const lrv2CursorFlag = "lrv2" | |||
There was a problem hiding this comment.
keep the cursor flag; we might need it again in the future and we don't want to break existing cursors. we should just return an error if the cursor does not have it
There was a problem hiding this comment.
I don't see this change reverted
| @@ -158,7 +158,6 @@ func RegisterServeFlags(cmd *cobra.Command, config *server.Config) error { | |||
|
|
|||
| experimentalFlags := nfs.FlagSet(BoldBlue("Experimental")) | |||
| // Flags for experimental features | |||
| experimentalFlags.BoolVar(&config.EnableExperimentalLookupResources, "enable-experimental-lookup-resources", true, "enables the experimental version of the lookup resources API") | |||
There was a problem hiding this comment.
don't remove, as it will break callers. Instead, deprecate it and if someone gives a value of false, issue a warning that the flag will be removed in the future
tstirrat15
force-pushed
the
remove-lookup-resources-v1
branch
from
4b2f970
to
ada2c56
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Now that v2 is on by default, this cleans up the old v1 implementation. It should also reduce our test times by getting rid of a branch of the consistency tests.
Changes
Will annotate.
Testing
See that everything goes green.