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

feat(ISV-5128): add new Tekton task to update component sboms #656

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

wcheang
Copy link
Contributor

@wcheang wcheang commented Oct 31, 2024

No description provided.

Copy link

openshift-ci bot commented Oct 31, 2024

Hi @wcheang. Thanks for your PR.

I'm waiting for a konflux-ci member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Contributor

@mmalina mmalina left a comment

Choose a reason for hiding this comment

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

"wip" is not a valid commit title prefix. This should probably be "feat". If you want to say that it's not ready for review yet, you can change the PR to Draft. (If a PR is created as Draft from the start, the reviewers won't be added until it's changed to Ready - maybe for next time.)

@wcheang wcheang marked this pull request as draft October 31, 2024 13:24
@wcheang wcheang changed the title wip(ISV-5128): add new Tekton task to update component sboms feat(ISV-5128): add new Tekton task to update component sboms Oct 31, 2024
@wcheang
Copy link
Contributor Author

wcheang commented Nov 1, 2024

@mmalina the CONTRIBUTING.md says Tekton task tests are not required, but the check is failing due to missing tests directory? All the logic is in the Python script and already covered by unit tests, but I can add some Tekton task tests too if it is required.

mmalina added a commit that referenced this pull request Nov 4, 2024
This recently came up in a PR:
#656

We definitely do require tests for all new tasks. For existing
tasks, we mostly do as well, but there are still three tasks
without tests:

- base64-encode-checksum
- create-internal-request
- prepare-validation

Signed-off-by: Martin Malina <[email protected]>
@mmalina
Copy link
Contributor

mmalina commented Nov 4, 2024

@mmalina the CONTRIBUTING.md says Tekton task tests are not required, but the check is failing due to missing tests directory? All the logic is in the Python script and already covered by unit tests, but I can add some Tekton task tests too if it is required.

The doc is not correct. Fix here: #664

To explain, when we came up with the mechanism to write task tests, none of the tasks had tests, but there would be a tests directory and a sample run.yaml which was a taskrun definition using the task. So the reason for that sentence was that if you changed an existing task, you wouldn't be required to write tests for it.

I understand that in your case the task will not do much besides running the script, but it's still nice have some basic test to ensure there is no error in the task definition or something like that. I would recommend using a mock for the python script and just check that it was called with the parameters it was expected to be called with.

@wcheang wcheang force-pushed the ISV-5128 branch 3 times, most recently from 69b389f to 642b662 Compare November 5, 2024 07:08
mmalina added a commit to mmalina/release-service-catalog that referenced this pull request Nov 5, 2024
This recently came up in a PR:
konflux-ci#656

We definitely do require tests for all new tasks. For existing
tasks, we mostly do as well, but there are still three tasks
without tests:

- base64-encode-checksum
- create-internal-request
- prepare-validation

Signed-off-by: Martin Malina <[email protected]>
mmalina added a commit that referenced this pull request Nov 5, 2024
This recently came up in a PR:
#656

We definitely do require tests for all new tasks. For existing
tasks, we mostly do as well, but there are still three tasks
without tests:

- base64-encode-checksum
- create-internal-request
- prepare-validation

Signed-off-by: Martin Malina <[email protected]>
@wcheang wcheang force-pushed the ISV-5128 branch 3 times, most recently from 24058f6 to 121ab41 Compare November 5, 2024 22:38
@wcheang
Copy link
Contributor Author

wcheang commented Nov 5, 2024

The symlink appears to be set up with the wrong path in my previous change in the utils repos. PR to fix it here: konflux-ci/release-service-utils#295

@wcheang wcheang force-pushed the ISV-5128 branch 3 times, most recently from 335ea29 to 35ac838 Compare November 6, 2024 19:52
@wcheang
Copy link
Contributor Author

wcheang commented Nov 6, 2024

@mmalina Could you please help me figure out why the Tekton task tests are running into "permission denied" errors on all the scripts (tests/pre-apply-task-hook.sh and /home/sbom/update_component_sbom? I looked everything over on both repos, and couldn't tell what I was missing. The /home/sbom dir is added to the path, and there doesn't seem to be any other permission change needed for the utils scripts.

@mmalina
Copy link
Contributor

mmalina commented Nov 8, 2024

@mmalina Could you please help me figure out why the Tekton task tests are running into "permission denied" errors on all the scripts (tests/pre-apply-task-hook.sh and /home/sbom/update_component_sbom? I looked everything over on both repos, and couldn't tell what I was missing. The /home/sbom dir is added to the path, and there doesn't seem to be any other permission change needed for the utils scripts.

The script doesn't have execute permissions:

$ ls -l sbom
total 72
lrwxr-xr-x@ 1 mmalina  staff    22  4 Oct 12:33 create_product_sbom -> create_product_sbom.py
-rwxr-xr-x@ 1 mmalina  staff  4862  8 Nov 13:12 create_product_sbom.py
-rw-r--r--@ 1 mmalina  staff  9927  8 Nov 13:12 test_create_product_sbom.py
-rw-r--r--@ 1 mmalina  staff  7457  4 Nov 09:15 test_update_component_sbom.py
lrwxr-xr-x@ 1 mmalina  staff    24  8 Nov 13:12 update_component_sbom -> update_component_sbom.py
-rw-r--r--@ 1 mmalina  staff  4466  4 Nov 09:15 update_component_sbom.py

That's something that's typically not shown in Github UI. It has the permissions it had when it was added. You can chmod 755 sbom/update_component_sbom.py and commit that change.

@wcheang wcheang force-pushed the ISV-5128 branch 3 times, most recently from 6ef7541 to 76e2c2c Compare November 11, 2024 22:02
@wcheang wcheang marked this pull request as ready for review November 11, 2024 22:02
@wcheang wcheang requested a review from mmalina November 11, 2024 22:11
@@ -54,8 +54,8 @@ spec:

NUM_COMPONENTS=$(jq '.components | length' "${PYXIS_FILE}")

mkdir /workdir/sboms
cd /workdir/sboms
mkdir "$(workspaces.data.path)/downloaded-sboms"
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should use a subdir for this like https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/collect-mrrc-params/collect-mrrc-params.yaml#L37
The reason is just in case we have concurrent runs using the same workspace, we want to use subdirs per pipelinerun to prevent collisions

metadata:
name: update-component-sbom
labels:
app.kubernetes.io/version: "0.0.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

No real way of knowing this, but we typically do 0.1.0 to start. Can you switch to this?

set -eux

INPUT_PATH="$(workspaces.data.path)/$(params.downloadedSbomPath)"
OUTPUT_PATH="$(workspaces.data.path)/updated-sboms"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not rewrite them in place?

@@ -92,6 +92,8 @@ spec:
echo "ERROR: Expected to fetch sbom for $PYXIS_IMAGES images, but only $SBOM_COUNT were saved"
exit 1
fi

echo -n "$(workspaces.data.path)/downloaded-sboms" > "$(results.downloadedSbomPath.path)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't want the workspace in the result. You use this
INPUT_PATH="$(workspaces.data.path)/$(params.downloadedSbomPath)"
so this is going to end up evaluating to /workspace/workspace/downloaded-sboms currently, which doesn't exist

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to also test that update_component_sbom was called the proper number of times. All this currently does is test echo -n "$OUTPUT_PATH" > "$(results.sbomPath.path)" from the task, which is only one line of multiple that exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants