-
Notifications
You must be signed in to change notification settings - Fork 231
[CI] Get the correct target branch on public interface validation #3833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpdates Fastlane lane to compute the target branch dynamically, run public interface analysis on the current branch, switch to the computed target branch to run the analysis again, generate a diff report, optionally post a PR comment in CI, and finally restore the original branch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as CI/Developer
participant FL as Fastlane (validate_public_interface)
participant GIT as Git
participant ANALYZER as Interface Analyzer
participant GH as GitHub PR
CI->>FL: Invoke lane
FL->>GIT: Read env (GITHUB_BASE_REF) & current branch
FL->>FL: Determine target_branch (release/*→main, else develop)
Note right of FL: Logs selected target_branch
rect rgba(200,230,255,0.3)
FL->>ANALYZER: Analyze current branch
ANALYZER-->>FL: Current report
end
FL->>GIT: fetch target_branch
FL->>GIT: checkout target_branch
rect rgba(200,255,200,0.3)
FL->>ANALYZER: Analyze target branch
ANALYZER-->>FL: Target report
end
FL->>FL: Generate diff report
alt Running in CI
FL->>GH: Post PR comment with diff
else Local
FL->>CI: Print diff to console
end
FL->>GIT: fetch original_branch
FL->>GIT: checkout original_branch
Note over FL,GIT: Workspace restored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fastlane/Fastfile (2)
858-863: Consider adding error handling for git operations.The checkout logic is functionally correct, but the git commands could fail if the target branch doesn't exist or if there are local changes preventing the checkout.
Consider wrapping the git commands with error handling or verification:
# Checkout the target branch +begin sh("git fetch origin #{target_branch}") sh("git checkout #{target_branch}") +rescue => e + UI.user_error!("Failed to checkout target branch '#{target_branch}': #{e.message}") +endAlternatively, verify the branch exists before attempting checkout:
# Verify target branch exists sh("git ls-remote --heads origin #{target_branch} | grep -q #{target_branch}") rescue UI.user_error!("Target branch '#{target_branch}' does not exist")
884-886: Consider wrapping branch restoration in an ensure block.The original branch restoration should occur even if earlier steps fail. Currently, if any step between lines 856-882 raises an exception, the lane might leave the working directory on the wrong branch.
Apply this pattern to ensure branch restoration:
original_branch = current_branch target_branch = ENV['GITHUB_BASE_REF'] || (original_branch.include?('release/') ? 'main' : 'develop') UI.important("Target branch: #{target_branch} 🕊️") + begin # Run the analysis on the current branch sh('interface-analyser analysis ../Sources/ public_interface_current.json') # Checkout the target branch sh("git fetch origin #{target_branch}") sh("git checkout #{target_branch}") # Run the analysis on the target branch sh('interface-analyser analysis ../Sources/ public_interface_previous.json') # Run diff report_path = 'interface-analyser-report.md' sh("interface-analyser diff public_interface_current.json public_interface_previous.json #{report_path}") # Check if report exists and is non-zero in size diff = if File.exist?(report_path) && File.size(report_path) > 0 File.read(report_path).strip else '🚀 No changes affecting the public interface.' end # Generate markdown table for the PR comment header = '## Public Interface' content = "#{header}\n#{diff}" # Post PR comment if running in CI pr_comment(text: content, edit_last_comment_with_text: header) if is_ci + ensure # Checkout the original branch sh("git fetch origin #{original_branch}") sh("git checkout #{original_branch}") + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
fastlane/Fastfile(1 hunks)
⏰ 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). (4)
- GitHub Check: Automated Code Review
- GitHub Check: Build Test App and Frameworks
- GitHub Check: Build LLC + UI (Xcode 15)
- GitHub Check: Metrics
🔇 Additional comments (3)
fastlane/Fastfile (3)
850-853: LGTM!The dynamic target branch determination is well-implemented. It correctly prioritizes
GITHUB_BASE_REFfrom the CI environment and falls back to a sensible convention based on branch naming (release branches → main, others → develop).
855-856: LGTM!Running the interface analysis on the current branch before switching is the correct approach. This captures the state of the PR branch for later comparison.
865-882: LGTM!The diff generation and PR comment posting logic is well-structured:
- Correctly runs the diff comparison between current and previous analyses
- Handles the case where no changes are detected
- Posts a clear markdown comment to the PR
Summary by CodeRabbit