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

SAST tests: Creation of tests for SAST tasks #1843

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jperezdealgaba
Copy link
Contributor

Before you complete this pull request ...

Look for any open pull requests in the repository with the title "e2e-tests update" and
see if there are recent e2e-tests updates that will be applicable to your change.

@jperezdealgaba jperezdealgaba force-pushed the sast-tests branch 4 times, most recently from d895de6 to b472131 Compare January 26, 2025 18:10
@jperezdealgaba
Copy link
Contributor Author

@kdudka Would you mind giving a preliminary review? There are a few discussions needed here

workspace: tests-workspace
params:
- name: url
value: https://github.com/jperezdealgaba/test_unicode_control
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdudka This is the repo that was used to test the original task (https://gitlab.cee.redhat.com/chhan/unicode_control_test). I moved it to GitHub as the tests couldn't resolve to the internal GitLab.
This is in my personal organization, I really think this should be moved to a more "professional" org if not choosing a different repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ralphbean Could we include the test data into https://github.com/konflux-ci/testrepo or another project in the konflux-ci GitHub namespace so that the tests do not depend on 3rd party repositories?

@kdudka
Copy link
Contributor

kdudka commented Jan 27, 2025

/ok-to-test

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.

Code-wise looks fine to me.

value: task/init/0.2/init.yaml
params:
- name: image-url
value: "quay.io/redhat-user-workloads/jperezde-tenant/tests/tests-sast-unicode-check:latest"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another aspect to discuss is the fact that this repo doesn't exist. So the upload is never done. I understand that the upload process is outside of the scope of the function.
Do we want to upload the results?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a meaningful image-url that could be used for testing, it would increase the test coverage. If not, we can modify the task to skip the upload in case an empty string is provided as image-url, which is how the sast-snyk-check task worked initially.

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