Skip to content

Conversation

Piskoo
Copy link
Collaborator

@Piskoo Piskoo commented Aug 8, 2025

This PR solves problem with comments being removed when --format is used.
Example policies in docs were re-linted with the tool.

Closes #2320

Piskoo added 4 commits August 8, 2025 12:30
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo Piskoo marked this pull request as ready for review August 8, 2025 10:57
@Piskoo Piskoo requested review from javirln, jiparis and migmartri August 8, 2025 10:58
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Awesome, do you mind explaining a little bit more about how you solved the issue?

Do you think it's ok to remove protoyaml? I remember something like it had built-in validation (the annotations in the yaml files) of the proto object, I guess we've lost that?

"skipped": skipped,
"violations": violations,
"skip_reason": skip_reason,
"skipped": skipped,
Copy link
Member

Choose a reason for hiding this comment

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

why do we have more spaces now?

Do ew want to have 4 instead of 2? I personally like 2 spaces, any reason for using 4?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems like a bug, must have missed this, thanks!

@Piskoo
Copy link
Collaborator Author

Piskoo commented Aug 11, 2025

Awesome, do you mind explaining a little bit more about how you solved the issue?

Do you think it's ok to remove protoyaml? I remember something like it had built-in validation (the annotations in the yaml files) of the proto object, I guess we've lost that?

Protoyaml is still used so the validation is present. The issue was solved with another library that allows unmarshaling yaml into yaml nodes and then traversing those nodes to update only the embedded policies. This adds some overhead because yaml gets unmarshaled twice when --format is used, but due to limitations present in both libraries that's the only way I found to preserve the comments.

Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Thanks, let's make sure we keep two spaces

@Piskoo Piskoo merged commit 6f6a4cb into chainloop-dev:main Aug 11, 2025
13 checks passed
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.

policy devel lint --format removes the comments

2 participants