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

fix(KONFLUX-3663): format PipelineRun files and upload SAST results #1800

Closed

Conversation

ccronca
Copy link

@ccronca ccronca commented Aug 16, 2024

This update configures the SAST task to upload SARIF results to quay.io for long-term storage

Please note that this PR was automatically generated and may include unrelated changes due to automatic YAML formatting performed by yq
The YAML files will be indented using 2 spaces, if the YAML file uses indentationless list the automation will try to keep this format

The PR contains two separate commits:

  1. Format YAML files: Ensures consistent indentation and formatting of the YAML files
  2. Upload SAST results: Configures the PipelineRun files to enable uploading SARIF results to quay.io

Separating these changes into two commits simplifies the review process. The first commit focuses on indentation and formatting, while the second commit contains the semantic changes

Related:

Format PipelineRun files with yq for consistent indentation and format

Signed-off-by: ccronca <[email protected]>
Configure the SAST task to upload SARIF results to quay.io for
long-term storage

Signed-off-by: ccronca <[email protected]>
@ccronca ccronca requested a review from a team as a code owner August 16, 2024 08:51
@Molter73
Copy link
Collaborator

I know this sort of comment is annoying, but why do we need the first commit that does automatic formatting and nothing else? We've written our tekton files in a way that make them easy for us to work with, since we write them by hand and are not auto-generated, removing the spaces between blocks just makes them hard to spot.

If we want to keep this new style of formatting, I would ask that we add a way to enforce it in CI so it stays consistent, otherwise we drop the first commit and only keep the changes that are meaningful.

@ccronca
Copy link
Author

ccronca commented Aug 16, 2024

I know this sort of comment is annoying, but why do we need the first commit that does automatic formatting and nothing else? We've written our tekton files in a way that make them easy for us to work with, since we write them by hand and are not auto-generated, removing the spaces between blocks just makes them hard to spot.

If we want to keep this new style of formatting, I would ask that we add a way to enforce it in CI so it stays consistent, otherwise we drop the first commit and only keep the changes that are meaningful.

Hi @Molter73 please note that this PR was done programmatically, so maintaining the same format as the original file is quite complex due to limitations in the underlying tools used. If you don't want to merge all the changes, you can implement just the changes from the second commit in the PR.

@msugakov
Copy link
Contributor

Hi @ccronca

This, I think, is the first time we receive a PR contribution from the Konflux team.
Do you foresee that we'll become a common practice in the future?

Not sure about yq, but the formatting would not be lost if files are processed with Python in streaming mode. It's tiny bit more difficult than loading and saving, but it preserves the original style.

@ccronca
Copy link
Author

ccronca commented Aug 16, 2024

Hi @ccronca

This, I think, is the first time we receive a PR contribution from the Konflux team. Do you foresee that we'll become a common practice in the future?

Not sure about yq, but the formatting would not be lost if files are processed with Python in streaming mode. It's tiny bit more difficult than loading and saving, but it preserves the original style.

Please note that I'm part of the ProdSec team, not Konfux. This PR is required to push the results to quay.io for long-term storage. Without this, the sast-snyk-check will show an error due to the results not being pushed. This ensures ProdSec can access security scans for RedHat-related products. If this doesn't apply to you, feel free to ignore this PR.

@msugakov
Copy link
Contributor

There's absolutely no problem to connect it to quay, I'm re-creating these PRs for that but without formatting changes.

msugakov added a commit that referenced this pull request Aug 16, 2024
@msugakov msugakov mentioned this pull request Aug 16, 2024
1 task
@msugakov
Copy link
Contributor

#1801 is to replace this PR.

@msugakov msugakov closed this Aug 16, 2024
tommartensen pushed a commit that referenced this pull request Aug 19, 2024
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.

3 participants