fix double period character and actually test this#374
Conversation
|
Warning Rate limit exceeded@sensslen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe PR updates the GitHub Actions workflow to replace a single-line check with a multi-line block that writes license metadata to ./licenses-output.json and prints it. It adds "validate downloaded licenses" and "show downloaded licenses" PowerShell steps for net8, net9, net10 and net472 jobs to compare downloaded license files against the JSON, fail on mismatches, and list files. The final license listing loop path was adjusted to include framework subdirectories. In code, FileDownloader.cs now concatenates the file stem and extension without an extra dot. Two test project files update NUnit3TestAdapter to 6.0.1. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/action.yml (1)
324-332: Critical: Same matrix.framework issue.Line 327 also references the undefined
${{ matrix.framework }}. This path must be consistent with the download path. Apply the same fix chosen for the validation step above.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/action.yml(3 hunks)src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs(1 hunks)tests/NuGetLicense.Test/NuGetLicense.Test.csproj(1 hunks)tests/NuGetUtility.Test/NuGetUtility.Test.csproj(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
📚 Learning: 2025-11-07T21:33:27.230Z
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
Applied to files:
.github/workflows/action.yml
🧬 Code graph analysis (2)
.github/workflows/action.yml (2)
src/NuGetLicense/Program.cs (2)
Task(127-175)NuGetLicense(34-360)src/NuGetUtility/Wrapper/NuGetWrapper/Packaging/LicenseMetadata.cs (1)
LicenseMetadata(10-23)
src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs (2)
src/NuGetUtility/Wrapper/HttpClientWrapper/IFileDownloader.cs (3)
NuGetUtility(4-10)IFileDownloader(6-9)Task(8-8)src/NuGetUtility/Wrapper/HttpClientWrapper/NopFileDownloader.cs (2)
NopFileDownloader(6-12)NuGetUtility(4-13)
🪛 actionlint (1.7.9)
.github/workflows/action.yml
274-274: property "framework" is not defined in object type {project: string}
(expression)
326-326: property "framework" is not defined in object type {project: string}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check_licenses (net10.0, App)
- GitHub Check: check_licenses (net9.0, Tests)
- GitHub Check: check_licenses (net9.0, ProjectWithReferenceContainingFileLicense)
- GitHub Check: test (ubuntu-latest, net10.0)
- GitHub Check: test_file_license_matching (windows-latest)
- GitHub Check: test_file_license_matching (macos-latest)
- GitHub Check: test (ubuntu-latest, net8.0)
- GitHub Check: test_file_license_matching (ubuntu-latest)
- GitHub Check: test_windows (net8.0)
- GitHub Check: test_windows (net10.0)
- GitHub Check: test_windows (net9.0)
- GitHub Check: test_windows (net472)
🔇 Additional comments (6)
tests/NuGetUtility.Test/NuGetUtility.Test.csproj (1)
26-26: LGTM - consistent with other test project.The NUnit3TestAdapter version update matches the change in tests/NuGetLicense.Test/NuGetLicense.Test.csproj. See the verification comment in that file.
src/NuGetUtility/Wrapper/HttpClientWrapper/FileDownloader.cs (1)
30-30: LGTM - fixes the double period bug.The fix correctly removes the explicit dot since
Path.GetExtension()already includes the leading dot (e.g., ".txt", ".html"). The old code produced filenames like "package..txt" while the new code correctly produces "package.txt"..github/workflows/action.yml (4)
167-170: LGTM - enables validation testing.The change to output JSON to a file and display it enables the subsequent validation step to verify downloaded licenses match the metadata.
171-221: LGTM - comprehensive validation logic.The validation step thoroughly checks that downloaded license files match the JSON output, accounting for both .txt and .html extensions. The error reporting clearly identifies missing and unexpected files.
223-231: LGTM - path correctly includes framework subdirectory.The updated path correctly references the framework-specific subdirectory where licenses are downloaded.
268-270: Note: Download path differs from non-net472 jobs.The net472 job downloads to
./licenses/${{ matrix.project }}while the check_licenses job downloads to./licenses/${{ matrix.project }}/${{ matrix.framework }}. Ensure the validation step accounts for this difference.
sensslen
force-pushed
the
simon/fix-duplicated-period
branch
from
8421f65 to
9d022eb
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/action.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
📚 Learning: 2025-11-07T21:33:27.230Z
Learnt from: sensslen
Repo: sensslen/nuget-license PR: 327
File: .github/workflows/release.yml:38-38
Timestamp: 2025-11-07T21:33:27.230Z
Learning: In the release workflow (.github/workflows/release.yml), the test assembly pattern `NuGet*.Test.dll` intentionally excludes SPDXLicenseMatcher.Test.dll from the release workflow tests.
Applied to files:
.github/workflows/action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: check_licenses (net9.0, App)
- GitHub Check: check_licenses (net10.0, ProjectWithReferenceContainingLicenseExpression)
- GitHub Check: check_licenses (net9.0, ProjectWithReferenceContainingWindowsSpecificPath)
- GitHub Check: check_licenses (net9.0, Tests)
- GitHub Check: check_licenses (net10.0, App)
- GitHub Check: check_licenses (net9.0, ProjectWithReferenceContainingLicenseExpression)
- GitHub Check: check_licenses (net9.0, ProjectWithReferenceContainingFileLicense)
- GitHub Check: test_file_license_matching (macos-latest)
- GitHub Check: test_windows (net9.0)
- GitHub Check: test_windows (net10.0)
- GitHub Check: check_licenses_net472 (App)
- GitHub Check: check_licenses_net472 (Tests)
- GitHub Check: test_windows (net472)
- GitHub Check: test_windows (net8.0)
- GitHub Check: check_version_command_net472
- GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingLicenseExpression)
- GitHub Check: test_file_license_matching (windows-latest)
- GitHub Check: check_licenses_net472 (ProjectWithReferenceContainingWindowsSpecificPath)
- GitHub Check: test_file_license_matching (ubuntu-latest)
🔇 Additional comments (4)
.github/workflows/action.yml (4)
167-170: Good addition of JSON output capture.Piping the license check output to a JSON file enables the subsequent validation steps. The framework subdirectory in the path correctly matches the job's matrix configuration.
171-221: Excellent validation logic for downloaded licenses.The PowerShell validation step correctly:
- Compares actual downloaded files against the JSON metadata
- Checks for both missing (expected but absent) and extra (unexpected) files
- Handles both
.txtand.htmlextensions- Provides clear error messages with package details
- Uses the same path as the download step (line 168)
This validation will catch the double-period issue described in issue #372—if files are named with
..htmlinstead of.html, they'll be reported as missing.
268-270: Correct JSON output addition for net472.The download path
./licenses/${{ matrix.project }}correctly omits the framework subdirectory since thecheck_licenses_net472job matrix only definesproject(lines 235-237), notframework. This addresses the previous review concern.
272-322: Validation logic correctly implemented for net472.The validation path
./licenses/${{ matrix.project }}(line 276) correctly matches the download path (line 269) and omits the undefinedmatrix.frameworkvariable. This resolves the critical issue flagged in the previous review where an undefined variable would have caused path mismatches.
1 participant
fixes #372
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.