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.

Sign up for GitHub

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

Jump to bottom

sast: initial task for Coverity Buildless #1411

Merged
merged 1 commit into from
Dec 4, 2024
Merged

sast: initial task for Coverity Buildless #1411

merged 1 commit into from
Dec 4, 2024

Conversation

jperezdealgaba
Copy link
Contributor

@jperezdealgaba jperezdealgaba commented Sep 13, 2024 •
edited
Loading

Initial version of the Coverity Buildless task. The code will be scanned using coverity buildless mode, then the results are processuing using csgrep and the results are later filtered using csfilter-kfp.

This is a draft request and this can not be merged in this repository. It will be merged in a newly created repository.
Things pending to do:

  • Move this MR to a new and private repo - Not needed anymore
  • Remove installation of csdiff package once the container image is updated
  • Speak on how to store license in a secure place
  • Show results on UI
  • Enhanced Readme with private information - Not needed anymore

Apart from that, the MR can be reviewed as the funcionality will remain the same

@jperezdealgaba jperezdealgaba force-pushed the coverity-buildless branch 2 times, most recently from 48b9d8c to 06b8305 Compare September 15, 2024 15:30
@jperezdealgaba
Copy link
Contributor Author

jperezdealgaba commented Sep 15, 2024 •
edited
Loading

Results are parsed and shown in the UI:

Screenshot 2024-09-15 at 12 48 00

Here we have an example pipeline: https://konflux.apps.stone-prod-p02.hjvn.p1.openshiftapps.com/application-pipeline/workspaces/jperezde/applications/test-coverity/pipelineruns/ec-cli-on-pull-request-292mf

@jperezdealgaba jperezdealgaba force-pushed the coverity-buildless branch 5 times, most recently from 7d6c4fb to e833b16 Compare September 17, 2024 17:56
kdudka
kdudka requested changes Sep 18, 2024
View reviewed changes
task/sast-coverity-buildless-check/0.1/README.md Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/README.md Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/README.md Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/README.md Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/sast-coverity-buildless-check.yaml Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/sast-coverity-buildless-check.yaml Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/sast-coverity-buildless-check.yaml Outdated Show resolved Hide resolved
task/sast-coverity-buildless-check/0.1/sast-coverity-buildless-check.yaml Outdated Show resolved Hide resolved
@jperezdealgaba
Copy link
Contributor Author

Thanks for the thorough review @kdudka !

kdudka
kdudka reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

@jperezdealgaba Please close the review threads that have been resolved. I do not have sufficient permission to do it myself.

task/sast-coverity-buildless-check/0.1/sast-coverity-buildless-check.yaml Outdated Show resolved Hide resolved
@kdudka kdudka mentioned this pull request Sep 19, 2024
Merged
@jperezdealgaba jperezdealgaba force-pushed the coverity-buildless branch 2 times, most recently from 97be5ae to e52f763 Compare September 19, 2024 11:29
@jperezdealgaba
Copy link
Contributor Author

Solved all comments and added new changes. I will also apply the changes (the record excluded and update the upload task) to the snyk code task

kdudka
kdudka reviewed Sep 19, 2024
View reviewed changes
task/sast-coverity-check/0.1/README.md Outdated Show resolved Hide resolved
task/sast-coverity-check/0.1/README.md Outdated Show resolved Hide resolved
@jperezdealgaba jperezdealgaba force-pushed the coverity-buildless branch 3 times, most recently from f65c313 to ffff308 Compare September 20, 2024 09:28
kdudka
kdudka requested changes Sep 23, 2024
View reviewed changes
task/sast-coverity-check/0.1/README.md Outdated Show resolved Hide resolved
@jperezdealgaba
Copy link
Contributor Author

jperezdealgaba commented Sep 25, 2024 •
edited
Loading

Hey! @kdudka I just did a new MR with all the discussed changes:

The relationship between the two tasks are defined in the following file: pipelines/template-build/template-build.yaml
This is a big MR so I guess we should have Konflux eyes here

@jperezdealgaba jperezdealgaba reopened this Dec 3, 2024
@jperezdealgaba
Copy link
Contributor Author

@tnevrlka Would you mind retriggering the tests? This was just merged: release-engineering/rhtap-ec-policy#85 (comment)

@tnevrlka
Copy link
Contributor

tnevrlka commented Dec 3, 2024

/ok-to-test

@tnevrlka
Copy link
Contributor

tnevrlka commented Dec 3, 2024

/ok-to-test

@tnevrlka
Copy link
Contributor

tnevrlka commented Dec 3, 2024

/retest

@tnevrlka
Copy link
Contributor

tnevrlka commented Dec 4, 2024

/retest

@tnevrlka
Copy link
Contributor

tnevrlka commented Dec 4, 2024 •
edited
Loading

The Run Task Tests check was introduced yesterday by merging #1644 and is currently failing due to missing tests.

The README says When updating tasks, if the tasks doesn't have tests, try to add a few tests. Currently it is not mandatory, but is recommended. Would you be okay with merging this @tisutisu?

