From 63bc7a7e79b21c5576d4cebc34acc62a09d19643 Mon Sep 17 00:00:00 2001 From: Kayvan Sylvan Date: Mon, 21 Jul 2025 15:43:14 -0700 Subject: [PATCH] feat: improve timestamp handling and merge commit detection in changelog generator ## CHANGES - Add debug logging for date parsing failures - Pass forcePRSync parameter explicitly to fetchPRs method - Implement comprehensive merge commit detection using parents - Capture actual commit timestamps from GitHub API - Calculate version dates from most recent commit - Add parent commit SHAs for merge detection - Use real commit dates instead of current time - Add timestamp validation with fallback handling --- .../internal/cache/cache.go | 6 +- .../internal/changelog/generator.go | 11 ++- .../changelog/merge_detection_test.go | 82 +++++++++++++++++++ .../internal/changelog/processing.go | 79 ++++++++++++++++-- .../internal/github/client.go | 13 +++ .../internal/github/types.go | 9 +- 6 files changed, 183 insertions(+), 17 deletions(-) create mode 100644 cmd/generate_changelog/internal/changelog/merge_detection_test.go diff --git a/cmd/generate_changelog/internal/cache/cache.go b/cmd/generate_changelog/internal/cache/cache.go index 657b7ce2..8638bb06 100644 --- a/cmd/generate_changelog/internal/cache/cache.go +++ b/cmd/generate_changelog/internal/cache/cache.go @@ -204,7 +204,11 @@ func (c *Cache) GetVersions() (map[string]*git.Version, error) { // Try RFC3339Nano first (for nanosecond precision), then fall back to RFC3339 v.Date, err = time.Parse(time.RFC3339Nano, dateStr.String) if err != nil { - v.Date, _ = time.Parse(time.RFC3339, dateStr.String) + fmt.Printf("Error parsing date with RFC3339Nano: %v\n", err) + v.Date, err = time.Parse(time.RFC3339, dateStr.String) + if err != nil { + fmt.Printf("Error parsing date with RFC3339: %v\n", err) + } } } diff --git a/cmd/generate_changelog/internal/changelog/generator.go b/cmd/generate_changelog/internal/changelog/generator.go index cc7f03ac..abc92f4b 100644 --- a/cmd/generate_changelog/internal/changelog/generator.go +++ b/cmd/generate_changelog/internal/changelog/generator.go @@ -65,7 +65,7 @@ func (g *Generator) Generate() (string, error) { return "", fmt.Errorf("failed to collect data: %w", err) } - if err := g.fetchPRs(); err != nil { + if err := g.fetchPRs(g.cfg.ForcePRSync); err != nil { return "", fmt.Errorf("failed to fetch PRs: %w", err) } @@ -193,7 +193,7 @@ func (g *Generator) collectData() error { return nil } -func (g *Generator) fetchPRs() error { +func (g *Generator) fetchPRs(forcePRSync bool) error { // First, load all cached PRs if g.cache != nil { cachedPRs, err := g.cache.GetAllPRs() @@ -229,7 +229,7 @@ func (g *Generator) fetchPRs() error { } // If we have never synced or it's been more than 24 hours, do a full sync // Also sync if we have versions with PR numbers that aren't cached - needsSync := lastSync.IsZero() || time.Since(lastSync) > 24*time.Hour || g.cfg.ForcePRSync || missingPRs + needsSync := lastSync.IsZero() || time.Since(lastSync) > 24*time.Hour || forcePRSync || missingPRs if !needsSync { fmt.Fprintf(os.Stderr, "Using cached PR data (last sync: %s)\n", lastSync.Format("2006-01-02 15:04:05")) @@ -706,10 +706,9 @@ func (g *Generator) SyncDatabase() error { fmt.Fprintf(os.Stderr, "🔄 Starting database synchronization...\n") - // Step 1: Force PR sync (reuse existing logic) + // Step 1: Force PR sync (pass true explicitly) fmt.Fprintf(os.Stderr, "📥 Forcing PR sync from GitHub...\n") - g.cfg.ForcePRSync = true - if err := g.fetchPRs(); err != nil { + if err := g.fetchPRs(true); err != nil { return fmt.Errorf("failed to sync PRs: %w", err) } diff --git a/cmd/generate_changelog/internal/changelog/merge_detection_test.go b/cmd/generate_changelog/internal/changelog/merge_detection_test.go new file mode 100644 index 00000000..613b930d --- /dev/null +++ b/cmd/generate_changelog/internal/changelog/merge_detection_test.go @@ -0,0 +1,82 @@ +package changelog + +import ( + "testing" + "time" + + "github.com/danielmiessler/fabric/cmd/generate_changelog/internal/github" +) + +func TestIsMergeCommit(t *testing.T) { + tests := []struct { + name string + commit github.PRCommit + expected bool + }{ + { + name: "Regular commit with single parent", + commit: github.PRCommit{ + SHA: "abc123", + Message: "Fix bug in user authentication", + Author: "John Doe", + Date: time.Now(), + Parents: []string{"def456"}, + }, + expected: false, + }, + { + name: "Merge commit with multiple parents", + commit: github.PRCommit{ + SHA: "abc123", + Message: "Merge pull request #42 from feature/auth", + Author: "GitHub", + Date: time.Now(), + Parents: []string{"def456", "ghi789"}, + }, + expected: true, + }, + { + name: "Merge commit detected by message pattern only", + commit: github.PRCommit{ + SHA: "abc123", + Message: "Merge pull request #123 from user/feature-branch", + Author: "GitHub", + Date: time.Now(), + Parents: []string{}, // Empty parents - fallback to message detection + }, + expected: true, + }, + { + name: "Merge branch commit pattern", + commit: github.PRCommit{ + SHA: "abc123", + Message: "Merge branch 'feature' into main", + Author: "Developer", + Date: time.Now(), + Parents: []string{"def456"}, // Single parent but merge pattern + }, + expected: true, + }, + { + name: "Regular commit with no merge patterns", + commit: github.PRCommit{ + SHA: "abc123", + Message: "Add new feature for user management", + Author: "Jane Doe", + Date: time.Now(), + Parents: []string{"def456"}, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isMergeCommit(tt.commit) + if result != tt.expected { + t.Errorf("isMergeCommit() = %v, expected %v for commit: %s", + result, tt.expected, tt.commit.Message) + } + }) + } +} diff --git a/cmd/generate_changelog/internal/changelog/processing.go b/cmd/generate_changelog/internal/changelog/processing.go index 5b15c1b6..15f81fe0 100644 --- a/cmd/generate_changelog/internal/changelog/processing.go +++ b/cmd/generate_changelog/internal/changelog/processing.go @@ -14,6 +14,32 @@ import ( "github.com/danielmiessler/fabric/cmd/generate_changelog/internal/github" ) +// isMergeCommit determines if a commit is a merge commit based on multiple criteria +func isMergeCommit(commit github.PRCommit) bool { + // Primary method: Check parent count (merge commits have multiple parents) + if len(commit.Parents) > 1 { + return true + } + + // Fallback method: Check commit message patterns + // Common merge commit message patterns + mergePatterns := []string{ + `^Merge pull request #\d+`, // "Merge pull request #123 from..." + `^Merge branch '.*' into .*`, // "Merge branch 'feature' into main" + `^Merge remote-tracking branch`, // "Merge remote-tracking branch..." + `^Merge '.*' into .*`, // "Merge 'feature' into main" + `^Merge .*`, // General "Merge ..." patterns + } + + for _, pattern := range mergePatterns { + if matched, _ := regexp.MatchString(pattern, commit.Message); matched { + return true + } + } + + return false +} + // ProcessIncomingPR processes a single PR for changelog entry creation func (g *Generator) ProcessIncomingPR(prNumber int) error { if err := g.validatePRState(prNumber); err != nil { @@ -129,8 +155,24 @@ func (g *Generator) CreateNewChangelogEntry(version string) error { return nil } + // Calculate the version date for the changelog entry as the most recent commit date from processed PRs + changelogDate := time.Now() // fallback to current time + if len(fetchedPRs) > 0 { + var mostRecentCommitDate time.Time + for _, pr := range fetchedPRs { + for _, commit := range pr.Commits { + if commit.Date.After(mostRecentCommitDate) { + mostRecentCommitDate = commit.Date + } + } + } + if !mostRecentCommitDate.IsZero() { + changelogDate = mostRecentCommitDate + } + } + entry := fmt.Sprintf("## %s (%s)\n\n%s", - version, time.Now().Format("2006-01-02"), strings.TrimLeft(content.String(), "\n")) + version, changelogDate.Format("2006-01-02"), strings.TrimLeft(content.String(), "\n")) if err := g.insertVersionAtTop(entry); err != nil { return fmt.Errorf("failed to update CHANGELOG.md: %w", err) @@ -152,14 +194,21 @@ func (g *Generator) CreateNewChangelogEntry(version string) error { // Save individual commits to cache for each PR for _, pr := range fetchedPRs { for _, commit := range pr.Commits { + // Use actual commit timestamp, with fallback to current time if invalid + commitDate := commit.Date + if commitDate.IsZero() { + commitDate = time.Now() + fmt.Fprintf(os.Stderr, "Warning: Commit %s has invalid timestamp, using current time\n", commit.SHA) + } + // Convert github.PRCommit to git.Commit gitCommit := &git.Commit{ SHA: commit.SHA, Message: commit.Message, Author: commit.Author, - Email: "", // Not available from GitHub API - Date: time.Now(), // Use current time as fallback - IsMerge: false, // We don't have this info from GitHub API + Email: "", // Not available from GitHub API + Date: commitDate, // Use actual commit timestamp from GitHub API + IsMerge: isMergeCommit(commit), // Detect merge commits using parents and message patterns PRNumber: pr.Number, } if err := g.cache.SaveCommit(gitCommit, version); err != nil { @@ -169,12 +218,28 @@ func (g *Generator) CreateNewChangelogEntry(version string) error { } } + // Calculate the version date as the most recent commit date from processed PRs + versionDate := time.Now() // fallback to current time + if len(fetchedPRs) > 0 { + var mostRecentCommitDate time.Time + for _, pr := range fetchedPRs { + for _, commit := range pr.Commits { + if commit.Date.After(mostRecentCommitDate) { + mostRecentCommitDate = commit.Date + } + } + } + if !mostRecentCommitDate.IsZero() { + versionDate = mostRecentCommitDate + } + } + // Create a proper new version entry for the database newVersionEntry := &git.Version{ Name: version, - Date: time.Now(), - CommitSHA: "", // Will be set when the release commit is made - PRNumbers: prNumbers, // Now we have the actual PR numbers + Date: versionDate, // Use most recent commit date instead of current time + CommitSHA: "", // Will be set when the release commit is made + PRNumbers: prNumbers, // Now we have the actual PR numbers AISummary: content.String(), } diff --git a/cmd/generate_changelog/internal/github/client.go b/cmd/generate_changelog/internal/github/client.go index 1231d73f..ada1b6a7 100644 --- a/cmd/generate_changelog/internal/github/client.go +++ b/cmd/generate_changelog/internal/github/client.go @@ -207,6 +207,18 @@ func (c *Client) convertGitHubPR(ghPR *github.PullRequest, commits []*github.Rep } if commit.Commit.Author != nil { prCommit.Author = getString(commit.Commit.Author.Name) + // Capture actual commit timestamp from GitHub API + if commit.Commit.Author.Date != nil { + prCommit.Date = commit.Commit.Author.Date.Time + } + } + // Capture parent commit SHAs for merge detection + if commit.Parents != nil { + for _, parent := range commit.Parents { + if parent.SHA != nil { + prCommit.Parents = append(prCommit.Parents, *parent.SHA) + } + } } result.Commits = append(result.Commits, prCommit) } @@ -395,6 +407,7 @@ func (c *Client) FetchAllMergedPRsGraphQL(since time.Time) ([]*PR, error) { SHA: commitNode.Commit.OID, Message: strings.TrimSpace(commitNode.Commit.Message), Author: commitNode.Commit.Author.Name, + Date: commitNode.Commit.AuthoredDate, // Use actual commit timestamp } pr.Commits = append(pr.Commits, commit) } diff --git a/cmd/generate_changelog/internal/github/types.go b/cmd/generate_changelog/internal/github/types.go index 6b48a1c9..d7ae9bd2 100644 --- a/cmd/generate_changelog/internal/github/types.go +++ b/cmd/generate_changelog/internal/github/types.go @@ -26,6 +26,8 @@ type PRCommit struct { SHA string Message string Author string + Date time.Time // Add timestamp field + Parents []string // Add parent commits for merge detection } // GraphQL query structures for hasura client @@ -50,9 +52,10 @@ type PullRequestsQuery struct { Commits struct { Nodes []struct { Commit struct { - OID string `graphql:"oid"` - Message string - Author struct { + OID string `graphql:"oid"` + Message string + AuthoredDate time.Time `graphql:"authoredDate"` + Author struct { Name string } }