feat: add policy develop lint by Piskoo · Pull Request #2250 · chainloop-dev/chainloop

Piskoo · 2025-07-14T11:41:17Z

This PR adds policy linting and auto-formatting capabilities

Usage

# Single file that will get auto formatted
$ chainloop policy devel lint --policy mypolicy.yaml --format
  
# All files in the directory no formatting
$ chainloop policy devel lint

Flags:

--policy: File or directory, by default current working directory
--format: Auto-formatting for rego files with OPA fmt

Summary

Validates both standalone Rego files and YAML policies with embedded Rego
Checks for required policy structure and fields
- apiVersion, kind, spec for .yaml files
- skipped, violations, skip_reason for result in .rego files
Verifies proper Rego syntax using Regal linter
Optional auto-formatting of Rego code with --format flag
Preserves YAML structure and comments when formatting embedded policies

Part of #1108

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

migmartri

I think we should re-evaluate the marshaling and validation of the policy spec by using the actual spec file, not custom ducktyping.

basically import the policy spec generated code, we might do it in other places already, then do protojson.Unmarshal or similar

additionally, once you have the marshalled object, you might be able to run protovalidate if the proto file has additional validation annotations.

Does it make sense?

migmartri · 2025-07-15T12:09:33Z

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package policy


this package name should probably match the directory name that way you do not need to policy "github.com/chainloop-dev/chainloop/app/cli/internal/policydevel" :)

migmartri · 2025-07-15T12:10:52Z

+			return nil, fmt.Errorf("reading file: %w", err)
+		}
+
+		switch strings.ToLower(filepath.Ext(absPath)) {


this code seems repeated ot the one above. Maybe this detection can be extracted? It looks like a good opportunity to maybe to a recursive function when you find potential candidate files in a directory :)

migmartri · 2025-07-15T12:11:43Z

