Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 29 additions & 16 deletions fastlane/Fastfile
Original file line number Diff line number Diff line change
Expand Up @@ -2501,17 +2501,19 @@ platform :ios do
end


# Regenerated baselines make the freshness diff empty, so the gate needs the merge base.
# Regenerated baselines make the freshness diff empty, so the gate needs an earlier base.
runner = ->(*command) { sh(*command, log: false) }
merge_base = ApiDiffHelper.resolve_merge_base(runner: runner)
UI.message("Comparing against the merge base #{merge_base}")
branch = ApiDiffHelper.current_branch(runner: runner)
on_main = ApiDiffHelper.main_branch?(branch)
comparison_base = ApiDiffHelper.resolve_comparison_base(runner: runner, branch: branch)
UI.message("Comparing #{branch} against #{comparison_base}")

breaks_by_scheme = {}
reports_by_target = {}

schemes.each do |scheme|
base_dir = File.join(ApiDiffHelper::MERGE_BASE_SWIFTINTERFACE_DIR, scheme)
ApiDiffHelper.extract_baselines_at(merge_base, base_dir, scheme, runner: runner)
base_dir = File.join(ApiDiffHelper::BASE_SWIFTINTERFACE_DIR, scheme)
ApiDiffHelper.extract_baselines_at(comparison_base, base_dir, scheme, runner: runner)
prefix = ApiDiffHelper.api_file_prefix(scheme)
scheme_breaks = []

Expand Down Expand Up @@ -2554,11 +2556,16 @@ platform :ios do

