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.

Sign up for GitHub

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

Jump to bottom

Add ability for opa inspect to inspect a single file outside of any bundle #6873

Merged
merged 1 commit into from
Jul 21, 2024
Merged

Add ability for opa inspect to inspect a single file outside of any bundle #6873

merged 1 commit into from
Jul 21, 2024

Conversation

tjons
Copy link
Contributor

@tjons tjons commented Jul 17, 2024 •
edited
Loading

Why the changes in this PR are needed?

Resolves #6799 by adding the ability for the opa inspect command to handle single files in addition to the currently supported bundle directory and tarball options. This PR is necessary to improve the UX of inspecting annotations in a single file.

What are the changes in this PR?

Adds a new method and decision tree to the opa inspect command that will process a single file if the argument passed to the command ends with .rego. Will follow other opa inspect processing semantics and behavior - changes are constrained to loading the single file and processing it as its own entity outside of the scope of a bundle.

Notes to assist PR review:

I'm not terribly familiar with OPA's WASM capabilities yet - @anderseknert is there anything in this issue you were thinking would extend to WASM as well that I could add?

I also updated vendored deps as part of this PR - let me know if I should back those changes out.

Further comments:

@tjons tjons mentioned this pull request Jul 17, 2024
Closed
@tjons tjons force-pushed the tjons/6799-opa-inspect-single-file branch 2 times, most recently from f333058 to d653074 Compare July 17, 2024 14:40
anderseknert
anderseknert reviewed Jul 17, 2024
View reviewed changes
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.

Change looks good to me. I'm not sure I fully understood the changes to deps though.

Thanks for working on this, Tyler!

internal/bundle/inspect/inspect.go Outdated
}

if includeAnnotations {
as, errs := ast.BuildAnnotationSet(maps.Values(moduleMap))
Copy link
Member

Choose a reason for hiding this comment

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

Did we add an extra dependency to OPA just for maps.Values? I think we could just add a util function for that if we don't have one already.

@ashutosh-narkar
Copy link
Member

I also updated vendored deps as part of this PR - let me know if I should back those changes out.

@tjons thanks for the contribution. It would be great if we could limit the PR to resolve #6799 and avoid the vendor changes.

@tjons
Copy link
Contributor Author

tjons commented Jul 19, 2024

@ashutosh-narkar will do - have been busy but will circle back to this over the weekend, I also need to fix some of the tests as the output table width in CI is not matching my machine.

Are you proposing that we add the --data flag to just allow inspection of single data files, similar to what this PR does for .rego files? If so, what if we just added that as a automagic feature that is triggered on .json extension, similar to how this PR triggers single-file inspection on the .rego extension? Are there other data sources supported other than JSON files?

@ashutosh-narkar
Copy link
Member

@ashutosh-narkar will do - have been busy but will circle back to this over the weekend, I also need to fix some of the tests as the output table width in CI is not matching my machine.

Are you proposing that we add the --data flag to just allow inspection of single data files, similar to what this PR does for .rego files? If so, what if we just added that as a automagic feature that is triggered on .json extension, similar to how this PR triggers single-file inspection on the .rego extension? Are there other data sources supported other than JSON files?

I was thinking in line with the opa eval command which also has a --data flag for supplying policy and data files. You could avoid the file extension check if you went with the flag. If this happens by default that's fine I guess.

@anderseknert
Copy link
Member

Let’s not change the scope as part of this PR. We can always create new issues later if we want the command to do more.

@ashutosh-narkar
Copy link
Member

Let’s not change the scope as part of this PR. We can always create new issues later if we want the command to do more.

Sure but I don't think it would be that big of a change to include data files. Either option is fine with me btw.

@tjons tjons force-pushed the tjons/6799-opa-inspect-single-file branch 4 times, most recently from 06fcdca to a4450b1 Compare July 20, 2024 17:26
@tjons
Copy link
Contributor Author

tjons commented Jul 20, 2024 •
edited
Loading

@ashutosh-narkar this should be good now! I'm going to open a follow-on issue (#6879) for the --data file flag and will tackle that one once this merges, I think it's a great idea.

@tjons tjons mentioned this pull request Jul 20, 2024
Open
anderseknert
anderseknert reviewed Jul 21, 2024
View reviewed changes
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.

Thanks @tjons!

Code LGTM, but I'm afraid I still don't understand the dependency changes in this PR 😅 Some dependencies got bumped because it was needed for something changed here, or just as a side quest of sorts? If the latter, perhaps you could do that in a separate PR, so the changes here are isolated to resolving the issue?

Thanks! And sorry if I'm being obtuse about it 😄

@tjons tjons force-pushed the tjons/6799-opa-inspect-single-file branch from ebda3f8 to f34c89c Compare July 21, 2024 14:04
@tjons
add ability for opa inspect to inspect a single file outside of any b…
eeb09fb 
…undle

Signed-off-by: Tyler Schade <tyler.schade@solo.io>

add ability for opa inspect to inspect a single file outside of any bundle

Signed-off-by: Tyler Schade <tyler.schade@solo.io>
@tjons tjons force-pushed the tjons/6799-opa-inspect-single-file branch from f34c89c to eeb09fb Compare July 21, 2024 14:06
@tjons
Copy link
Contributor Author

tjons commented Jul 21, 2024

@anderseknert I had backed those changes out but unfortunately it appears I made a mistake and either wasn't thorough enough or more likely messed up the rebase from main. They're gone now!

anderseknert
anderseknert approved these changes Jul 21, 2024
View reviewed changes
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.

Thanks a lot!

@anderseknert anderseknert merged commit 5226cf3 into open-policy-agent:main Jul 21, 2024
28 checks passed
@tjons tjons deleted the tjons/6799-opa-inspect-single-file branch July 22, 2024 14:15
@BrewTestBot BrewTestBot mentioned this pull request Jul 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anderseknert anderseknert anderseknert approved these changes

@ashutosh-narkar ashutosh-narkar Awaiting requested review from ashutosh-narkar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow opa inspect to inspect a single file
3 participants
@tjons @ashutosh-narkar @anderseknert