-
Notifications
You must be signed in to change notification settings - Fork 36
breaking: Clean up deprecated functionality for v1 #562
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #562 +/- ##
==========================================
+ Coverage 39.28% 41.92% +2.64%
==========================================
Files 37 37
Lines 5448 4718 -730
==========================================
- Hits 2140 1978 -162
+ Misses 3063 2482 -581
- Partials 245 258 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
54f4359 to
f54b029
Compare
| registerConsistencyFlags(checkCmd.Flags()) | ||
|
|
||
| permissionCmd.AddCommand(checkBulkCmd) | ||
| checkBulkCmd.Flags().String("revision", "", "optional revision at which to check") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one wasn't marked as hidden, can we actually remove it?
i also see this
readCmd.Flags().String("revision", "", "optional revision at which to check")
_ = readCmd.Flags().MarkHidden("revision")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue yes, as long as we document it
|
I wonder if we could take this PR as an opportunity to:
This would be in the name of consistency 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran mage gen:docForPublish and inspected the docs at https://github.com/authzed/zed/blob/main/docs/zed.md. I saw this, can we update it?
Lines 97 to 101 in 57fdb03
| Example: ` | |
| Write to stdout: | |
| zed preview schema compile root.zed | |
| Write to an output file: | |
| zed preview schema compile root.zed --out compiled.zed |
f54b029 to
f3e2bb1
Compare
f3e2bb1 to
e5c14c4
Compare
tstirrat15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
|
|
||
| relCmd := commands.RegisterRelationshipCmd(rootCmd) | ||
|
|
||
| commands.RegisterWatchCmd(rootCmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the aliases that we removed.
| schemaCompileCmd := registerAdditionalSchemaCmds(schemaCmd) | ||
| registerPreviewCmd(rootCmd, schemaCompileCmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer pass out the schemaCompileCmd as it's no longer needed as an arg to the preview command.
| schemaCmd := &cobra.Command{ | ||
| Use: "schema <subcommand>", | ||
| Short: "Manage schema for a permissions system", | ||
| Deprecated: "please use `zed schema compile`", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer any commands under this.
| schemaCmd.AddCommand(schemaCompileCmd) | ||
| schemaCompileCmd.Flags().String("out", "", "output filepath; omitting writes to stdout") | ||
|
|
||
| return schemaCompileCmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note about no longer passing things out of this command.
| // Deprecated (hidden) flag. | ||
| if revision := cobrautil.MustGetStringExpanded(cmd, "revision"); revision != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now covered by the consistency option; we want to get rid of the old way of doing things.
| Short: "Check permissions in bulk exist for resource-permission-subject triplets", | ||
| Args: ValidationWrapper(cobra.MinimumNArgs(1)), | ||
| RunE: checkBulkCmdFunc, | ||
| Aliases: []string{"check-bulk", "bulk-check"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding aliases here.
| lookupCmd := &cobra.Command{ | ||
| Use: "lookup <type> <permission> <subject:id>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer using a separate command for lookup. This is covered by lookup-resources.
|
|
||
| permissionCmd.AddCommand(expandCmd) | ||
| expandCmd.Flags().Bool("json", false, "output as JSON") | ||
| expandCmd.Flags().String("revision", "", "optional revision at which to check") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal here.
| Args: ValidationWrapper(StdinOrExactArgs(3)), | ||
| ValidArgsFunction: GetArgs(ResourceID, Permission, SubjectTypeWithOptionalRelation), | ||
| RunE: writeRelationshipCmdFunc(v1.RelationshipUpdate_OPERATION_CREATE, os.Stdin), | ||
| Aliases: []string{"write"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding zed relationship write as an alias - this is what i always want to write.
Description
Part of getting a v1 of Zed out the door. We want to take the opportunity to get rid of some deprecated codepaths, which will keep complexity down in the long term.
Changes
zed preview schema compile- it's now justzed schema compilezed watch- it's now justzed relationship watchrevisionflag - use the other consistency flagszed permission lookup- it'szed permission lookup-resourcesestimated-countflag onbulk-delete- uselimitinsteadTesting
Review. See that tests still pass.