+	}
+
+	// Process each embedded policy
+	if spec, ok := data["spec"].(map[string]interface{}); ok {


I think you should unmarshal against the actual proto spec using protojson.Unmarshal and protovalidaet.Validate.

… replicated code Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

javirln · 2025-07-16T16:37:22Z

Thanks for you PR @Piskoo one question, how difficult would it be to be able to pass a custom configuration file to the regal module?

Internally we have a set of custom configuration for regal and we were wondering how difficult would it be to add such option.

migmartri · 2025-07-16T17:17:50Z

Thanks for you PR @Piskoo one question, how difficult would it be to be able to pass a custom configuration file to the regal module?

Internally we have a set of custom configuration for regal and we were wondering how difficult would it be to add such option.

I'd love it if we could come up with sensible defaults for that configuration, which one are we using internally?

migmartri · 2025-07-16T17:19:53Z

Thanks for you PR @Piskoo one question, how difficult would it be to be able to pass a custom configuration file to the regal module?
Internally we have a set of custom configuration for regal and we were wondering how difficult would it be to add such option.

I'd love it if we could come up with sensible defaults for that configuration, which one are we using internally?

I think we are using this one correct? Do we like it? Should we make that a sensible default for our linter? cc/ @javirln @jiparis

# based from https://github.com/spacelift-io/spacelift-policies-example-library/blob/main/.regal/config.yaml
rules:
  idiomatic:
    directory-package-mismatch:
      level: ignore
    no-defined-entrypoint:
      level: warning
  style:
    todo-comment:
      level: ignore
    line-length:
      level: ignore
    rule-length:
      level: ignore
      ignore:
        files:
          - "*_test.rego"

in any case @Piskoo, adding support for configuration override or adding one built-in could come in another PR

migmartri · 2025-07-16T17:21:14Z

+}
+
+// Read policy files from the given directory or file
+func Read(absPath string, format bool) (*Policy, error) {


Suggested change

func Read(absPath string, format bool) (*Policy, error) {

func Lookup(absPath string, format bool) (*Policy, error) {

migmartri · 2025-07-16T17:21:58Z

+	"github.com/styrainc/regal/pkg/rules"
+)
+
+type Policy struct {


Suggested change

type Policy struct {

type PolicyToLint struct {

migmartri · 2025-07-16T17:22:17Z

+
+// Read policy files from the given directory or file
+func Read(absPath string, format bool) (*Policy, error) {
+	fileInfo, err := os.Stat(absPath)


capture other errors too

migmartri · 2025-07-16T17:23:36Z

+	return policy, nil
+}
+
+func readDirectory(policy *Policy, dirPath string) error {


please add a comment indicating that this is a one level directory lookup

migmartri · 2025-07-16T17:27:53Z

+	return allErrors
+}
+
+func (p *Policy) validateYAMLFile(file *File) []error {


I do not think this is idiomatic. []error just return an error and use https://pkg.go.dev/errors#example-Join

I've changed the way that violations are stored/passed between functions

javirln · 2025-07-17T07:12:21Z

+	}
+
+	// 2. Structural validation
+	errs = append(errs, checkResultStructure(content, path, []string{"skipped", "violations", "skip_reason"})...)


@jiparis should we try to add here ignore as well? Or no? The engine by default fallback to false

javirln · 2025-07-17T07:20:23Z

Thanks for you PR @Piskoo one question, how difficult would it be to be able to pass a custom configuration file to the regal module?
Internally we have a set of custom configuration for regal and we were wondering how difficult would it be to add such option.

I'd love it if we could come up with sensible defaults for that configuration, which one are we using internally?

I think we are using this one correct? Do we like it? Should we make that a sensible default for our linter? cc/ @javirln @jiparis
# based from https://github.com/spacelift-io/spacelift-policies-example-library/blob/main/.regal/config.yaml
rules:
  idiomatic:
    directory-package-mismatch:
      level: ignore
    no-defined-entrypoint:
      level: warning
  style:
    todo-comment:
      level: ignore
    line-length:
      level: ignore
    rule-length:
      level: ignore
      ignore:
        files:
          - "*_test.rego"
in any case @Piskoo, adding support for configuration override or adding one built-in could come in another PR

From the list of rules we are ignoring on the custom configuration, I would say that we can remove the _test.rego since that's something specific to tests and shouldn't affect the actual policy.

The most important is no-defined-entrypoint. Our engine expects a custom result structure and we don't care about the entrypoint since the policies are meant to be run in our engine. Here's some documentation: https://docs.styra.com/regal/rules/idiomatic/no-defined-entrypoint

The rest of the options is up to us, not a strong opinion there.

migmartri · 2025-07-17T17:21:34Z

@Piskoo let me know what you think about adding the following defaults

# based from https://github.com/spacelift-io/spacelift-policies-example-library/blob/main/.regal/config.yaml
rules:
  idiomatic:
    directory-package-mismatch:
      level: ignore
    no-defined-entrypoint:
      level: warning
  style:
    todo-comment:
      level: ignore
    line-length:
      level: ignore
    rule-length:
      level: ignore
      ignore:
        files:
          - "*_test.rego"

once done we can assume this PR is done :)

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

Piskoo · 2025-07-18T08:16:47Z

@Piskoo let me know what you think about adding the following defaults

# based from https://github.com/spacelift-io/spacelift-policies-example-library/blob/main/.regal/config.yaml
rules:
  idiomatic:
    directory-package-mismatch:
      level: ignore
    no-defined-entrypoint:
      level: warning
  style:
    todo-comment:
      level: ignore
    line-length:
      level: ignore
    rule-length:
      level: ignore
      ignore:
        files:
          - "*_test.rego"

once done we can assume this PR is done :)

Looks good to me, although like @javirln mentioned _test.rego is not needed, so we can remove that from the default config.

migmartri · 2025-07-18T08:59:57Z

Let me know if this is ready for review, otherwise, please mark it as draft, thanks!

Piskoo · 2025-07-18T09:06:03Z

Let me know if this is ready for review, otherwise, please mark it as draft, thanks!

It's ready for review, I'll add configs in another PR

migmartri

Thanks

Piskoo added 2 commits July 14, 2025 13:23

feat: add policy develop lint

1c5e1ae

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

fix linting error

33151f4

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

Piskoo marked this pull request as ready for review July 14, 2025 11:53

migmartri approved these changes Jul 15, 2025

View reviewed changes

migmartri requested changes Jul 15, 2025

View reviewed changes

Piskoo added 2 commits July 16, 2025 14:03

use proto unmarshal and validation for yaml schema validation; remove…

d5cbbea

… replicated code Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

fix tests

04c3c6d

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

migmartri reviewed Jul 16, 2025

View reviewed changes

javirln reviewed Jul 17, 2025

View reviewed changes

Piskoo added 3 commits July 18, 2025 09:12

rename policy; add comments, change return type for linting functions

5a6fc8d

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

refactor handling storage during linting process

b905bce

Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>

resolve merge conflicts

2fc1359

migmartri approved these changes Jul 18, 2025

View reviewed changes

Piskoo merged commit f252dd8 into chainloop-dev:main Jul 18, 2025
13 checks passed

	func Read(absPath string, format bool) (*Policy, error) {
	func Lookup(absPath string, format bool) (*Policy, error) {

Uh oh!

Conversation

Piskoo commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage

Summary

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javirln commented Jul 16, 2025

Uh oh!

migmartri commented Jul 16, 2025

Uh oh!

migmartri commented Jul 16, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javirln commented Jul 17, 2025

Uh oh!

migmartri commented Jul 17, 2025

Uh oh!

Piskoo commented Jul 18, 2025

Uh oh!

migmartri commented Jul 18, 2025

Uh oh!

Piskoo commented Jul 18, 2025

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Piskoo commented Jul 14, 2025 •

edited

Loading