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

feat: tuple write/delete cmd accepts file; deprecate tuple import #165

Merged
merged 5 commits into from
Sep 27, 2023
Merged

feat: tuple write/delete cmd accepts file; deprecate tuple import #165

merged 5 commits into from
Sep 27, 2023

Conversation

gabrielbussolo
Copy link
Contributor

@gabrielbussolo gabrielbussolo commented Sep 24, 2023

aims to fix #161

validates if one of the conditions met: a file or the 3 arguments

➜  openfga-cli git:(161-add...) ✗ ./dist/fga tuple write 
Error: you need to specify either 3 arguments or a file

writing with a file

./dist/fga tuple write --store-id 01HB3AZ6RAAGHJQ9HKXV9C89SE --file file.json  
{
  "successful": [
    {
      "object":"document:Z",
      "relation":"reader",
      "user":"user:anne"
    },
    {
      "object":"document:Z",
      "relation":"reader",
      "user":"user:jones"
    },
    {
      "object":"document:Z",
      "relation":"reader",
      "user":"user:gabriel"
    }
  ],
  "failed":null
}

file: file.json

[
    {
        "user":"user:anne",
        "relation":"reader",
        "object":"document:Z"
    },
    {
        "user":"user:jones",
        "relation":"reader",
        "object":"document:Z"
    },
    {
        "user":"user:gabriel",
        "relation":"reader",
        "object":"document:Z"
    }
]

file.yaml

- user: "user:anne"
  relation: "reader"
  object: "document:Z"
- user: "user:jones"
  relation: "reader"
  object: "document:Z"
- user: "user:gabriel"
  relation: "reader"
  object: "document:Z"

keep the current behavior of insert with args:

➜  openfga-cli git:(161-add...) ✗ ./dist/fga tuple write user:hello reader document:Z
{}

there is no rule to warn if both are informed (file AND args) but args will be ignored

delete command works same

Deprecate tuple import command

 ./dist/fga tuple import --store-id 01HB3AZ6RAAGHJQ9HKXV9C89SE --file file.json
Command "import" is deprecated, use the write/delete command with the flag --file instead

Description

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@gabrielbussolo gabrielbussolo marked this pull request as ready for review September 26, 2023 08:48
@gabrielbussolo gabrielbussolo requested a review from a team as a code owner September 26, 2023 08:48
@gabrielbussolo gabrielbussolo changed the title [WIP]feat: write and delete tuples from json file Accept --file flag on write/delete tuple command and deprecate import command Sep 26, 2023
@gabrielbussolo
Copy link
Contributor Author

if you can take a look @rhamzeh ;D

@rhamzeh rhamzeh changed the title Accept --file flag on write/delete tuple command and deprecate import command feat: accept --file flag on write/delete tuple command and deprecate import command Sep 26, 2023
@rhamzeh rhamzeh changed the title feat: accept --file flag on write/delete tuple command and deprecate import command feat: tuple write/delete cmd accepts file; deprecate tuple import Sep 26, 2023
Copy link
Member

@rhamzeh rhamzeh left a comment

Choose a reason for hiding this comment

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

Thanks! Can I also ask you to run make format and make lint to make sure we got rid of any lint errors?

cmd/tuple/delete.go Show resolved Hide resolved
cmd/tuple/import.go Outdated Show resolved Hide resolved
cmd/tuple/write.go Show resolved Hide resolved
@rhamzeh
Copy link
Member

rhamzeh commented Sep 26, 2023

@gabrielbussolo may I ask you to also update the README in this PR to document the new options for write and delete?

@gabrielbussolo
Copy link
Contributor Author

@gabrielbussolo may I ask you to also update the README in this PR to document the new options for write and delete?

sure thing @rhamzeh, ill do it tomorrow ;D

@gabrielbussolo
Copy link
Contributor Author

@gabrielbussolo may I ask you to also update the README in this PR to document the new options for write and delete?

Updated the README, also fixed the comments. ;D

There is a specific lint screaming at me:

➜  openfga-cli git:(161-add-support-for-specifying-a-list-of-tuples-to-delete) ✗ make lint
go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest && golangci-lint run
# github.com/golangci/golangci-lint/cmd/golangci-lint
ld: warning: -bind_at_load is deprecated on macOS
cmd/tuple/delete.go:111:11: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"at least %d arg(s) are required OR the flag --%s\", n, flag)" (goerr113)
                        return fmt.Errorf("at least %d arg(s) are required OR the flag --%s", n, flag)
                               ^
make: *** [lint] Error 1

is asking to create a specific error for it, but on this case I dont think it makes sense once i need to format the error before I send it (same approach as the cobra.ExactArgs).

also, about the lint, it forces to create a const for a number, and its ok on some contexts, but when we have a function such as cobra.ExactArgs(3) its pretty explicit what it does, do imho we could ignore it.

the first one i didnt solved, the second one i created a const. What is the team approach on it?

@rhamzeh
Copy link
Member

rhamzeh commented Sep 27, 2023

the first one i didnt solved, the second one i created a const. What is the team approach on it?

For both of these cases, it's safe to add a lint ignore.

Other than this lint issue, good to merge!

@gabrielbussolo
Copy link
Contributor Author

Other than this lint issue, good to merge!

perfect, so its done @rhamzeh

@rhamzeh
Copy link
Member

rhamzeh commented Sep 27, 2023

Thanks @gabrielbussolo!

@rhamzeh rhamzeh added this pull request to the merge queue Sep 27, 2023
Merged via the queue into openfga:main with commit 72e7b06 Sep 27, 2023
@gabrielbussolo gabrielbussolo deleted the 161-add-support-for-specifying-a-list-of-tuples-to-delete branch September 27, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for specifying a list of tuples to delete
2 participants