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

opa fmt --diff fails unless external diff tool is present #6284

Closed
anderseknert opened this issue Oct 9, 2023 · 7 comments · Fixed by #6452
Closed

opa fmt --diff fails unless external diff tool is present #6284

anderseknert opened this issue Oct 9, 2023 · 7 comments · Fixed by #6452

Comments

@anderseknert
Copy link
Member

Conducting some tests with OPA on a GCP compute instance running Windows Server 2022, and I was surprised to see the opa fmt --diff command fail:

Screenshot 2023-10-09 at 07 40 46

There must be plenty of tools available to display diffs in the Go ecosystem, and perhaps some we depend on already. Perhaps we could use one of those? OTOH this code is from 2017, and if no one else complained in the time since then, I guess that flag is uncommon enough that not many have had to see this.

@srenatus
Copy link
Contributor

srenatus commented Oct 9, 2023

There's something weird going on here. The "is there a diff?" logic uses bytes.Equal. Oh wait, we do use diff for formatting later on: https://github.com/open-policy-agent/opa/blob/main/cmd/fmt.go#L209.

Yup we should be able to get rid of this by using some golang code or a new (small) dependency.

@srenatus
Copy link
Contributor

srenatus commented Oct 9, 2023

https://play.golang.com/p/XhCf9WJaI_a -- using the first hit on google, https://github.com/sergi/go-diff.

Copy link

stale bot commented Nov 8, 2023

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 stale bot added the inactive label Nov 8, 2023
@colinjlacy
Copy link
Contributor

I can take this one

@srenatus
Copy link
Contributor

Since my last comment, I also came across this -- https://cs.opensource.google/go/go/+/master:src/internal/diff/diff.go?q=diff&ss=go%2Fgo -- we'd need to copy-vendor this, but the license lets us do that, I think.

@colinjlacy
Copy link
Contributor

@srenatus it turns out sergi/go-diff is already in the repo, so I can use that if you're still good with taking that approach.

@anderseknert
Copy link
Member Author

If we can use an existing dependency, all the better!

colinjlacy added a commit to colinjlacy/opa that referenced this issue Dec 2, 2023
Replaces call to local diff command as a hidden dependency
with call to go-diff, which is already included in the repo.

Fixes open-policy-agent#6284

Signed-off-by: Colin Lacy <colinjlacy@gmail.com>
anderseknert pushed a commit that referenced this issue Dec 4, 2023
Replace call to local diff command as a hidden dependency
with call to go-diff, which is already included in the repo.

Fixes #6284

Signed-off-by: Colin Lacy <colinjlacy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants