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

exposing disableInlining option via Compile API #4378

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

srlk
Copy link
Contributor

@srlk srlk commented Feb 24, 2022

This change enables consumers of Compile API to specify packages those should not be inlined in partial evaluation response.

Fixes: #4357
Signed-off-by: Serol Kosunda skosunda@adobe.com

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! Just a nitpick, this looks good.

Since you've marked it draft (sorry for reviewing WIP) -- what is it you'd like to add or wait for?

| --- | --- | --- | --- |
| `query` | `string` | Yes | The query to partially evaluate and compile. |
| `input` | `any` | No | The input document to use during partial evaluation (default: undefined). |
| `options` | `any` | No | Additional options to use during partial evaluation. Only `disableInlining` option is supported. (default: undefined). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say object[string, any] or something like that? The any for input really means anything -- an integer, a boolean, some object, ... This one is more restricted, I believe.

server/server.go Outdated
@@ -2712,10 +2718,15 @@ func readInputCompilePostV1(r io.ReadCloser) (*compileRequest, *types.ErrorV1) {
}
}

options := compileRequestOptions{
DisableInlining: request.Options.DisableInlining,
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 It's not entirely clear to me why it's safe to access request.Options.DisableInlining -- do we know that request.Options can't be nil? Ah, that's because the field is struct{} not *struct{}, hm? OK.

We could inline it, then:

	result := &compileRequest{
		Query:    query,
		Input:    input,
		Unknowns: unknowns,
		Options:  compileRequestOptions{
				DisableInlining: request.Options.DisableInlining,
		},
	}

@@ -1281,10 +1281,11 @@ Evaluation in OPA, see [this post on blog.openpolicyagent.org](https://blog.open

Compile API requests contain the following fields:

| Field | Type | Requried | Description |
| Field | Type | Required | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@srlk
Copy link
Contributor Author

srlk commented Feb 24, 2022

Thanks for picking this up! Just a nitpick, this looks good.

Since you've marked it draft (sorry for reviewing WIP) -- what is it you'd like to add or wait for?

Thanks for the swift review @srenatus , I'm keeping it on review until the checks runs are completed.

@srlk srlk marked this pull request as ready for review February 24, 2022 09:09
@srlk srlk requested a review from srenatus February 24, 2022 09:55
srenatus
srenatus previously approved these changes Feb 24, 2022
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks again!

Would you mind squashing your commits and adding a brief commit message? (https://www.openpolicyagent.org/docs/edge/contrib-code/#commit-messages)

This change enables consumers of Compile API to specify packages those should not be inlined in partial evaluation response.

Fixes: open-policy-agent#4357
Signed-off-by: skosunda <skosunda@adobe.com>
@srlk
Copy link
Contributor Author

srlk commented Feb 24, 2022

Thanks again!

Would you mind squashing your commits and adding a brief commit message? (https://www.openpolicyagent.org/docs/edge/contrib-code/#commit-messages)

Sure thing, done

@srlk srlk requested a review from srenatus February 24, 2022 10:35
@srenatus srenatus merged commit 3ea03ea into open-policy-agent:main Feb 24, 2022
@srenatus
Copy link
Contributor

Thanks again!

@srlk srlk deleted the issue/4357 branch February 24, 2022 12:18
damienjburks pushed a commit to damienjburks/opa that referenced this pull request Mar 9, 2022
author Itay Shakury <itay@itaysk.com> 1646501263 +0200
committer Damien Burks <damien@damienjburks.com> 1646794189 -0600

docs: add missing title to annotations index (open-policy-agent#4406)

Signed-off-by: Itay Shakury <itay@itaysk.com>

Update philosophy.md (open-policy-agent#4412)

I am suggesting this change to fix grammatical errors which will increase readability for consumers of the OPA Philosophy document.

---

1. I noticed there is a missing word ("be") in the OPA Document Model section, just above the table that summarizes the different models for loading base documents into OPA. This change adds the omitted word.

2. I've also made a change to the first note at the bottom of the page ([1]). "However" is used as a conjunctive adverb, I've updated the sentence's punctuation to reflect this.

3. Lastly, I've made a punctuation change to note number three at the bottom of the page ([3])  to increase readability.

Signed-off-by: Arthur Jones <arthur.h.jones3@gmail.com>

Co-authored-by: Stephan Renatus <stephan.renatus@gmail.com>

docs: mention who created OPA and who owns it (open-policy-agent#4414)

These are two common questions that come up and we do not mention this
anywhere else in the docs.

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

committing latest changes

Signed-off-by: Damien Burks <damien@damienjburks.com>

committing latest changes to unsued imports function

Signed-off-by: Damien Burks <damien@damienjburks.com>

removing print statements and adding more test cases

Signed-off-by: Damien Burks <damien@damienjburks.com>

server: exposing disableInlining option via Compile API (open-policy-agent#4378)

This change enables consumers of Compile API to specify packages
those should not be inlined in partial evaluation response.

Fixes: open-policy-agent#4357

Signed-off-by: skosunda <skosunda@adobe.com>

docs: Update Envoy primer to use variables instead of objects

Previously, the Envoy primer provided samples of policies for
status-code, body, and headers that explicitly returned an object
in individual rules instead of the more common and recommended
approach of using separate variables for each concern.

This change replaces those sample policies with ones that use
variables for each of allow, status_code, headers, body,
and it adds a section on the output expected by Envoy,
where we include the snippet combining those variables into
the JSON object expected by Envoy.

Signed-off-by: Tim Hinrichs <tim@styra.com>

make: Disable WASM by default on darwin/arm64 (open-policy-agent#4382)

Signed-off-by: Anders Eknert <anders@eknert.com>

docs: Update Istio tutorial to expose application to outside traffic (open-policy-agent#4384)

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

fixing linting issues

Signed-off-by: Damien Burks <damien@damienjburks.com>

committing latest changes for PR

Signed-off-by: Damien Burks <damien@damienjburks.com>

adding info to strict.md

adding switch statement for 'in' operator

adding additional case to if statement

finalizing changes for broken test cases

Signed-off-by: Damien Burks <damien@damienjburks.com>
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.

Support to expose disableInlining via Compile API
2 participants