@chmeliik
Copy link
Contributor

chmeliik commented Dec 4, 2024

The Run Task Tests check was introduced yesterday by merging #1644 and is currently failing due to missing tests.

The README says When updating tasks, if the tasks doesn't have tests, try to add a few tests. Currently it is not mandatory, but is recommended. Would you be okay with merging this @tisutisu?

I think we also need a fix tor the Task Tests workflow then, tests should really be optional at the moment

@tisutisu
Copy link
Contributor

tisutisu commented Dec 4, 2024

The Run Task Tests check was introduced yesterday by merging #1644 and is currently failing due to missing tests.
The README says When updating tasks, if the tasks doesn't have tests, try to add a few tests. Currently it is not mandatory, but is recommended. Would you be okay with merging this @tisutisu?

I think we also need a fix tor the Task Tests workflow then, tests should really be optional at the moment

I have skipped task tests workflow with this PR till it is fixed.

@jperezdealgaba
Copy link
Contributor Author

Hey! Although I appreaciate your comments (and agree with you about skipping the tests). @tisutisu @chmeliik Could this be merged?
Thank you!

@jperezdealgaba
sast: initial task for Coverity Buildless
ac3c8d5 
Solves: https://issues.redhat.com/browse/OSH-740

Initial version of the Coverity Buildless task. In introduces two different tasks: A task checking the availability of Coverity license and authentication token, and a task for scanning the code. The code will be scanned using coverity buildless mode, then the results are processing using csgrep and the results are later filtered using csfilter-kfp.
@tnevrlka
Copy link
Contributor

tnevrlka commented Dec 4, 2024

/ok-to-test

@tnevrlka tnevrlka enabled auto-merge December 4, 2024 11:40
@tnevrlka tnevrlka disabled auto-merge December 4, 2024 11:56
@tnevrlka tnevrlka added this pull request to the merge queue Dec 4, 2024
Merged via the queue into konflux-ci:main with commit 9a46c89 Dec 4, 2024
16 checks passed
@rh-tap-build-team rh-tap-build-team bot mentioned this pull request Dec 4, 2024
Closed
@jperezdealgaba jperezdealgaba mentioned this pull request Dec 5, 2024
Merged
pierDipi
pierDipi reviewed Dec 6, 2024
View reviewed changes
pipelines/docker-build-multi-platform-oci-ta/README.md
|caTrustConfigMapKey| The name of the key in the ConfigMap that contains the CA bundle data.| ca-bundle.crt| |
|caTrustConfigMapName| The name of the ConfigMap to read CA bundle data from.| trusted-ca| |
|image-digest| Image digest to report findings for.| None| '$(tasks.build-container.results.IMAGE_DIGEST)'|
|image-url| Image URL.| None| '$(tasks.build-container.results.IMAGE_URL)'|
Copy link
Contributor

Choose a reason for hiding this comment

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

@jperezdealgaba for multi-arch I think this should be build-image-index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pierDipi Sorry! We weren't aware of this. Would you mind adding more information about how this is used in this tracker: https://issues.redhat.com/browse/OSH-790 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 6, 2024
@kdudka
template-build: fix parmeters of sast-coverity-check
3402a6e 
... and coverity-availability-check to make the template work with
multiarch builds.

Fixes: konflux-ci#1411
Resolves: https://issues.redhat.com/browse/OSH-790
Resolves: https://issues.redhat.com/browse/KFLUXSPRT-847
@kdudka kdudka mentioned this pull request Dec 6, 2024
Merged
kdudka added a commit to kdudka/build-definitions that referenced this pull request Dec 6, 2024
@kdudka
template-build: fix parmeters of sast-coverity-check
98f0dcb 
... and coverity-availability-check to make the template work with
multiarch builds.

Fixes: konflux-ci#1411
Resolves: https://issues.redhat.com/browse/OSH-790
Resolves: https://issues.redhat.com/browse/KFLUXSPRT-847
github-merge-queue bot pushed a commit that referenced this pull request Dec 9, 2024
@kdudka
template-build: fix parmeters of sast-coverity-check
e20e80e 
... and coverity-availability-check to make the template work with
multiarch builds.

Fixes: #1411
Resolves: https://issues.redhat.com/browse/OSH-790
Resolves: https://issues.redhat.com/browse/KFLUXSPRT-847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsztuka jsztuka jsztuka left review comments

@pierDipi pierDipi pierDipi left review comments

@kdudka kdudka kdudka approved these changes

@tnevrlka tnevrlka tnevrlka approved these changes

@hongweiliu17 hongweiliu17 hongweiliu17 approved these changes

@14rcole 14rcole 14rcole approved these changes

@mkosiarc mkosiarc Awaiting requested review from mkosiarc

@Josh-Everett Josh-Everett Awaiting requested review from Josh-Everett

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jperezdealgaba @hongweiliu17 @kdudka @tnevrlka @14rcole @chmeliik @tisutisu @pierDipi @jsztuka