Add --format discard to opa eval #6080

26tanishabanik · 2023-07-06T16:54:28Z

Why the changes in this PR are needed?

fixes #5863

What are the changes in this PR?

adds a subcommand for format which will discard the result

Notes to assist PR review:

Further comments:

internal/presentation/presentation.go

tester/runner.go

charlieegan3

Looking good, I've left a few suggestions 🙂

cmd/eval_test.go

charlieegan3

Looking good! One final thing would be to add some docs, I think we can just add one line here:

opa/cmd/eval.go

Line 236 in ecac4c7

    
               --format=raw       : output the values from query results in a scripting friendly format

cmd/eval_test.go

26tanishabanik · 2023-07-11T13:47:27Z

Looking good! One final thing would be to add some docs, I think we can just add one line here:

opa/cmd/eval.go

Line 236 in ecac4c7

--format=raw : output the values from query results in a scripting friendly format

Sure, I missed it, my bad

26tanishabanik · 2023-07-11T13:53:28Z

Looking good! One final thing would be to add some docs, I think we can just add one line here:

opa/cmd/eval.go

Line 236 in ecac4c7

--format=raw : output the values from query results in a scripting friendly format

Sure, I missed it, my bad

Is this fine output the result field as "discarded" ?

charlieegan3

Thanks for all your work on this @26tanishabanik. I think it looks good now 🙂. I'll mention to the others to give this a final look over before we get it merged in.

charlieegan3 · 2023-07-11T14:59:46Z

cmd/eval.go

@@ -232,6 +234,7 @@ Set the output format with the --format flag.
    --format=pretty    : output query results in a human-readable format
    --format=source    : output partial evaluation results in a source format
    --format=raw       : output the values from query results in a scripting friendly format
+	--format=discard   : output the result field as "discarded"


This looks great, thanks for adding it 🙂!

Nit: Seems to mis-aligned with the previous line. Also in output the result field as "discarded" lets clarify if this is the case when the result is non-nil.

@26tanishabanik can you please look into 👆

Yes, I had formatted it, and now did it again 🤔, have pushed it the change

cmd/eval_test.go

26tanishabanik · 2023-07-11T15:13:50Z

Thanks for all your work on this @26tanishabanik. I think it looks good now 🙂. I'll mention to the others to give this a final look over before we get it merged in.

Sure, thanks a lot 🙂

ashutosh-narkar

@26tanishabanik the changes look good. Few comments inline. Thanks for working on this!

ashutosh-narkar · 2023-07-11T16:30:25Z

cmd/eval.go

@@ -232,6 +234,7 @@ Set the output format with the --format flag.
    --format=pretty    : output query results in a human-readable format
    --format=source    : output partial evaluation results in a source format
    --format=raw       : output the values from query results in a scripting friendly format
+	--format=discard   : output the result field as "discarded"


Nit: Seems to mis-aligned with the previous line. Also in output the result field as "discarded" lets clarify if this is the case when the result is non-nil.

ashutosh-narkar · 2023-07-11T16:32:28Z

go.sum

+golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df h1:UA2aFVmmsIlefxMk29Dp2juaUSth8Pyn3Tq5Y5mJGME=
+golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df/go.mod h1:FXUEEKJgO7OQYeo8N01OfiKP8RXMtf6e8aTskBGqWdc=


Why did this file change?

I am not sure, might have changed when running go mod tidy, I think

I have reverted the change

ashutosh-narkar · 2023-07-11T16:34:49Z

cmd/eval_test.go

@@ -1312,6 +1312,116 @@ time.clock(input.y, time.clock(input.x))
 	}
 }

