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

Potential issue with opa fmt when interacted via stdin #2235

Closed
psibi opened this issue Mar 28, 2020 · 6 comments · Fixed by #2249
Closed

Potential issue with opa fmt when interacted via stdin #2235

psibi opened this issue Mar 28, 2020 · 6 comments · Fixed by #2249
Labels

Comments

@psibi
Copy link
Contributor

psibi commented Mar 28, 2020

Expected Behavior

Should properly format and write out the code

Actual Behavior

Produces no output. (This has a side effect of deleting the entire code when this is integrated with editors!)

Steps to Reproduce the Problem

This is a sample step to reproduce:

$ opa fmt
package hello

hello = "world"
Ctrl-D
$

Note that if Ctrl-D is pressed without entering a newline, it works as expected:

$ opa fmt
package hello

hello = "world"Ctrl-D
package hello

hello = "world"
$

Additional Info

This is much frequently reproduced when automatic formatting is enabled on the emacs major mode for rego: https://github.com/psibi/rego-mode

@patrick-east
Copy link
Contributor

patrick-east commented Mar 29, 2020

I think this is normal, you either have to have a newline+ctrl-d or use two ctrl-d's to indicate that the input is done. If you do just a single ctrl-d in bash (not sure if its just bash, or linux, etc) just sends the line without an EOF. So you need either the newline or the second ctrl-d to get the EOF through.

You get the same behavior with other unix-y tools (try it out with wc or something)

@psibi
Copy link
Contributor Author

psibi commented Mar 29, 2020

@patrick-east

you either have to have a newline+ctrl-d

But I'm having a "newline + Ctrl-D" in the first example and it reproduces the bug.

@patrick-east
Copy link
Contributor

Sorry! I totally misread the issue from the first example. That is not normal 😄

@patrick-east
Copy link
Contributor

Looks like this might be intentional behavior if there are no changes after formatting:

opa/cmd/fmt.go

Lines 161 to 164 in 29d8fbb

if !bytes.Equal(formatted, contents) {
_, err := w.Write(formatted)
return err
}

Will need to look into why it does that, and if we are OK with changing that behavior.

In the short term with the plugin if you can just check if it has nothing in stdout from opa fmt it should be assumed that there were no changes required to the original content.

@patrick-east
Copy link
Contributor

Will need to look into why it does that, and if we are OK with changing that behavior.

Checked with the team and we're ok to make the behavior change to always write the formatted policy to stdout.

patrick-east added a commit to patrick-east/opa that referenced this issue Mar 31, 2020
There was an explicit check that would prevent writing to stdout if
the formatting didn't change anything.

This makes sense for the "write to file" or "diff" options as we can
save work by bailing out early, but if configured to write the file
contents to stdout we should do so in all cases.

Fixes: open-policy-agent#2235
Signed-off-by: Patrick East <[email protected]>
@psibi
Copy link
Contributor Author

psibi commented Mar 31, 2020

Thanks @patrick-east for the quick update. :-) I'm happy that this is getting fixed!

tsandall pushed a commit that referenced this issue Apr 2, 2020
There was an explicit check that would prevent writing to stdout if
the formatting didn't change anything.

This makes sense for the "write to file" or "diff" options as we can
save work by bailing out early, but if configured to write the file
contents to stdout we should do so in all cases.

Fixes: #2235
Signed-off-by: Patrick East <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants