feat(sync): improve content sync PR summary#6
Conversation
6ec9e50 to
4506374
Compare
3b2cb9e to
bb906de
Compare
marcusburghardt
left a comment
There was a problem hiding this comment.
@sonupreetam , the code LGTM but I think for next updates ideally we should create a new spec in specs instead of updating already merged specs. This help us to keep the history of changes more transparent and easier to follow. Also, the incremental specs are expected to be much smaller and easier to review.
|
@marcusburghardt Apologies for the confusion there. After PR #4 merges and we rebase PR #6 onto main, the diff will shrink to just the 2 commits that are truly PR #6's work. So the previous spec is truly PR #4 work. But you are right to point out about the incremental specs for this PR. So, I would add the specs with the next PR. |
The merge-base changed after approval.
Signed-off-by: Sonu Preetam <spreetam@redhat.com>
Signed-off-by: Sonu Preetam <spreetam@redhat.com>
bb906de to
df44a1a
Compare
| return len(r.added) > 0 || len(r.updated) > 0 || len(r.removed) > 0 | ||
| } | ||
|
|
||
| func shortSHA(sha string) string { |
There was a problem hiding this comment.
@sonupreetam this is a completely non-blocking nit, but a potential good "issue" to open for an advancement later / for community mtg. Maybe I could open an issue later with "Make shortSHA length 7 characters to align with Git default SHA format." Wdyt? That could be added to the Community board.
There was a problem hiding this comment.
@hbraswelrh Although it is required for a website repo, we can definitely create an issue to align with the format. Thank you!!
There was a problem hiding this comment.
@hbraswelrh I checked the Git's --abbrev-commit uses 7 as a starting point but grows as needed for uniqueness. There's no canonical "the short SHA is always 7 chars" contract. Also it confers no correctness or usability benefit for the website's use case. So no issue is needed for this, if you did create, let me know we can close it.
| } | ||
| } | ||
|
|
||
| func (r *syncResult) writeUpdatedRepoBlock(b *strings.Builder, name string) { |
There was a problem hiding this comment.
@sonupreetam another non-blocking but potential later refinement would be adding another case where oldSHA is empty but newSHA is present with a fallback to "Pinned to." Wdyt as an issue for the Community Board? It's not necessary at the moment.
There was a problem hiding this comment.
@hbraswelrh Thank you for your review here. The cleanup.go should take care of any orphan files, and there shouldn't be an oldSHA being empty. Will investigate it if this is an issue and will let you know about the community board.
There was a problem hiding this comment.
@hbraswelrh For the website's production use case, the fallback is not needed, --lock is always present in both the deploy and check workflows, so oldSHA is always populated for updated repos. It's only the local developer run and --write flag will always take care of it. So no issue is needed for this, if you did create, let me know we can close it.
hbraswelrh
left a comment
There was a problem hiding this comment.
LGTM. Left two non-blocking nit comments that could be converted to issues (nit-picky enhancements).
Summary
The weekly content sync PR (
sync-content-check.yml) currently produces a minimal summary — just repo names and a single "synced" counter that conflates repos with files. This makes it hard for reviewers to understand what changed.Dependency: #4
Issue first appeared in: #5
This PR enhances
toMarkdown()to produce a rich, actionable summary:old_sha...new_sha) so reviewers can click through to see upstream changesBefore
After
Changes
sync.go: AddrepoSummarystruct,repoFilestracking,filesProcessedcounter, richtoMarkdown()with helper methodsmain.go: Enrich repo details with old SHAs from the lock before generating the summarysync_test.go: 13 new test functions covering all summary variationsTest plan
go vet ./...— cleangofmt -l .— cleango test -race -count=1 ./...— all tests pass (existing + 13 new)Made with Cursor