+func TestEvalDiscardOutput(t *testing.T) {


It would be helpful if we add a test case for the scenario when the result is nil.

I have added that as a second case in that tests struct

I would have expected opa eval '1*2+3' -format=discard to produce nothing unless something like --profile was set. @charlieegan3 and @26tanishabanik am I missing something here?

I think, if I remember properly, the idea was to set it empty if the result is itself nil

I see three desirable behaviours as goals for this:

Undefined results are not shown as discarded as they were never there in the first place.

Discarded results appear differently from undefined results to make the distinction clear.

--format=discard will always produce valid JSON. From the issue (Add --format discard to opa eval #5863) it's meant to be the 'opposite' of --format=raw which removes all structure around the result.

In the go run main.go eval '1*2+3' --format=discard example, the result defined and so is discarded in the output. We can't return {} since it'd break goal 2.

$ go run main.go eval '1*2+3' --format=discard { "result": "discarded" }

I think this handling of undefined results is also correct:

$ go run main.go eval '1' --format=discard { "result": "discarded" } $ go run main.go eval 'input.undefined' --format=discard {}

(I think that what we have at the moment does this, but open to other ideas)

How should we proceed further?

I'll let @ashutosh-narkar weigh in to check we're all in agreement on the desired outputs first. I can work with you again on this tomorrow if we need to make any updates 🙂

Sure, thanks 🙂

This sounds fine. Thanks for the clarification @26tanishabanik and @charlieegan3.

26tanishabanik · 2023-07-11T17:05:31Z

@charlieegan3 , @ashutosh-narkar , not sure, why this error is coming:
https://github.com/open-policy-agent/opa/actions/runs/5522647272/jobs/10072559658?pr=6080

ashutosh-narkar · 2023-07-11T17:08:30Z

https://github.com/open-policy-agent/opa/actions/runs/5522647272/jobs/10072559658?pr=6080

Will re-run the test.

ashutosh-narkar · 2023-07-12T16:14:34Z

cmd/eval.go

@@ -232,6 +234,7 @@ Set the output format with the --format flag.
    --format=pretty    : output query results in a human-readable format
    --format=source    : output partial evaluation results in a source format
    --format=raw       : output the values from query results in a scripting friendly format
+    --format=discard   : output the result field as "discarded"


Nit: output the result field as "discarded" lets clarify that this is the case when the result is non-nil.

Does this clarify it?

@26tanishabanik sorry I meant lets update the text in the --format=discard description to something like --format=discard : output the result field as "discarded" when non-nil

Sure, will update that

ashutosh-narkar · 2023-07-12T16:17:10Z

@26tanishabanik I've added one small comment. Also can you please squash your commits and author a message per these guidelines and we can get this in. Thanks for working on this!

Fixes: open-policy-agent#5863 Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>

Signed-off-by: Tanisha Banik <26tanishabanik@gmail.com>

Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>

26tanishabanik · 2023-07-12T18:45:56Z

@26tanishabanik I've added one small comment. Also can you please squash your commits and author a message per these guidelines and we can get this in. Thanks for working on this!

Is the commit fine now, @ashutosh-narkar ?

ashutosh-narkar · 2023-07-12T21:18:34Z

@26tanishabanik I've added one small comment. Also can you please squash your commits and author a message per these guidelines and we can get this in. Thanks for working on this!

Is the commit fine now, @ashutosh-narkar ?

Hey @26tanishabanik looks like some additional files (go.mod, go.sum, vendor/modules.txt) were added when you squashed your commits. Can you please remove those and push a single commit. We can then merge this. When you have your local changes in one commit, run:

git fetch upstream
git rebase upstream/main

This will sync upstream changes to your local branch. You can then (force) push your commit.

26tanishabanik · 2023-07-13T06:22:11Z

@26tanishabanik I've added one small comment. Also can you please squash your commits and author a message per these guidelines and we can get this in. Thanks for working on this!

Is the commit fine now, @ashutosh-narkar ?

Hey @26tanishabanik looks like some additional files (go.mod, go.sum, vendor/modules.txt) were added when you squashed your commits. Can you please remove those and push a single commit. We can then merge this. When you have your local changes in one commit, run:
git fetch upstream
git rebase upstream/main
This will sync upstream changes to your local branch. You can then (force) push your commit.

@ashutosh-narkar , these files were already there, right?
Did you mean additional lines were added in those files (go.mod, go.sum, vendor/modules.txt) ?

charlieegan3 · 2023-07-13T08:03:22Z

Hey @26tanishabanik, Yeah I think so. It looks like there are some new lines that weren't there before in those files.

26tanishabanik · 2023-07-13T08:08:09Z

Hey @26tanishabanik, Yeah I think so. It looks like there are some new lines that weren't there before in those files.

Thanks, @charlieegan3 , will fix those

charlieegan3 · 2023-07-18T09:46:09Z

Hey @26tanishabanik, it'd be nice to get this into the next release. We're nearly there.

I think you can address the go.mod diffs with:

git checkout main -- go.mod
git checkout main -- go.sum
git checkout main -- vendor/modules.txt

Then git add . and commit 🙂

…open-policy-agent#5863 Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>

26tanishabanik · 2023-07-18T10:05:42Z

Hey @26tanishabanik, it'd be nice to get this into the next release. We're nearly there.

I think you can address the go.mod diffs with:
git checkout main -- go.mod
git checkout main -- go.sum
git checkout main -- vendor/modules.txt
Then git add . and commit 🙂

Sure, @charlieegan3, thanks 🙂

26tanishabanik · 2023-07-18T15:46:13Z

@ashutosh-narkar , @charlieegan3 , Are there any other changes required from my end?

charlieegan3 · 2023-07-18T15:57:00Z

Hey @26tanishabanik, it looks like there are still some changes in the go.mod file. How about we do this:

remote_name=$(git remote -v | grep 'github.com/open-policy-agent/opa' | head -n 1 | awk '{print $1}')
fork_name=$(git remote -v | grep 'github.com/26tanishabanik/opa' | head -n 1 | awk '{print $1}')
git fetch $remote_name
git rebase $remote_name/main
git checkout main -- go.mod
git checkout main -- go.sum
git checkout main -- vendor/modules.txt
git add .
git commit -m "Remove go.mod changes" --sign-off
git push -u $fork_name 'feature/issue-5863'

The three sets of changes which we need to remove are:

When we're done here, they should not appear in the PR diff.

26tanishabanik · 2023-07-18T16:26:51Z

Hey @26tanishabanik, it looks like there are still some changes in the go.mod file. How about we do this:
remote_name=$(git remote -v | grep 'github.com/open-policy-agent/opa' | head -n 1 | awk '{print $1}')
fork_name=$(git remote -v | grep 'github.com/26tanishabanik/opa' | head -n 1 | awk '{print $1}')
git fetch $remote_name
git rebase $remote_name/main
git checkout main -- go.mod
git checkout main -- go.sum
git checkout main -- vendor/modules.txt
git add .
git commit -m "Remove go.mod changes" --sign-off
git push -u $fork_name 'feature/issue-5863'
The three sets of changes which we need to remove are:

https://github.com/open-policy-agent/opa/pull/6080/files#diff-3295df7234525439d778f1b282d146a4f1ff6b415248aaac074e8042d9f42d63

https://github.com/open-policy-agent/opa/pull/6080/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6

https://github.com/open-policy-agent/opa/pull/6080/files#diff-b318e1e6bbcc59892bb6b4298564b051c6f6d5d848878d14aab856509ff0625c

When we're done here, they should not appear in the PR diff.

Can you check now?

charlieegan3 · 2023-07-18T16:43:44Z

🤔 hmm, I think that merging from main won't do the job here. I can still see the changes to go.mod, go.sum and modules.txt...

running

git checkout main -- go.mod
git checkout main -- go.sum
git checkout main -- vendor/modules.txt

And pushing a new commit without merging main, should address the issue.

(I'm heading out for the day, I'll check back tomorrow morning UK time.)

ashutosh-narkar · 2023-07-18T17:41:38Z

@26tanishabanik if it's easier you can close this PR. Then create new branch synced with upstream/main and then add your changes. For ex

Create a new branch:

git checkout -b my-new-feature
git fetch upstream
git rebase upstream/main

Now add your changes:

git add eval/eval.go
git add eval/eval_test.go
git add internal/presentation/presentation.go
git commit -s -m "Add --format discard to opa eval" -m "Fixes: #5863"
git push origin my-new-feature

Then create a PR. Hope this helps.

26tanishabanik · 2023-07-19T07:17:15Z

@ashutosh-narkar , @charlieegan3 , please check #6103

charlieegan3 · 2023-07-19T08:45:37Z

👏 Thanks @26tanishabanik for all the time you spent on this one. Great to see it's been merged now 🙂 great work.

26tanishabanik · 2023-07-19T08:50:35Z

👏 Thanks @26tanishabanik for all the time you spent on this one. Great to see it's been merged now 🙂 great work.

Thanks @charlieegan3 for your guidance 🙂

charlieegan3 · 2023-07-19T10:35:11Z

Very welcome!

charlieegan3 reviewed Jul 6, 2023

View reviewed changes

internal/presentation/presentation.go Outdated Show resolved Hide resolved

charlieegan3 changed the title ~~Fix for issue 5863~~ Add --format discard to opa eval Jul 6, 2023

charlieegan3 reviewed Jul 11, 2023

View reviewed changes

tester/runner.go Outdated Show resolved Hide resolved

charlieegan3 reviewed Jul 11, 2023

View reviewed changes

cmd/eval_test.go Outdated Show resolved Hide resolved

cmd/eval_test.go Outdated Show resolved Hide resolved

cmd/eval_test.go Outdated Show resolved Hide resolved

charlieegan3 reviewed Jul 11, 2023

View reviewed changes

cmd/eval_test.go Outdated Show resolved Hide resolved

cmd/eval_test.go Show resolved Hide resolved

charlieegan3 previously approved these changes Jul 11, 2023

View reviewed changes

ashutosh-narkar reviewed Jul 11, 2023

View reviewed changes

26tanishabanik dismissed charlieegan3’s stale review via bdd26a0 July 11, 2023 16:45

ashutosh-narkar reviewed Jul 12, 2023

View reviewed changes

cmd: add discard, a format subcommand for discarding result

f27dd20

Fixes: open-policy-agent#5863 Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>

26tanishabanik force-pushed the feature/issue-5863 branch from 13a073e to f27dd20 Compare July 12, 2023 18:21

26tanishabanik added 2 commits July 12, 2023 23:53

Merge branch 'main' into feature/issue-5863

97bbb19

Signed-off-by: Tanisha Banik <26tanishabanik@gmail.com>

cmd: adds a discard subcommand for format to discard the result

59b3172

Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>

cmd: adds a discard subcommand for format to discard the result Fixes: …

9253704

…open-policy-agent#5863 Signed-off-by: 26tanishabanik <26tanishabanik@gmail.com>

26tanishabanik force-pushed the feature/issue-5863 branch from fabfe31 to 9253704 Compare July 18, 2023 16:23

Merge branch 'main' into feature/issue-5863

615a877

26tanishabanik closed this Jul 19, 2023

		golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df h1:UA2aFVmmsIlefxMk29Dp2juaUSth8Pyn3Tq5Y5mJGME=
		golang.org/x/exp v0.0.0-20230626212559-97b1e661b5df/go.mod h1:FXUEEKJgO7OQYeo8N01OfiKP8RXMtf6e8aTskBGqWdc=

Add --format discard to opa eval #6080

Add --format discard to opa eval #6080

Conversation

26tanishabanik commented Jul 6, 2023

Why the changes in this PR are needed?

What are the changes in this PR?

Notes to assist PR review:

Further comments:

charlieegan3 left a comment

Choose a reason for hiding this comment

charlieegan3 left a comment

Choose a reason for hiding this comment

26tanishabanik commented Jul 11, 2023

26tanishabanik commented Jul 11, 2023

charlieegan3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

26tanishabanik Jul 11, 2023 • edited

Choose a reason for hiding this comment

26tanishabanik commented Jul 11, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

26tanishabanik Jul 11, 2023 • edited

Choose a reason for hiding this comment

charlieegan3 Jul 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

26tanishabanik commented Jul 11, 2023

ashutosh-narkar commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Jul 12, 2023

26tanishabanik commented Jul 12, 2023

ashutosh-narkar commented Jul 12, 2023

26tanishabanik commented Jul 13, 2023

charlieegan3 commented Jul 13, 2023

26tanishabanik commented Jul 13, 2023

charlieegan3 commented Jul 18, 2023

26tanishabanik commented Jul 18, 2023 • edited

26tanishabanik commented Jul 18, 2023

charlieegan3 commented Jul 18, 2023

26tanishabanik commented Jul 18, 2023

charlieegan3 commented Jul 18, 2023

ashutosh-narkar commented Jul 18, 2023

26tanishabanik commented Jul 19, 2023

charlieegan3 commented Jul 19, 2023

26tanishabanik commented Jul 19, 2023

charlieegan3 commented Jul 19, 2023

26tanishabanik Jul 11, 2023 •

edited

26tanishabanik Jul 11, 2023 •

edited

charlieegan3 Jul 12, 2023 •

edited

26tanishabanik commented Jul 18, 2023 •

edited