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

ast: Adding capability feature for the rego.v1 import #6375

Merged
merged 11 commits into from
Nov 15, 2023

Conversation

johanfylling
Copy link
Contributor

Resolves: #6366

Resolves: open-policy-agent#6366
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 3067859
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/65549fc0e600b0000798e09d
😎 Deploy Preview https://deploy-preview-6375--openpolicyagent.netlify.app/docs/edge/deployments
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rego/rego.go Outdated
@@ -1837,6 +1837,7 @@ func (r *Rego) loadFiles(ctx context.Context, txn storage.Transaction, m metrics
defer m.Timer(metrics.RegoLoadFiles).Stop()

result, err := loader.NewFileLoader().
WithCapabilities(r.capabilities).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the parser didn't have access to the capabilities file passed in via e.g. opa eval --capabilities. So effectively, there was no way of turning off individual future keywords.

I'm pondering breaking this out into its own bug report and PR 🤔 .

Copy link
Member

Choose a reason for hiding this comment

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

A new PR or a separate commit should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
@@ -342,6 +342,7 @@
"v0.57.0",
"v0.57.1",
"v0.58.0",
"v0.59.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what ramifications making this update now has .. but this change and adding capabilities/v0.59.0.json is necessary for the new tests to pass.

Does anyone know if this will mess anything up, and/or if there is an alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we're using published git tags for building the releases.yaml file used for iterating versions in the docs. So this might be safe after all, at least when it concerns docs ..

Copy link
Member

Choose a reason for hiding this comment

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

Do you have an idea if this has any impact when we cut the actual v0.59.0 release? I would imagine the release scripts would just overwrite this but I'm not sure. Als it's a bit weird we have published v0.59.0 artifacts w/o an actual release but that's ok I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the web deployment preview of #6346, it doesn't look like this will break the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running the release-patch make target on this branch also has no unexpected ill effects. So I think we're good to go here :) .

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
@@ -449,6 +449,18 @@ Note that the metaschemas [http://json-schema.org/draft-04/schema](http://json-s

Similarly, the `allow_net` capability restricts what hosts the `http.send` built-in function may send requests to, and what hosts the `net.lookup_ip_addr` built-in function may resolve IP addresses for.

### Features
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additional information is added to this section in #6346.

anderseknert
anderseknert previously approved these changes Nov 8, 2023
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question, but I'm not sure if it's relevant.

"Minor": 59,
"Patch": 0,
"PreRelease": "",
"Metadata": ""
Copy link
Member

Choose a reason for hiding this comment

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

What's metadata here? Seem like it would be good to have a description of what this means, but perhaps this is used for something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know, to be honest. This is generated by

, which is out of scope for this PR.

ashutosh-narkar
ashutosh-narkar previously approved these changes Nov 8, 2023
Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

LGTM

ast/parser.go Outdated
@@ -2561,6 +2561,11 @@ func (p *Parser) futureImport(imp *Import, allowedFutureKeywords map[string]toke
}

func (p *Parser) regoV1Import(imp *Import) {
if !p.po.Capabilities.ContainsFeature(FeatureRegoV1Import) {
p.errorf(imp.Path.Location, "Invalid import, `%s` is not supported by current capabilities", regoV1CompatibleRef)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Invalid -> invalid to stay consistent with the error format.

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Conflicts:
	ast/capabilities.go
	ast/capabilities_test.go
	capabilities.json
	capabilities/v0.59.0.json
	docs/content/deployments.md
@johanfylling johanfylling merged commit bcf82eb into open-policy-agent:main Nov 15, 2023
24 checks passed
@johanfylling johanfylling deleted the rego-v1/capability branch November 15, 2023 11:16
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 capability for import rego.v1
3 participants