Skip to content

Conversation

@jason-lynch
Copy link
Member

@jason-lynch jason-lynch commented Jan 14, 2026

Summary

Fixes two issues with the licenses-ci target:

  • go-licenses can output notices in non-deterministic order. I've changed this check to sort the contents of each NOTICE.txt before diffing to make it order-independent.
    • I'm writing these out to temporary files instead of using <() to maintain POSIX compatibility.
  • Backticks create a subshell in bash and other POSIX shells, which is why the output looked strange when this check fails. I'v replaced the backticks in the "Please commit..." message with single quotes.

Testing

  • Run make licenses-ci. It should report no differences.
  • Change the version of one of our packages in go.mod, such as incrementing afero to v1.15.0
  • Re-run make licenses-ci. It should report a difference.

Summary by CodeRabbit

  • Chores
    • Improved the license validation process in the build system with updated diff-based comparison logic and cleanup procedures.

✏️ Tip: You can customize this high-level summary in your review settings.

Fixes two issues with the licenses-ci target:

- go-licenses can output notices in non-deterministic order. I've
  changed this check to sort the contents of each `NOTICE.txt` before
  diffing to make it order-independent.
  - I'm writing these out to temporary files instead of using <() to
    maintain POSIX compatibility.
- Backticks create a subshell in bash and other POSIX shells, which is
  why the output looked strange when this check fails. I'v replaced the
  backticks in the "Please commit..." message with single quotes.
@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough

The licenses-ci target in the Makefile has been updated to replace git-diff-based verification with a sorted file comparison approach. The new implementation creates temporary files, sorts both local and upstream NOTICE.txt versions, compares them directly, and removes temporary artifacts after execution.

Changes

Cohort / File(s) Summary
Build System License Verification
Makefile
Modified licenses-ci target to replace git-diff-based check with sorted file comparison; introduces temporary files for local and upstream NOTICE.txt sorting; adds cleanup logic for temporary artifacts; preserves status messaging.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • rshoemaker

Poem

🐰 A Makefile most merry, now checking with care,
Sorting licenses left and right in the air,
Temp files dance briefly, then vanish away,
NOTICE.txt stays neat and tidy each day! 📋✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'ci: fix licenses-ci target' follows Conventional Commits style and clearly describes the main change—fixing the licenses-ci Make target.
Description check ✅ Passed The description covers summary, changes, and testing sections well, but lacks explicit checklist items and changelog/issue linking information required by the template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


🧹 Recent nitpick comments
Makefile (1)

143-150: Solid fix for the non-deterministic ordering issue.

The sorted comparison approach correctly addresses the ordering variability from go-licenses. One minor gap: if git show HEAD:NOTICE.txt fails (e.g., file doesn't exist in HEAD on first commit), licenses-ci-local.txt will be orphaned. This is unlikely to matter in ephemeral CI environments, but for robustness you could use a trap or consolidate into a single shell block:

♻️ Optional: ensure cleanup on intermediate failures
 .PHONY: licenses-ci
 licenses-ci: licenses
-	`@sort` NOTICE.txt > ./licenses-ci-local.txt
-	`@git` show HEAD:NOTICE.txt | sort > ./licenses-ci-upstream.txt
-	`@if` ! diff ./licenses-ci-local.txt ./licenses-ci-upstream.txt > /dev/null; then \
+	`@sort` NOTICE.txt > ./licenses-ci-local.txt; \
+	git show HEAD:NOTICE.txt | sort > ./licenses-ci-upstream.txt; \
+	diff_result=0; \
+	diff ./licenses-ci-local.txt ./licenses-ci-upstream.txt > /dev/null || diff_result=$$?; \
+	rm -f ./licenses-ci-local.txt ./licenses-ci-upstream.txt; \
+	if [ "$$diff_result" -ne 0 ]; then \
 		echo "Please commit the updated NOTICE.txt file via 'make licenses'."; \
-		rm ./licenses-ci-local.txt ./licenses-ci-upstream.txt; \
 		exit 1; \
 	fi
-	`@rm` ./licenses-ci-local.txt ./licenses-ci-upstream.txt
 	`@echo` "NOTICE.txt is up to date."

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5726ad9 and 666dd54.

📒 Files selected for processing (1)
  • Makefile
⏰ 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). (1)
  • GitHub Check: Codacy Static Code Analysis

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@rshoemaker rshoemaker left a comment

Choose a reason for hiding this comment

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

LGTM

@jason-lynch jason-lynch merged commit b32c835 into main Jan 14, 2026
3 checks passed
@jason-lynch jason-lynch deleted the ci/fix-licenses branch January 14, 2026 20:34
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.

3 participants