# Informational: a GitHub or Slack outage must not decide whether the gate fails.
begin
announcement = notify_api_changes_on_slack(
reports_by_target: reports_by_target,
breaks: all_breaks,
labels: labels
)
# Announcing only from main keeps the feed to one post per change.
announcement = if on_main
notify_api_changes_on_slack(
reports_by_target: reports_by_target,
breaks: all_breaks,
labels: labels
)
else
{ fingerprint: nil, notice: nil }
end
schemes.each do |scheme|
upsert_api_diff_comment(
scheme: scheme,
Expand All @@ -2578,7 +2585,12 @@ platform :ios do
end

if ApiDiffHelper.gate_blocked?(all_breaks, labels)
UI.user_error!("Breaking public API changes detected. Add #{ApiDiffHelper::BREAKING_CHANGE_LABEL} if intentional.")
# main has no PR to read the label from, and the PR run already gated the change.
if on_main
UI.important("Breaking public API changes on main, already gated on the PR run")
else
UI.user_error!("Breaking public API changes detected. Add #{ApiDiffHelper::BREAKING_CHANGE_LABEL} if intentional.")
end
end
end

Expand Down Expand Up @@ -2645,7 +2657,7 @@ platform :ios do
bot_token = ENV["SLACK_ACCESS_TOKEN_CIRCLE_CI_NOTIFY_ORB_IOS"]
channel = ENV["SLACK_CHANNEL_SDK_NEW_API"]

source = api_gate_pr_link
source = api_gate_commit_link
modules = ApiDiffHelper.changed_modules(options[:reports_by_target])

summary = ApiDiffHelper.slack_summary(
Expand Down Expand Up @@ -2702,11 +2714,12 @@ platform :ios do
comment ? comment["body"].to_s : ""
end

private_lane :api_gate_pr_link do
pr_number = detect_pr_number
next "" if pr_number.nil?
# The commit page carries the PR link, and its sha is what tells a rerun it already announced.
private_lane :api_gate_commit_link do
sha = sh("git", "rev-parse", "HEAD", log: false).strip
next "" if sha.empty?

"<https://github.com/RevenueCat/#{REPO_NAME}/pull/#{pr_number}|##{pr_number}>"
"<https://github.com/RevenueCat/#{REPO_NAME}/commit/#{sha}|#{sha[0, 7]}>"
end


Expand Down
35 changes: 31 additions & 4 deletions fastlane/api_diff_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,27 @@ def api_changes_reported?(output)
!output.to_s.include?(NO_CHANGES_MARKER)
end

MERGE_BASE_SWIFTINTERFACE_DIR = "/tmp/merge-base-swiftinterface".freeze
BASE_SWIFTINTERFACE_DIR = "/tmp/comparison-base-swiftinterface".freeze

MAIN_BRANCH = "main".freeze

def main_branch?(branch)
branch.to_s.strip == MAIN_BRANCH
end

def current_branch(runner:)
branch = ENV["CIRCLE_BRANCH"].to_s.strip
return branch unless branch.empty?

runner.call("git", "rev-parse", "--abbrev-ref", "HEAD").to_s.strip
end

# On main the merge base is HEAD, so HEAD^ is what the merge replaced.
def resolve_comparison_base(runner:, branch:)
return resolve_previous_commit(runner: runner) if main_branch?(branch)

resolve_merge_base(runner: runner)
end

# The PR's own baselines match the generated files once regenerated, erasing the evidence.
def resolve_merge_base(runner:)
Expand All @@ -307,6 +327,13 @@ def resolve_merge_base(runner:)
sha
end

def resolve_previous_commit(runner:)
sha = runner.call("git", "rev-parse", "HEAD^").to_s.strip
raise "Could not resolve the commit before HEAD" if sha.empty?

sha
end

def extract_baselines_at(sha, destination_dir, scheme, runner:)
FileUtils.mkdir_p(destination_dir)
prefix = api_file_prefix(scheme)
Expand Down Expand Up @@ -743,11 +770,11 @@ def unbroken_modifications(modifications, breaks)
def slack_summary(breaks, labels, source:, new_declarations: [], modules: [], modifications: [])
changed = unbroken_modifications(modifications, breaks)
headline = if breaks.any?
gate_blocked?(breaks, labels) ? ":warning: *Breaking public API changes*" : ":warning: *Breaking public API changes* (allowed by label)"
gate_blocked?(breaks, labels) ? ":warning: *Breaking public API landed on main*" : ":warning: *Breaking public API landed on main* (allowed by label)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Breaks always look unapproved

Low Severity

Slack announcements now run only on main, where pr_labels_for_api_gate always returns no labels. slack_summary still picks its breaking headline via gate_blocked?, so intentional pr:breaking-api landings always post the unapproved wording and never the allowed-by-label variant.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d688eb7. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we only merge approved prs, and only post merged to main, then all posts are approved 🤷

elsif new_declarations.any?
":sparkles: *New public API*"
":sparkles: *New public API landed on main*"
else
":pencil2: *Public API changed*"
":pencil2: *Public API changed on main*"
end
headline = [headline, announcement_identity(modules)].join(" · ")

Expand Down
99 changes: 94 additions & 5 deletions fastlane/api_diff_helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,52 @@ def test_resolve_merge_base_raises_when_empty
assert_match(/merge base/, error.message)
end

def test_current_branch_prefers_the_ci_variable
with_circle_branch("main") do
assert_equal "main", ApiDiffHelper.current_branch(runner: ->(*_c) { "detached\n" })
end
end

def test_current_branch_falls_back_to_git
with_circle_branch(nil) do
runner = ->(*command) { command == ["git", "rev-parse", "--abbrev-ref", "HEAD"] ? "my-branch\n" : "" }

assert_equal "my-branch", ApiDiffHelper.current_branch(runner: runner)
end
end

# On main the merge base is HEAD, so reusing it would compare the commit against itself.
def test_comparison_base_on_main_is_the_previous_commit
runner = lambda do |*command|
case command
when ["git", "rev-parse", "HEAD^"] then "prevsha\n"
when ["git", "merge-base", "origin/main", "HEAD"] then "headsha\n"
end
end

assert_equal "prevsha", ApiDiffHelper.resolve_comparison_base(runner: runner, branch: "main")
end

def test_comparison_base_off_main_stays_the_merge_base
runner = lambda do |*command|
case command
when ["git", "rev-parse", "HEAD^"] then "prevsha\n"
when ["git", "merge-base", "origin/main", "HEAD"] then "forksha\n"
end
end

["my-branch", "release/5.86.0", "gh-readonly-queue/main/pr-1-abc"].each do |branch|
assert_equal "forksha", ApiDiffHelper.resolve_comparison_base(runner: runner, branch: branch)
end
end

# A root commit would otherwise diff the whole API in as new.
def test_previous_commit_raises_when_empty
error = assert_raises(RuntimeError) { ApiDiffHelper.resolve_previous_commit(runner: ->(*_c) { "\n" }) }

assert_match(/before HEAD/, error.message)
end

def test_extract_baselines_writes_one_file_per_platform
Dir.mktmpdir do |dir|
runner = ->(*command) { "public func fromMergeBase()\n// #{command.last}\n" }
Expand Down Expand Up @@ -1333,15 +1379,15 @@ def test_slack_summary_leads_with_breaks_when_there_are_any

message = ApiDiffHelper.slack_summary(breaks, [], source: "<url|#42> Some PR", new_declarations: ["public func a()"])

assert message.start_with?(":warning: *Breaking public API changes*")
assert message.start_with?(":warning: *Breaking public API landed on main*")
assert_includes message, "<url|#42> Some PR"
assert_includes message, "1 potential break"
end

def test_slack_summary_leads_with_new_api_when_nothing_breaks
message = ApiDiffHelper.slack_summary([], [], source: "<url|#42> Some PR", new_declarations: ["public func a()", "public var b: Swift.Int"])

assert message.start_with?(":sparkles: *New public API*")
assert message.start_with?(":sparkles: *New public API landed on main*")
assert_includes message, "2 new declarations"
end

Expand Down Expand Up @@ -1689,7 +1735,7 @@ def test_slack_summary_reports_an_attribute_only_modification

message = ApiDiffHelper.slack_summary([], [], source: "<url|#7439>", modules: ["RevenueCat"], modifications: modifications)

assert message.start_with?(":pencil2: *Public API changed* · iOS :ios: · `RevenueCat`")
assert message.start_with?(":pencil2: *Public API changed on main* · iOS :ios: · `RevenueCat`")
assert_includes message, "1 modification"
assert_includes message, "~ added @available(*, deprecated…): "
assert_includes message, "purchaseDate(forEntitlement"
Expand Down Expand Up @@ -1726,7 +1772,7 @@ def test_slack_summary_still_leads_with_new_api_when_something_was_added
[], [], source: "", new_declarations: ["public func a()"], modules: ["RevenueCat"], modifications: modifications
)

assert message.start_with?(":sparkles: *New public API*")
assert message.start_with?(":sparkles: *New public API landed on main*")
assert_includes message, "1 new declaration, 1 modification"
assert_includes message, "+ public func a()"
assert_includes message, "~ added @available"
Expand All @@ -1735,7 +1781,7 @@ def test_slack_summary_still_leads_with_new_api_when_something_was_added
def test_slack_summary_labels_the_platform_and_modules
message = ApiDiffHelper.slack_summary([], [], source: "<url|#42>", new_declarations: ["public func a()"], modules: ["RevenueCatUI"])

assert message.start_with?(":sparkles: *New public API* · iOS :ios: · `RevenueCatUI`")
assert message.start_with?(":sparkles: *New public API landed on main* · iOS :ios: · `RevenueCatUI`")
end

def test_changed_modules_names_only_the_schemes_that_changed
Expand Down Expand Up @@ -1835,8 +1881,51 @@ def test_the_announcement_happens_before_the_comment_is_written
end


# A rerun of the same main job must not post twice, and last_announcement bails on an empty
# source, so the commit link is what keeps the suppression alive.
def test_the_announcement_source_is_the_commit
lane = File.read(File.expand_path("Fastfile", __dir__))
link = lane[/private_lane :api_gate_commit_link do.*?\n end\n/m]

refute_nil link, "the api_gate_commit_link lane moved; update this test"
assert_match(%r{/commit/}, link, "the message links the commit, not the PR")
assert_match(/next "" if sha\.empty\?/, link)
refute_match(/detect_pr_number/, lane[/source = api_gate_commit_link/] || "x")
end

# The feed was noisy because every PR run posted. Only main does now, so each change lands once.
def test_slack_is_announced_only_on_main
lane = File.read(File.expand_path("Fastfile", __dir__))
announce = lane[/announcement = if on_main.*?\n end\n/m]

refute_nil announce, "the Slack announcement is no longer gated on main; update this test"
assert_match(/notify_api_changes_on_slack/, announce)
assert_match(/\{ fingerprint: nil, notice: nil \}/, announce,
"a PR run still needs an announcement shape for the comment")
end

# main carries no PR to hold the label, so the gate there would fail changes the PR approved.
def test_the_breaking_change_gate_cannot_redden_main
lane = File.read(File.expand_path("Fastfile", __dir__))
gate = lane[/if ApiDiffHelper\.gate_blocked\?.*?\n end\n/m]

refute_nil gate, "the gate section of check_api_changes moved; update this test"
assert_match(/if on_main/, gate, "main must not fail the gate")
assert_operator gate.index("if on_main"), :<, gate.index("UI.user_error!"),
"the main branch of the gate must come before the failure"
end


# --- Attribute additions: allowlist, not denylist ---

def with_circle_branch(value)
previous = ENV["CIRCLE_BRANCH"]
ENV["CIRCLE_BRANCH"] = value
yield
ensure
ENV["CIRCLE_BRANCH"] = previous
end

def modification_adding(attribute)
"// From\npublic func f()\n\n// To\npublic func f()\n\n/**\nChanges:\n- Added attribute `#{attribute}`\n*/"
end
Expand Down