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

Renaming --rego-v1 cmd flag to --v0-v1 #7225

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions cmd/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,8 @@ func init() {
addCapabilitiesFlag(checkCommand.Flags(), checkParams.capabilities)
addSchemaFlags(checkCommand.Flags(), checkParams.schema)
addStrictFlag(checkCommand.Flags(), &checkParams.strict, false)
// FIXME: Rename or add new flag with same effect? '--rego-v1' will become even more confusing in 1.0, as what it actually means is check that module is compatible with BOTH v0 and v1.
addRegoV1FlagWithDescription(checkCommand.Flags(), &checkParams.regoV1, false,
"check for Rego v1 compatibility (policies must also be compatible with current OPA version)")
addRegoV0V1FlagWithDescription(checkCommand.Flags(), &checkParams.regoV1, false,
"check for Rego v0 and v1 compatibility (policies must be compatible with both Rego versions)")
addV0CompatibleFlag(checkCommand.Flags(), &checkParams.v0Compatible, false)
addV1CompatibleFlag(checkCommand.Flags(), &checkParams.v1Compatible, false)
RootCommand.AddCommand(checkCommand)
Expand Down
6 changes: 5 additions & 1 deletion cmd/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,12 @@ func addStrictFlag(fs *pflag.FlagSet, strict *bool, value bool) {
fs.BoolVarP(strict, "strict", "S", value, "enable compiler strict mode")
}

func addRegoV1FlagWithDescription(fs *pflag.FlagSet, regoV1 *bool, value bool, description string) {
func addRegoV0V1FlagWithDescription(fs *pflag.FlagSet, regoV1 *bool, value bool, description string) {
fs.BoolVar(regoV1, "v0-v1", value, description)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if --v0-v1 is the best name, but I can't come up with anything better.

Could call it --v0-v1-compatible to be more similar to --v0-compatible; but that command covers a slightly different use-case, so could be more confusing than helpful.

Suggestions are welcome :).


// For backwards compatibility
fs.BoolVar(regoV1, "rego-v1", value, description)
_ = fs.MarkHidden("rego-v1")
}

func addV0CompatibleFlag(fs *pflag.FlagSet, v1Compatible *bool, value bool) {
Expand Down
34 changes: 31 additions & 3 deletions cmd/fmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ is provided - this tool will use stdin.
The format of the output is not defined specifically; whatever this tool outputs
is considered correct format (with the exception of bugs).

If the '-w' option is supplied, the 'fmt' command with overwrite the source file
If the '-w' option is supplied, the 'fmt' command will overwrite the source file
instead of printing to stdout.

If the '-d' option is supplied, the 'fmt' command will output a diff between the
Expand All @@ -70,7 +70,26 @@ that would change if formatted. The '-l' option will suppress any other output
to stdout from the 'fmt' command.

If the '--fail' option is supplied, the 'fmt' command will return a non zero exit
code if a file would be reformatted.`,
code if a file would be reformatted.

The 'fmt' command can be run in several compatibility modes for consuming and outputting
different Rego versions:

* 'opa fmt':
* v1 Rego is formatted to v1
* 'rego.v1'/'future.keywords' imports are NOT removed
* 'rego.v1'/'future.keywords' imports are NOT added if missing
* v0 rego is rejected
* 'opa fmt --v0-compatible':
* v0 Rego is formatted to v0
* v1 Rego is rejected
* 'opa fmt --v0-v1':
* v0 Rego is formatted to be compatible with v0 AND v1
* v1 Rego is rejected
* 'opa fmt --v0-v1 --v1-compatible':
* v1 Rego is formatted to be compatible with v0 AND v1
* v0 Rego is rejected
`,
PreRunE: func(cmd *cobra.Command, _ []string) error {
return env.CmdFlags.CheckEnvironmentVariables(cmd)
},
Expand Down Expand Up @@ -138,6 +157,10 @@ func formatFile(params *fmtCommandParams, out io.Writer, filename string, info o
DropV0Imports: params.dropV0Imports,
}

if params.regoV1 {
opts.ParserOptions = &ast.ParserOptions{RegoVersion: ast.RegoV0}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we expect incoming policies to be v0, we'll error if the relevant imports aren't present; which isn't ideal.

If we drop this, then we'll default to v1, and the user can override to with --v0-compatible (now they can actually override to v1 with the hidden --v1-compatible flag).

Alternatively, we could try parsing with one version, and if it fails, parse with the other; but that's hard to make fool-proof.

Copy link
Contributor Author

@johanfylling johanfylling Dec 18, 2024

Choose a reason for hiding this comment

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

To clarify the current behaviour:

  • opa fmt:
    • v1 rego is formatted to v1
    • rego.v1/future.keywords imports are NOT removed
    • rego.v1/future.keywords imports are NOT added if missing
    • v0 rego is rejected
  • opa fmt --v0-compatible:
    • v0 rego is formatted to v0
    • v1 rego is rejected
  • opa fmt --v0-v1:
    • v0 rego is formatted to be compatible with v0 AND v1
    • v1 rego is rejected
  • opa fmt --v0-v1 --v1-compatible:
    • v1 rego is formatted to be compatible with v0 AND v1
    • v0 rego is rejected
    • --v1-compatible flag is hidden in opa fmt --help

A mix of v0/v1 modules is NOT supported.

In the above, a v0 module can have rego.v1 and future.keywords imports and be v1 compliant.

}

if params.v0Compatible {
// v0 takes precedence over v1
opts.ParserOptions = &ast.ParserOptions{RegoVersion: ast.RegoV0}
Expand Down Expand Up @@ -216,6 +239,11 @@ func formatStdin(params *fmtCommandParams, r io.Reader, w io.Writer) error {

opts := format.Opts{}
opts.RegoVersion = params.regoVersion()

if params.regoV1 {
opts.ParserOptions = &ast.ParserOptions{RegoVersion: ast.RegoV0}
}

formatted, err := format.SourceWithOpts("stdin", contents, opts)
if err != nil {
return err
Expand Down Expand Up @@ -252,7 +280,7 @@ func init() {
formatCommand.Flags().BoolVarP(&fmtParams.list, "list", "l", false, "list all files who would change when formatted")
formatCommand.Flags().BoolVarP(&fmtParams.diff, "diff", "d", false, "only display a diff of the changes")
formatCommand.Flags().BoolVar(&fmtParams.fail, "fail", false, "non zero exit code on reformat")
addRegoV1FlagWithDescription(formatCommand.Flags(), &fmtParams.regoV1, false, "format module(s) to be compatible with both Rego v1 and current OPA version)")
addRegoV0V1FlagWithDescription(formatCommand.Flags(), &fmtParams.regoV1, false, "format module(s) to be compatible with both Rego v0 and v1")
addV0CompatibleFlag(formatCommand.Flags(), &fmtParams.v0Compatible, false)
addV1CompatibleFlag(formatCommand.Flags(), &fmtParams.v1Compatible, false)
formatCommand.Flags().BoolVar(&fmtParams.checkResult, "check-result", true, "assert that the formatted code is valid and can be successfully parsed (default true)")
Expand Down
29 changes: 23 additions & 6 deletions cmd/fmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -746,10 +746,11 @@ func TestFmtMultipleWrongArityError(t *testing.T) {

func TestFmtRegoV1(t *testing.T) {
tests := []struct {
note string
input string
expected string
expectedErr string
note string
v1Compatible bool
input string
expected string
expectedErr string
}{
{
note: "no future imports",
Expand Down Expand Up @@ -835,14 +836,30 @@ q := all([true, false])
%ROOT%/policy.rego:3: rego_type_error: deprecated built-in function calls in expression: any
%ROOT%/policy.rego:6: rego_type_error: deprecated built-in function calls in expression: all`,
},
{
note: "v1 module",
v1Compatible: true,
input: `package test

p contains x if {
some x in ["a", "b", "c"]
}`,
expected: `package test

import rego.v1

p contains x if {
some x in ["a", "b", "c"]
}
`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case illustrates a scenario that can be solved with setting the --v1-compatible flag. This flag is however hidden, so not ideal.

converting v0->v1 is likely the more common use case, but we can't guarantee this won't happen; and will probably become more likely as time pass ..

},
}

for _, tc := range tests {
t.Run(tc.note, func(t *testing.T) {
params := fmtCommandParams{
// Locking rego-version to v0, as it's only then the --rego-v1 flag is relevant
v0Compatible: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why the tests didn't fail on main when we moved to v1 as default.

regoV1: true,
v1Compatible: tc.v1Compatible,
}

files := map[string]string{
Expand Down