opa icon indicating copy to clipboard operation
opa copied to clipboard

Allow multiple imports on a single line

Open anderseknert opened this issue 2 years ago • 4 comments

It would be nice enhancement if there was support for importing multiple rules, or future keywords on a single line. As it currently stands, explicitly importing all the future keywords — which we recommend over the "catch all" import — plus any rules or functions required from other packages, is a bit verbose, and tends to distract from what's important when demonstrating policy authoring. I.e. rather than having to do something like:

package policy

import future.keywords.every
import future.keywords.contains
import future.keywords.if

import data.other.policy.foo
import data.other.policy.bar as b

deny contains "not on my watch!" if {
    every x in foo {
        x == b
    }
}

One would be able to do something like:

package policy

import future.keywords [every, contains, if]

import data.other.policy [foo, bar as b]

deny contains "not on my watch!" if {
    every x in foo {
        x == b
    }
}

Open to suggestions on the syntax — whatever looks best on a single line is fine with me :)

anderseknert avatar Sep 07 '22 13:09 anderseknert

The obvious alternative... but I don't know python so this might or might not be similar enough.

from future.keywords import [every, contains, if]

srenatus avatar Sep 07 '22 13:09 srenatus

Yeah, that'd be nice... although it would require the use of a new from keyword... and it's not clear how that could be imported

from future.keywords import [from]

😆

anderseknert avatar Sep 07 '22 13:09 anderseknert

There's also Haskell's import syntax. They allow qualified, unqualified, and renamed imports, as well as the ability to import all names except a selection from the other module, which is interesting.

philipaconrad avatar Sep 11 '22 20:09 philipaconrad

Yeah.. another request that's come up a few times is to forbid imports that don't "resolve" to something, like... I guess something close to how PHP differentiates between "import" and "require". Or to be able to exclude something from the stdlib or a keyword, in order to use that for your own custom functions or rules. Both could be useful, but let's keep the scope to multiple imports for this issue 😄

This is not much of a problem in "production" policies, but it would help a lot with providing concise, copy-pasteable snippets, like in the Rego Style Guide, or as shown in presentations and demos.

anderseknert avatar Sep 11 '22 21:09 anderseknert

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

stale[bot] avatar Oct 19 '22 12:10 stale[bot]

This is quite an interesting one, I might play around with this if its okay and see if I can make something cool :smile:

Parsifal-M avatar Oct 31 '22 14:10 Parsifal-M

I'd love to see it @Parsifal-M 😃

anderseknert avatar Oct 31 '22 16:10 anderseknert

Is there consensus on how to do it? I presume we'll go with

import future.keywords [every, contains, if]

import data.other.policy [foo, bar as b]

srenatus avatar Nov 01 '22 10:11 srenatus

Is there consensus on how to do it? I presume we'll go with

import future.keywords [every, contains, if]

import data.other.policy [foo, bar as b]

question

This is what I had in mind to try out :+1: , LMK if you think otherwise.

Parsifal-M avatar Nov 01 '22 11:11 Parsifal-M

It's fine. I just wanted everyone to be on the same page.

I think an interesting question here is capabilities: How can we introduce this change so that the user can take the capabilities file of some OPA version before this feature was introduced, and have it fail in a sensible way?

(Usually, you'll have a myriad of OPA versions running in your installation, and you'd take the oldest running version's capabilities file when bundling policies for your fleet.)

srenatus avatar Nov 01 '22 11:11 srenatus

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

stale[bot] avatar Dec 01 '22 20:12 stale[bot]

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

stale[bot] avatar Apr 07 '23 09:04 stale[bot]

Is anyone working on this issue?

26tanishabanik avatar Jul 06 '23 08:07 26tanishabanik

I started understanding parser file under ast folder and token and scaner files from internal, but kind of wondering where to get started with so that the new form of import import future.keywords [every, contains, if] can be parsed

26tanishabanik avatar Jul 06 '23 09:07 26tanishabanik

That's awesome @26tanishabanik 😃 I'd imagine the tokenizer would need to be made aware of the new format, and that the parser would "rewrite" the short form syntax into the longer form. I'd need to look into it, but I'm currently on leave :) So perhaps @srenatus or @johanfylling could help provide some pointers.

One thing that I hadn't really considered is the compatibility problem. Any policy using short form imports would be incompatible with previous version of OPA. I can't really think of any way around that 🤔 I guess we could have tooling that rewrote the short form to it's longer one when e.g. building bundles, or whatnot... but it would obviously not cover all cases. Perhaps we could sneak in support but not make it official / documented until X releases later, as we'd then have at least X number of past versions that supported it 😅

Not sure if we're OK with biting that bullet. Personally, I'm in favor, but I'd appreciate hearing what others think about that.

anderseknert avatar Jul 06 '23 14:07 anderseknert

That's awesome @26tanishabanik 😃 I'd imagine the tokenizer would need to be made aware of the new format, and that the parser would "rewrite" the short form syntax into the longer form. I'd need to look into it, but I'm currently on leave :) So perhaps @srenatus or @johanfylling could help provide some pointers.

One thing that I hadn't really considered is the compatibility problem. Any policy using short form imports would be incompatible with previous version of OPA. I can't really think of any way around that 🤔 I guess we could have tooling that rewrote the short form to it's longer one when e.g. building bundles, or whatnot... but it would obviously not cover all cases. Perhaps we could sneak in support but not make it official / documented until X releases later, as we'd then have at least X number of past versions that supported it 😅

Not sure if we're OK with biting that bullet. Personally, I'm in favor, but I'd appreciate hearing what others think about that.

Thanks a lot @anderseknert for your inputs. I will surely wait for @srenatus and @johanfylling to give their inputs too. This is my first time working with parser so would need some pointers 😬

26tanishabanik avatar Jul 06 '23 14:07 26tanishabanik

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

stale[bot] avatar Aug 05 '23 20:08 stale[bot]

I'm going to close this one for now, seeing as the future keywords are soon to be made included by default. There's still a case to be made for this and regular imports, I think, but it's not as urgent, and would still require new syntax. If anyone else thinks this is important, let me know.

anderseknert avatar Oct 02 '23 09:10 anderseknert