Conversation
…lterSBOM) Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
📝 WalkthroughWalkthroughThe StripSBOM function is modified to preserve the Relationships field instead of explicitly clearing it to nil. Corresponding test expectations are updated to reflect that relationships remain in the SBOM after stripping. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/apis/softwarecomposition/v1beta1/syfttypes_test.go (2)
40-40: Consider a stronger assertion thanNotNilto confirm relationship content is preserved.
assert.NotNilonly guards againstnil; it won't catchStripSBOMpartially modifying or replacing the slice. Since both fixtures seed exactly one relationship,assert.Lenis a tighter check with almost no extra cost.♻️ Proposed refinement
- assert.NotNil(t, sbom.Relationships, "relationships should be preserved") + assert.Len(t, sbom.Relationships, 1, "relationships should be preserved")Apply the same change at both line 40 and line 54.
Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/apis/softwarecomposition/v1beta1/syfttypes_test.go` at line 40, Replace the weak NotNil checks on the relationships slice with a length assertion to ensure content is preserved: where the test currently calls assert.NotNil(t, sbom.Relationships, "relationships should be preserved") (and the similar call at the second occurrence), change it to assert.Len(t, sbom.Relationships, 1, "relationships should be preserved") so the test verifies that the single seeded relationship remains after StripSBOM; keep the same message and apply the change for both occurrences referencing sbom.Relationships.
15-79: Add a test case for a nilRelationshipsfield to close the "preserve" contract.The PR goal is to preserve relationships — which means nil should stay nil (no accidental initialization) and non-nil should stay non-nil. Currently only the non-nil path is exercised. A small extra case validates the other half:
♻️ Suggested additional test case
+ { + name: "nil relationships preserved", + input: &sbom.SBOM{ + Artifacts: sbom.Artifacts{ + FileMetadata: map[syftfile.Coordinates]syftfile.Metadata{}, + FileDigests: map[syftfile.Coordinates][]syftfile.Digest{}, + FileContents: map[syftfile.Coordinates]string{}, + Unknowns: map[syftfile.Coordinates][]string{}, + }, + Relationships: nil, + }, + verify: func(t *testing.T, sbom *sbom.SBOM) { + assert.Nil(t, sbom.Relationships, "nil relationships should remain nil after strip") + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/apis/softwarecomposition/v1beta1/syfttypes_test.go` around lines 15 - 79, Test suite TestStripSBOM is missing a case that verifies nil Relationships are preserved (stay nil) — add a new test case named "nil relationships" in the tests slice that supplies an SBOM whose Relationships field is nil (e.g. create an input via a helper like createSBOMWithNilRelationships() or clone createCompleteSBOM and set Relationships = nil) and a verify function that asserts sbom.Relationships is nil and also checks the same preservation/clearing expectations as the other cases (Descriptor.Configuration cleared, FileMetadata/FileDigests/FileContents preserved, FileLicenses/Executables cleared, Unknowns preserved, and package-level assertions if packages exist); place this case alongside the existing "nil packages collection" and "complete SBOM" cases so both nil and non-nil relationship paths are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/apis/softwarecomposition/v1beta1/syfttypes_test.go`:
- Line 40: Replace the weak NotNil checks on the relationships slice with a
length assertion to ensure content is preserved: where the test currently calls
assert.NotNil(t, sbom.Relationships, "relationships should be preserved") (and
the similar call at the second occurrence), change it to assert.Len(t,
sbom.Relationships, 1, "relationships should be preserved") so the test verifies
that the single seeded relationship remains after StripSBOM; keep the same
message and apply the change for both occurrences referencing
sbom.Relationships.
- Around line 15-79: Test suite TestStripSBOM is missing a case that verifies
nil Relationships are preserved (stay nil) — add a new test case named "nil
relationships" in the tests slice that supplies an SBOM whose Relationships
field is nil (e.g. create an input via a helper like
createSBOMWithNilRelationships() or clone createCompleteSBOM and set
Relationships = nil) and a verify function that asserts sbom.Relationships is
nil and also checks the same preservation/clearing expectations as the other
cases (Descriptor.Configuration cleared, FileMetadata/FileDigests/FileContents
preserved, FileLicenses/Executables cleared, Unknowns preserved, and
package-level assertions if packages exist); place this case alongside the
existing "nil packages collection" and "complete SBOM" cases so both nil and
non-nil relationship paths are exercised.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/apis/softwarecomposition/v1beta1/syfttypes.gopkg/apis/softwarecomposition/v1beta1/syfttypes_test.go
💤 Files with no reviewable changes (1)
- pkg/apis/softwarecomposition/v1beta1/syfttypes.go
|
Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit