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

Prepare for OPA 1.0 #1317

Merged
merged 7 commits into from
Jan 13, 2025
Merged

Prepare for OPA 1.0 #1317

merged 7 commits into from
Jan 13, 2025

Conversation

anderseknert
Copy link
Member

Go:

  • Use OPA v1 packages
  • Add function to parse Rego of unknown version
  • Allow providing the Rego version via config or .manifest files
  • Make regal fix formatter consider version to format for

Rego:

  • Remove import rego.v1 (via opa fmt)
  • Add config.capabilities.is_opa_v1
  • Use that to disable use-if, use-contains, use-rego-v1
  • Update tests for 1.0 compliance

Docs:

  • TODO remove import rego.v1 from all examples
  • regal fix doesn't work well
  • Many failing e2e tests, despite some effort
  • Test LSP functionality

.vscode/launch.json Outdated Show resolved Hide resolved
Copy link
Member

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

Gosh, big one! great work. Few small comments. Happy to pick up the fix and LS work on this one.

rego-version: 1
```

Additionally, Regal will scan the project for any `.manifest` files, and user any `rego_version` found in the manifest
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Additionally, Regal will scan the project for any `.manifest` files, and user any `rego_version` found in the manifest
Additionally, Regal will scan the project for any `.manifest` files, and use any `rego_version` found in the manifest

Copy link
Member

Choose a reason for hiding this comment

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

Should this part also mention the _0.rego suffix?

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

mod, err = ast.ParseModuleWithOpts(filename, policy, opts)
if err == nil {
if hasRegoV1Import(mod.Imports) {
Copy link
Member

Choose a reason for hiding this comment

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

what about future keywords? do we also need to handle these here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's still Rego v0 AFAICS.. i.e. you can import contains without using it, while you can't import rego.v1 without using contains, etc.

var versionsMap map[string]ast.RegoVersion

// TODO: How should we deal with this in the language server?
// AllRegoVersions will call WalkDir on the root to find manifests, but that's obviously not
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess we can do something at start up and when new files are found. We do this for ignored files, finding new ones, caching their builtins etc so maybe something like that makes the most sense here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose this is address by now, right?

pkg/linter/linter.go Outdated Show resolved Hide resolved
anderseknert and others added 7 commits January 13, 2025 12:40
Go:
- Use OPA v1 packages
- Add function to parse Rego of unknown version
- Allow providing the Rego version via config or .manifest files
- Make `regal fix` formatter consider version to format for

Rego:
- Remove `import rego.v1` (via `opa fmt`)
- Add `config.capabilities.is_opa_v1`
- Use that to disable `use-if`, `use-contains`, `use-rego-v1`
- Update tests for 1.0 compliance

Docs:
- TODO remove `import rego.v1` from all examples
- `regal fix` doesn't work well
- Many failing e2e tests, despite some effort
- Test LSP functionality

Signed-off-by: Anders Eknert <anders@styra.com>
* fixer: Prep for 1.0 tests

Signed-off-by: Charlie Egan <charlie@styra.com>

* fixer/fmt: Add tests for other rego versions

Signed-off-by: Charlie Egan <charlie@styra.com>

* fixer: Update fixer e2e test

Signed-off-by: Charlie Egan <charlie@styra.com>

* Correct formatting of launch.json

* fixer: Return formatting to Fixer test

Signed-off-by: Charlie Egan <charlie@styra.com>

---------

Signed-off-by: Charlie Egan <charlie@styra.com>
Mainly, this is ensuring the configs for each test are correct.

Signed-off-by: Charlie Egan <charlie@styra.com>
Signed-off-by: Charlie Egan <charlie@styra.com>
* lsp/server: Cache AllRegoVersions at config load

This does not reload when a .manifest file is changed yet.

Signed-off-by: Charlie Egan <charlie@styra.com>

* concurrent/map: Nil map is empty

Signed-off-by: Charlie Egan <charlie@styra.com>

---------

Signed-off-by: Charlie Egan <charlie@styra.com>
Also, use new version cache to determine version for formatting

Signed-off-by: Charlie Egan <charlie@styra.com>
- Remove `import rego.v1` from examples
- Add new page covering Regal and OPA 1.0
- Update logic of any rules obsolete in 1.0 to make sure
  they're automatically disabled when 1.0 is targeted, and
  continue to work when older versions are linted
- Update a few tests to remove `import rego.v1`
- Many minor fixes related to this theme

Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert anderseknert marked this pull request as ready for review January 13, 2025 11:47
@anderseknert anderseknert merged commit d569e50 into main Jan 13, 2025
5 checks passed
@anderseknert anderseknert deleted the opa1.0 branch January 13, 2025 11:49
@anderseknert anderseknert mentioned this pull request Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants