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

Support named function arguments for documentation and tooling #4080

Closed
anderseknert opened this issue Dec 2, 2021 · 7 comments
Closed

Support named function arguments for documentation and tooling #4080

anderseknert opened this issue Dec 2, 2021 · 7 comments

Comments

@anderseknert
Copy link
Member

What part of OPA would you like to see improved?

In order to leverage features like tool tips, auto-suggestions and in-line documentation in the various tools used for policy authoring, it would be useful if OPA supported named function arguments, and that these names got included in the capabilities.json file included with each release (as that's easily parsed by that type of tools).

Some considerations:

  • Backwards compatibility for existing custom functions (i.e. plugins) - could default to argX or similar.
  • We need to establish good and consistent naming conventions.
  • Use function signatures, including argument names, to generate (at least parts of) the policy reference section of the docs.

Out of scope:

  • Named args in function calls, i.e. no startswith(str="abcd", prefix="ab"). This is mainly for documentation and tooling.
  • Retroactively do this for older versions of OPA. Can be considered later if requested.
  • Functions declared in policy. These could use annotations or similar metadata for the same effect.
@stale
Copy link

stale bot commented Jan 1, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Jan 1, 2022
@anderseknert
Copy link
Member Author

anderseknert commented Jan 17, 2022

Given we had this, an entry in the capabilities file might look something like the below:

{
    "name": "abs",
    "description": "return the absolute value of x",
    "wasm_supported": true,
    "introduced_in": "v0.11.0",
    "decl": {
        "args": [
            {
                "name": "x",
                "type": "number",
                "description": "the number to return in absolute form"
            }
        ],
        "result": {
            "type": "number",
            "description": "the absolute value of x"
        },
        "type": "function"
    }
}

This would be easy to use as a source for e.g. both the policy reference docs as well as VS Code, Playground, etc.

Wasm support could be determined from https://github.com/open-policy-agent/opa/blob/v0.36.1/internal/compiler/wasm/wasm.go#L83-L176

As @srenatus pointed out, introduced_in could be determined by diffing the capabilities files from all versions, but it would be rather nice to export a single capabilities.json file to tooling authors.

@stale stale bot removed the inactive label Jan 17, 2022
@srenatus
Copy link
Contributor

🤔 I'd propose the "introduced in" section to be changed just a bit:

    "name": "abs",
    "description": "return the absolute value of x",
    "introduced": { 
      "rego": "v0.11.0",
      "wasm": "v0.21.0"
    },

or perhaps putting all of that into an extra key,

    "name": "abs",
    "meta": {
      "description": "return the absolute value of x",
      "introduced": { 
        "rego": "v0.11.0",
        "wasm": "v0.21.0"
      },
    }

@stale
Copy link

stale bot commented Feb 16, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@srenatus
Copy link
Contributor

$ opa eval -fpretty 'startswith(2, "foo")'              
1 error occurred: 1:1: rego_type_error: startswith: invalid argument(s)
        have: (number, string, ???)
        want: (base: string, search: string, result: boolean)

Yay or nay? I think yay. Let's do this.

For context, in some WIP I'm adding names to the function arguments and return values -- I have not taken on the extra desirables about version (wasm) support yet.

@anderseknert
Copy link
Member Author

Definitely yay!

@anderseknert
Copy link
Member Author

Fixed by #4705

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants