From 993922e8f4c17c95ec1518a0a75e35084691f8e5 Mon Sep 17 00:00:00 2001 From: Neil Carvalho Date: Mon, 18 May 2026 09:25:48 -0300 Subject: [PATCH 1/3] Replace gh CLI with Octokit for GitHub API calls We're calling `gh` directly, which needs a lot of extra set up to test the action. GitHub has its own gem that wraps its API, `octokit`. It's much easier to test `octokit` calls instead of a CLI. This commit swaps the shell-based GhClient for one backed by the octokit gem. `GithubClient` now takes `token` and an optional `client` instead of `runner`. The exe reads `GITHUB_TOKEN` and passes it through. --- Gemfile | 3 + Gemfile.lock | 32 +++++ action.yml | 3 +- exe/importmap-update | 12 +- lib/executor.rb | 2 +- lib/gh_client.rb | 156 --------------------- lib/github_client.rb | 87 ++++++++++++ test/executor_test.rb | 2 +- test/gh_client_test.rb | 277 ------------------------------------- test/github_client_test.rb | 150 ++++++++++++++++++++ 10 files changed, 284 insertions(+), 440 deletions(-) delete mode 100644 lib/gh_client.rb create mode 100644 lib/github_client.rb delete mode 100644 test/gh_client_test.rb create mode 100644 test/github_client_test.rb diff --git a/Gemfile b/Gemfile index 34af3d7..c25b3cb 100644 --- a/Gemfile +++ b/Gemfile @@ -1,9 +1,12 @@ source "https://rubygems.org" ruby ">= 3.2" +gem "octokit" + group :development, :test do gem "rake" gem "minitest" + gem "minitest-mock", "~> 5.27" end group :development do diff --git a/Gemfile.lock b/Gemfile.lock index 140c829..27a331b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,19 +1,35 @@ GEM remote: https://rubygems.org/ specs: + addressable (2.9.0) + public_suffix (>= 2.0.2, < 8.0) ast (2.4.3) drb (2.2.3) + faraday (2.14.2) + faraday-net_http (>= 2.0, < 3.5) + json + logger + faraday-net_http (3.4.2) + net-http (~> 0.5) json (2.19.5) language_server-protocol (3.17.0.5) lint_roller (1.1.0) + logger (1.7.0) minitest (6.0.6) drb (~> 2.0) prism (~> 1.5) + minitest-mock (5.27.0) + net-http (0.9.1) + uri (>= 0.11.1) + octokit (10.0.0) + faraday (>= 1, < 3) + sawyer (~> 0.9) parallel (1.28.0) parser (3.3.11.1) ast (~> 2.4.1) racc prism (1.9.0) + public_suffix (7.0.5) racc (1.8.1) rainbow (3.1.1) rake (13.4.2) @@ -37,6 +53,9 @@ GEM rubocop (>= 1.75.0, < 2.0) rubocop-ast (>= 1.47.1, < 2.0) ruby-progressbar (1.13.0) + sawyer (0.9.3) + addressable (>= 2.3.5) + faraday (>= 0.17.3, < 3) standard (1.54.0) language_server-protocol (~> 3.17.0.2) lint_roller (~> 1.0) @@ -54,6 +73,7 @@ GEM unicode-display_width (3.2.0) unicode-emoji (~> 4.1) unicode-emoji (4.2.0) + uri (1.1.1) PLATFORMS arm64-darwin-25 @@ -61,19 +81,29 @@ PLATFORMS DEPENDENCIES minitest + minitest-mock (~> 5.27) + octokit rake standardrb CHECKSUMS + addressable (2.9.0) sha256=7fdf6ac3660f7f4e867a0838be3f6cf722ace541dd97767fa42bc6cfa980c7af ast (2.4.3) sha256=954615157c1d6a382bc27d690d973195e79db7f55e9765ac7c481c60bdb4d383 drb (2.2.3) sha256=0b00d6fdb50995fe4a45dea13663493c841112e4068656854646f418fda13373 + faraday (2.14.2) sha256=73ccb9994a9e8648f010e32eca2ae82e41c57860aa10932cda29418b9e0223ad + faraday-net_http (3.4.2) sha256=f147758260d3526939bf57ecf911682f94926a3666502e24c69992765875906c json (2.19.5) sha256=218a18553e4801d579ca7e0f5bc72bafd776d7397238a1fb4e74db5b0a812c59 language_server-protocol (3.17.0.5) sha256=fd1e39a51a28bf3eec959379985a72e296e9f9acfce46f6a79d31ca8760803cc lint_roller (1.1.0) sha256=2c0c845b632a7d172cb849cc90c1bce937a28c5c8ccccb50dfd46a485003cc87 + logger (1.7.0) sha256=196edec7cc44b66cfb40f9755ce11b392f21f7967696af15d274dde7edff0203 minitest (6.0.6) sha256=153ea36d1d987a62942382b61075745042a2b3123b1cd48f4c3675af9cc7d6f1 + minitest-mock (5.27.0) sha256=7040ed7185417a966920987eaa6eaf1be4ea1fc5b25bb03ff4703f98564a55b0 + net-http (0.9.1) sha256=25ba0b67c63e89df626ed8fac771d0ad24ad151a858af2cc8e6a716ca4336996 + octokit (10.0.0) sha256=82e99a539b7637b7e905e6d277bb0c1a4bed56735935cc33db6da7eae49a24e8 parallel (1.28.0) sha256=33e6de1484baf2524792d178b0913fc8eb94c628d6cfe45599ad4458c638c970 parser (3.3.11.1) sha256=d17ace7aabe3e72c3cc94043714be27cc6f852f104d81aa284c2281aecc65d54 prism (1.9.0) sha256=7b530c6a9f92c24300014919c9dcbc055bf4cdf51ec30aed099b06cd6674ef85 + public_suffix (7.0.5) sha256=1a8bb08f1bbea19228d3bed6e5ed908d1cb4f7c2726d18bd9cadf60bc676f623 racc (1.8.1) sha256=4a7f6929691dbec8b5209a0b373bc2614882b55fc5d2e447a21aaa691303d62f rainbow (3.1.1) sha256=039491aa3a89f42efa1d6dec2fc4e62ede96eb6acd95e52f1ad581182b79bc6a rake (13.4.2) sha256=cb825b2bd5f1f8e91ca37bddb4b9aaf345551b4731da62949be002fa89283701 @@ -82,12 +112,14 @@ CHECKSUMS rubocop-ast (1.49.1) sha256=4412f3ee70f6fe4546cc489548e0f6fcf76cafcfa80fa03af67098ffed755035 rubocop-performance (1.26.1) sha256=cd19b936ff196df85829d264b522fd4f98b6c89ad271fa52744a8c11b8f71834 ruby-progressbar (1.13.0) sha256=80fc9c47a9b640d6834e0dc7b3c94c9df37f08cb072b7761e4a71e22cff29b33 + sawyer (0.9.3) sha256=0d0f19298408047037638639fe62f4794483fb04320269169bd41af2bdcf5e41 standard (1.54.0) sha256=7a4b08f83d9893083c8f03bc486f0feeb6a84d48233b40829c03ef4767ea0100 standard-custom (1.0.2) sha256=424adc84179a074f1a2a309bb9cf7cd6bfdb2b6541f20c6bf9436c0ba22a652b standard-performance (1.9.0) sha256=49483d31be448292951d80e5e67cdcb576c2502103c7b40aec6f1b6e9c88e3f2 standardrb (1.0.1) sha256=7a1328be429f4e97a97e357e2446f3509e80164a59ff00bc6a4daa78e3351f2c unicode-display_width (3.2.0) sha256=0cdd96b5681a5949cdbc2c55e7b420facae74c4aaf9a9815eee1087cb1853c42 unicode-emoji (4.2.0) sha256=519e69150f75652e40bf736106cfbc8f0f73aa3fb6a65afe62fefa7f80b0f80f + uri (1.1.1) sha256=379fa58d27ffb1387eaada68c749d1426738bd0f654d812fcc07e7568f5c57c6 RUBY VERSION ruby 4.0.1 diff --git a/action.yml b/action.yml index c271563..65f46ff 100644 --- a/action.yml +++ b/action.yml @@ -11,7 +11,7 @@ inputs: required: false default: ".github/importmap-updates.yml" github-token: - description: "Token used by gh to list, open, edit, and close PRs." + description: "Token used by Octokit to list, open, edit, and close PRs." required: true base-branch: description: "Branch to base PRs against." @@ -50,7 +50,6 @@ runs: shell: bash working-directory: ${{ inputs.working-directory }} env: - GH_TOKEN: ${{ inputs.github-token }} GITHUB_TOKEN: ${{ inputs.github-token }} GITHUB_REPOSITORY: ${{ github.repository }} INPUT_CONFIG_FILE: ${{ inputs.config-file }} diff --git a/exe/importmap-update b/exe/importmap-update index ed3f0ee..9eef128 100755 --- a/exe/importmap-update +++ b/exe/importmap-update @@ -17,7 +17,7 @@ # INPUT_CONFIG_FILE Path to the YAML config (default # .github/importmap-updates.yml). # GITHUB_REPOSITORY OWNER/REPO of the consuming repo. -# GITHUB_TOKEN / GH_TOKEN Auth for gh (the action.yml passes this in). +# GITHUB_TOKEN / GH_TOKEN Auth token passed to Octokit (action.yml passes this in). # IMPORTMAP_BASE_BRANCH Base branch for PRs (default `main`). # IMPORTMAP_DRY_RUN Set to "true" for a no-side-effects run. # IMPORTMAP_AUTHOR_NAME Git author name for commits. @@ -31,7 +31,7 @@ require "config" require "planner" require "reconciler" require "executor" -require "gh_client" +require "github_client" require "git_client" require "commands" require "parsers/outdated_parser" @@ -156,8 +156,14 @@ when :run exit 2 end + token = ENV["GITHUB_TOKEN"] || ENV["GH_TOKEN"] + if token.nil? || token.empty? + warn "GITHUB_TOKEN (or GH_TOKEN) is not set; refusing to run." + exit 2 + end + runner = Importmap::Update::Commands::ShellRunner.new - gh = Importmap::Update::GhClient.new(repo: repo, runner: runner) + gh = Importmap::Update::GitHubClient.new(repo: repo, token: token) git = Importmap::Update::GitClient.new( runner: runner, author_name: ENV.fetch("IMPORTMAP_AUTHOR_NAME", "github-actions[bot]"), diff --git a/lib/executor.rb b/lib/executor.rb index ce8f806..3017fd4 100644 --- a/lib/executor.rb +++ b/lib/executor.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require_relative "commands" -require_relative "gh_client" +require_relative "github_client" require_relative "git_client" require_relative "metadata" diff --git a/lib/gh_client.rb b/lib/gh_client.rb deleted file mode 100644 index 02dff18..0000000 --- a/lib/gh_client.rb +++ /dev/null @@ -1,156 +0,0 @@ -# frozen_string_literal: true - -require "json" -require_relative "commands" -require_relative "reconciler" - -module Importmap - module Update - # Wraps the `gh` CLI to give the executor a typed interface against - # GitHub. Every method shells out through a Commands::ShellRunner (or a - # FixtureRunner in tests). The wrapper does no orchestration logic — - # that's the executor's job — but it does parse `gh`'s JSON output into - # the structs the rest of the codebase already knows how to handle. - # - # Authentication: `gh` reads $GH_TOKEN or $GITHUB_TOKEN from the - # environment. Inside a GitHub Action, ${{ secrets.GITHUB_TOKEN }} - # passed as the `github-token` input is exposed as $GITHUB_TOKEN - # in the shell where this runs. - # - # Repository: every `gh` invocation uses --repo OWNER/REPO so we don't - # depend on the current directory being a git checkout pointed at the - # right remote. The action.yml will pass $GITHUB_REPOSITORY through. - class GhClient - # We deliberately cap below GitHub's hard limit. A consumer with more - # than this many bot-PRs open simultaneously has bigger problems - # than the action can fix, and we surface a warning in that case. - MAX_OPEN_PRS = 100 - - def initialize(repo:, runner: Commands::ShellRunner.new) - @repo = repo - @runner = runner - end - - # Returns an array of Reconciler::ExistingPR for all open PRs whose - # branch starts with `branch_prefix`. The body field is included so - # the reconciler can parse the metadata block out of it. - def list_open_prs(branch_prefix:) - # `head:foo/` is a GitHub-search-syntax prefix match. We can't use - # `gh pr list --head` because that's an exact-match filter. - result = @runner.run!( - "gh", "pr", "list", - "--repo", @repo, - "--state", "open", - "--search", "head:#{branch_prefix}/", - "--limit", MAX_OPEN_PRS.to_s, - "--json", "number,headRefName,title,body" - ) - parse_pr_list(result.stdout, branch_prefix) - end - - # Creates a new PR. Branch must already exist on the remote. - # Returns the new PR's number. - def create_pr(branch:, base:, title:, body:, labels: []) - argv = [ - "gh", "pr", "create", - "--repo", @repo, - "--head", branch, - "--base", base, - "--title", title, - "--body", body - ] - labels.each { |l| argv.push("--label", l) } - result = @runner.run!(*argv) - # `gh pr create` prints the PR URL on stdout; extract the number. - result.stdout.strip[%r{/pull/(\d+)}, 1]&.to_i - end - - # Ensures every label in +labels+ exists in the repo, creating any that - # are missing. Called once before opening PRs so that `create_pr` never - # fails with "label does not exist". Missing labels are created with a - # neutral default color; existing labels are left untouched. - def ensure_labels(labels) - return if labels.empty? - existing = list_label_names - labels.each do |label| - next if existing.include?(label) - @runner.run( - "gh", "label", "create", label, - "--repo", @repo, - "--color", "0075ca" - ) - end - end - - # Edits an existing PR's title and body. Used after force-pushing a - # changed branch — the body must be re-rendered so the metadata - # block reflects the new package set. - def update_pr(number:, title:, body:) - @runner.run!( - "gh", "pr", "edit", number.to_s, - "--repo", @repo, - "--title", title, - "--body", body - ) - nil - end - - # Closes a PR, optionally leaving a comment explaining why. The - # comment is what tells reviewers "this was managed by the action - # and was closed because…", which beats a silent close every time. - def close_pr(number:, comment: nil) - if comment && !comment.empty? - @runner.run!( - "gh", "pr", "comment", number.to_s, - "--repo", @repo, - "--body", comment - ) - end - @runner.run!( - "gh", "pr", "close", number.to_s, - "--repo", @repo - ) - nil - end - - private - - def list_label_names - result = @runner.run( - "gh", "label", "list", - "--repo", @repo, - "--json", "name", - "--limit", "200" - ) - return [] unless result.success? - JSON.parse(result.stdout.force_encoding("UTF-8")).map { |l| l["name"] } - rescue JSON::ParserError - [] - end - - def parse_pr_list(stdout, branch_prefix) - parsed = JSON.parse(stdout.force_encoding("UTF-8")) - # `head:foo/` is a *search* term, not a strict filter — GitHub's - # search can return PRs whose branch matches loosely. Belt and - # suspenders: re-filter on the client side too. - parsed - .select { |pr| pr["headRefName"].to_s.start_with?("#{branch_prefix}/") } - .map do |pr| - Reconciler::ExistingPR.new( - number: pr["number"], - branch: pr["headRefName"], - title: pr["title"], - body: pr["body"].to_s - ) - end - rescue JSON::ParserError => e - raise Commands::CommandError.new( - ["gh", "pr", "list"], - Commands::Result.new( - stdout: stdout, stderr: "Invalid JSON from gh: #{e.message}", exit_code: 1 - ) - ) - end - end - end -end diff --git a/lib/github_client.rb b/lib/github_client.rb new file mode 100644 index 0000000..5e536b2 --- /dev/null +++ b/lib/github_client.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require "octokit" +require_relative "reconciler" + +module Importmap + module Update + # Wraps the Octokit client to give the executor a typed interface against + # GitHub. Every method calls the injected Octokit client (or a test double). + # The wrapper does no orchestration logic — that's the executor's job — but + # it does translate Octokit responses into the structs the rest of the + # codebase already knows how to handle. + # + # Authentication: pass the token directly; inside a GitHub Action use + # ${{ secrets.GITHUB_TOKEN }} exposed as $GITHUB_TOKEN in the shell. + class GitHubClient + # Matches the old gh cap; GitHub's REST API allows up to 100 per_page. + MAX_OPEN_PRS = 100 + + def initialize(repo:, token:, client: nil) + @repo = repo + @client = client || Octokit::Client.new(access_token: token) + end + + # Returns an array of Reconciler::ExistingPR for all open PRs whose + # branch starts with `branch_prefix`. Fetches up to MAX_OPEN_PRS open + # PRs and filters locally — GitHub's REST endpoint doesn't support + # branch-name prefix filtering. + def list_open_prs(branch_prefix:) + prs = @client.pull_requests(@repo, state: "open", per_page: MAX_OPEN_PRS) + prs + .select { |pr| pr.head.ref.start_with?("#{branch_prefix}/") } + .map do |pr| + Reconciler::ExistingPR.new( + number: pr.number, + branch: pr.head.ref, + title: pr.title, + body: pr.body.to_s + ) + end + end + + # Creates a new PR. Branch must already exist on the remote. + # Returns the new PR's number. + def create_pr(branch:, base:, title:, body:, labels: []) + pr = @client.create_pull_request(@repo, base, branch, title, body) + @client.add_labels_to_an_issue(@repo, pr.number, labels) unless labels.empty? + pr.number + end + + # Ensures every label in +labels+ exists in the repo, creating any that + # are missing. Missing labels are created with a neutral default color; + # existing labels are left untouched. + def ensure_labels(labels) + return if labels.empty? + existing = list_label_names + labels.each do |label| + next if existing.include?(label) + @client.create_label(@repo, label, "0075ca") + end + end + + # Edits an existing PR's title and body. + def update_pr(number:, title:, body:) + @client.update_pull_request(@repo, number, title: title, body: body) + nil + end + + # Closes a PR, optionally leaving a comment explaining why. + def close_pr(number:, comment: nil) + if comment && !comment.empty? + @client.add_comment(@repo, number, comment) + end + @client.close_pull_request(@repo, number) + nil + end + + private + + def list_label_names + @client.labels(@repo).map(&:name) + rescue Octokit::Error + [] + end + end + end +end diff --git a/test/executor_test.rb b/test/executor_test.rb index 81e59ad..fca9e38 100644 --- a/test/executor_test.rb +++ b/test/executor_test.rb @@ -13,7 +13,7 @@ class ExecutorTest < Minitest::Test # ---- fakes ---- - # Spy GhClient that records calls and lets tests configure the PR number + # Spy GitHubClient that records calls and lets tests configure the PR number # returned by create_pr. class FakeGh attr_reader :created, :updated, :closed diff --git a/test/gh_client_test.rb b/test/gh_client_test.rb deleted file mode 100644 index 1f45351..0000000 --- a/test/gh_client_test.rb +++ /dev/null @@ -1,277 +0,0 @@ -# frozen_string_literal: true - -require_relative "test_helper" -require "gh_client" -require "commands" - -class GhClientTest < Minitest::Test - GhClient = Importmap::Update::GhClient - Commands = Importmap::Update::Commands - - REPO = "example-org/example-repo" - - def setup - @runner = Commands::FixtureRunner.new - @client = GhClient.new(repo: REPO, runner: @runner) - end - - # ---- list_open_prs ---- - - def test_list_open_prs_invokes_gh_with_expected_arguments - @runner.add( - pattern: [ - "gh", "pr", "list", "--repo", REPO, - "--state", "open", - "--search", "head:importmap-updates/", - "--limit", "100", - "--json", "number,headRefName,title,body" - ], - stdout: +"[]" - ) - @client.list_open_prs(branch_prefix: "importmap-updates") - assert_equal 1, @runner.calls.size - end - - def test_list_open_prs_parses_a_realistic_response - @runner.add( - pattern: [ - "gh", "pr", "list", "--repo", REPO, - "--state", "open", - "--search", "head:importmap-updates/", - "--limit", "100", - "--json", "number,headRefName,title,body" - ], - stdout: fixture("gh_pr_list_mixed.json") - ) - - prs = @client.list_open_prs(branch_prefix: "importmap-updates") - - assert_equal 3, prs.size - assert_equal 100, prs[0].number - assert_equal "importmap-updates/security-lodash", prs[0].branch - assert_includes prs[0].body, "importmap-update:metadata" - - # The hand-written foreign PR comes through too — filtering it as - # "foreign" is the reconciler's job, not the client's. - assert_equal 102, prs[2].number - refute_includes prs[2].body, "importmap-update:metadata" - end - - def test_list_open_prs_filters_out_branches_that_dont_actually_start_with_the_prefix - # GitHub's search syntax for `head:` is a prefix match, but it's a - # *search*, not a strict filter — partial matches can leak through. - # Belt and suspenders: re-filter client-side. - @runner.add( - pattern: [ - "gh", "pr", "list", "--repo", REPO, - "--state", "open", - "--search", "head:importmap-updates/", - "--limit", "100", - "--json", "number,headRefName,title,body" - ], - stdout: +<<~JSON - [ - { "number": 1, "headRefName": "importmap-updates/patch", "title": "ours", "body": "" }, - { "number": 2, "headRefName": "importmap-updates-related-feature", "title": "leak", "body": "" } - ] - JSON - ) - prs = @client.list_open_prs(branch_prefix: "importmap-updates") - assert_equal [1], prs.map(&:number) - end - - def test_list_open_prs_raises_on_invalid_json - @runner.add( - pattern: [ - "gh", "pr", "list", "--repo", REPO, - "--state", "open", - "--search", "head:importmap-updates/", - "--limit", "100", - "--json", "number,headRefName,title,body" - ], - stdout: +"not actually json" - ) - err = assert_raises(Commands::CommandError) do - @client.list_open_prs(branch_prefix: "importmap-updates") - end - assert_includes err.message, "Invalid JSON from gh" - end - - def test_list_open_prs_propagates_gh_failure - @runner.add( - pattern: [ - "gh", "pr", "list", "--repo", REPO, - "--state", "open", - "--search", "head:importmap-updates/", - "--limit", "100", - "--json", "number,headRefName,title,body" - ], - stderr: "auth required", - exit_code: 1 - ) - err = assert_raises(Commands::CommandError) do - @client.list_open_prs(branch_prefix: "importmap-updates") - end - assert_includes err.message, "auth required" - end - - # ---- ensure_labels ---- - - def test_ensure_labels_is_a_no_op_when_labels_is_empty - @client.ensure_labels([]) - assert_equal 0, @runner.calls.size - end - - def test_ensure_labels_creates_missing_labels - @runner.add( - pattern: ["gh", "label", "list", "--repo", REPO, "--json", "name", "--limit", "200"], - stdout: +%([{"name":"dependencies"}]) - ) - @runner.add( - pattern: ["gh", "label", "create", "javascript", "--repo", REPO, "--color", "0075ca"], - stdout: +"" - ) - @client.ensure_labels(%w[dependencies javascript]) - assert_equal 2, @runner.calls.size - end - - def test_ensure_labels_skips_labels_that_already_exist - @runner.add( - pattern: ["gh", "label", "list", "--repo", REPO, "--json", "name", "--limit", "200"], - stdout: +%([{"name":"dependencies"},{"name":"javascript"}]) - ) - @client.ensure_labels(%w[dependencies javascript]) - assert_equal 1, @runner.calls.size - end - - def test_ensure_labels_tolerates_gh_label_list_failure - @runner.add( - pattern: ["gh", "label", "list", "--repo", REPO, "--json", "name", "--limit", "200"], - stderr: "not found", - exit_code: 1 - ) - @runner.add( - pattern: ["gh", "label", "create", "dependencies", "--repo", REPO, "--color", "0075ca"], - stdout: +"" - ) - @client.ensure_labels(%w[dependencies]) - assert_equal 2, @runner.calls.size - end - - # ---- create_pr ---- - - def test_create_pr_invokes_gh_with_title_body_branch_and_base - @runner.add( - pattern: [ - "gh", "pr", "create", - "--repo", REPO, - "--head", "importmap-updates/patch", - "--base", "main", - "--title", "chore(deps): patch updates", - "--body", "PR body text" - ], - stdout: fixture("gh_pr_create_success.txt") - ) - - number = @client.create_pr( - branch: "importmap-updates/patch", - base: "main", - title: "chore(deps): patch updates", - body: "PR body text" - ) - assert_equal 123, number - end - - def test_create_pr_passes_labels_individually - # `gh pr create` accepts repeated --label flags; we want each label - # on its own --label so values containing commas don't get split. - @runner.add( - pattern: [ - "gh", "pr", "create", - "--repo", REPO, - "--head", "importmap-updates/patch", - "--base", "main", - "--title", "t", - "--body", "b", - "--label", "dependencies", - "--label", "javascript" - ], - stdout: "https://github.com/example-org/example-repo/pull/200\n" - ) - number = @client.create_pr( - branch: "importmap-updates/patch", - base: "main", - title: "t", - body: "b", - labels: %w[dependencies javascript] - ) - assert_equal 200, number - end - - def test_create_pr_propagates_branch_taken_failure_with_clear_message - @runner.add( - pattern: [ - "gh", "pr", "create", - "--repo", REPO, - "--head", "importmap-updates/patch", - "--base", "main", - "--title", "t", - "--body", "b" - ], - stderr: fixture("gh_pr_create_branch_taken.txt"), - exit_code: 1 - ) - err = assert_raises(Commands::CommandError) do - @client.create_pr(branch: "importmap-updates/patch", base: "main", title: "t", body: "b") - end - assert_includes err.message, "already exists" - end - - # ---- update_pr ---- - - def test_update_pr_edits_title_and_body - @runner.add( - pattern: [ - "gh", "pr", "edit", "42", - "--repo", REPO, - "--title", "new title", - "--body", "new body" - ], - stdout: "" - ) - assert_nil @client.update_pr(number: 42, title: "new title", body: "new body") - end - - # ---- close_pr ---- - - def test_close_pr_leaves_comment_then_closes_when_comment_is_given - @runner.add( - pattern: ["gh", "pr", "comment", "99", "--repo", REPO, "--body", "no longer needed"], - stdout: "" - ) - @runner.add( - pattern: ["gh", "pr", "close", "99", "--repo", REPO], - stdout: "" - ) - @client.close_pr(number: 99, comment: "no longer needed") - assert_equal 2, @runner.calls.size - # Comment must come before close so reviewers see the explanation - # alongside the closed status. - assert_equal "comment", @runner.calls[0][2] - assert_equal "close", @runner.calls[1][2] - end - - def test_close_pr_skips_comment_step_when_no_comment_provided - @runner.add(pattern: ["gh", "pr", "close", "99", "--repo", REPO], stdout: "") - @client.close_pr(number: 99) - assert_equal 1, @runner.calls.size - assert_equal "close", @runner.calls[0][2] - end - - def test_close_pr_skips_comment_step_when_comment_is_empty - # Defensive: a caller passing comment: "" shouldn't post an empty comment. - @runner.add(pattern: ["gh", "pr", "close", "99", "--repo", REPO], stdout: "") - @client.close_pr(number: 99, comment: "") - assert_equal 1, @runner.calls.size - end -end diff --git a/test/github_client_test.rb b/test/github_client_test.rb new file mode 100644 index 0000000..4135f1d --- /dev/null +++ b/test/github_client_test.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +require_relative "test_helper" +require "minitest/mock" +require "github_client" + +class GitHubClientTest < Minitest::Test + GitHubClient = Importmap::Update::GitHubClient + Reconciler = Importmap::Update::Reconciler + + REPO = "example-org/example-repo" + + def setup + @octokit = Minitest::Mock.new + @client = GitHubClient.new(repo: REPO, token: "unused-in-tests", client: @octokit) + end + + def teardown + assert_mock @octokit + end + + # ---- list_open_prs ---- + + def test_list_open_prs_returns_prs_matching_prefix + @octokit.expect(:pull_requests, [ + pr_stub(number: 100, ref: "importmap-updates/security-lodash", title: "Security", body: ""), + pr_stub(number: 101, ref: "importmap-updates/patch", title: "Patch", body: ""), + pr_stub(number: 102, ref: "other-branch", title: "Unrelated", body: "") + ], [REPO], state: "open", per_page: 100) + + prs = @client.list_open_prs(branch_prefix: "importmap-updates") + + assert_equal 2, prs.size + assert_equal 100, prs[0].number + assert_equal "importmap-updates/security-lodash", prs[0].branch + assert_includes prs[0].body, "importmap-update:metadata" + assert_equal 101, prs[1].number + end + + def test_list_open_prs_filters_out_branches_without_exact_prefix_slash + # A branch named "importmap-updates-related-feature" must not leak through. + @octokit.expect(:pull_requests, [ + pr_stub(number: 1, ref: "importmap-updates/patch", title: "ours", body: ""), + pr_stub(number: 2, ref: "importmap-updates-related-feature", title: "leak", body: "") + ], [REPO], state: "open", per_page: 100) + + prs = @client.list_open_prs(branch_prefix: "importmap-updates") + assert_equal [1], prs.map(&:number) + end + + def test_list_open_prs_returns_empty_when_no_open_prs + @octokit.expect(:pull_requests, [], [REPO], state: "open", per_page: 100) + assert_equal [], @client.list_open_prs(branch_prefix: "importmap-updates") + end + + # ---- create_pr ---- + + def test_create_pr_returns_pr_number + @octokit.expect(:create_pull_request, + pr_stub(number: 123), + [REPO, "main", "importmap-updates/patch", "chore(deps): patch updates", "PR body text"]) + number = @client.create_pr(branch: "importmap-updates/patch", base: "main", title: "chore(deps): patch updates", body: "PR body text") + assert_equal 123, number + end + + def test_create_pr_adds_labels_when_given + @octokit.expect(:create_pull_request, pr_stub(number: 200), + [REPO, "main", "importmap-updates/patch", "t", "b"]) + @octokit.expect(:add_labels_to_an_issue, nil, + [REPO, 200, %w[dependencies javascript]]) + number = @client.create_pr(branch: "importmap-updates/patch", base: "main", title: "t", body: "b", labels: %w[dependencies javascript]) + assert_equal 200, number + end + + def test_create_pr_skips_label_call_when_no_labels + @octokit.expect(:create_pull_request, pr_stub(number: 42), + [REPO, "main", "importmap-updates/patch", "t", "b"]) + # No add_labels_to_an_issue call expected. + @client.create_pr(branch: "importmap-updates/patch", base: "main", title: "t", body: "b") + end + + # ---- ensure_labels ---- + + def test_ensure_labels_is_a_no_op_when_labels_is_empty + # No octokit calls expected. + @client.ensure_labels([]) + end + + def test_ensure_labels_creates_missing_labels + @octokit.expect(:labels, [label_stub("dependencies")], [REPO]) + @octokit.expect(:create_label, nil, [REPO, "javascript", "0075ca"]) + @client.ensure_labels(%w[dependencies javascript]) + end + + def test_ensure_labels_skips_labels_that_already_exist + @octokit.expect(:labels, [label_stub("dependencies"), label_stub("javascript")], [REPO]) + # No create_label calls expected. + @client.ensure_labels(%w[dependencies javascript]) + end + + def test_ensure_labels_tolerates_labels_list_failure + @octokit.expect(:labels, nil) { |_repo| raise Octokit::Error } + @octokit.expect(:create_label, nil, [REPO, "dependencies", "0075ca"]) + @client.ensure_labels(%w[dependencies]) + end + + # ---- update_pr ---- + + def test_update_pr_edits_title_and_body + @octokit.expect(:update_pull_request, nil, [REPO, 42], title: "new title", body: "new body") + assert_nil @client.update_pr(number: 42, title: "new title", body: "new body") + end + + # ---- close_pr ---- + + def test_close_pr_leaves_comment_then_closes_when_comment_is_given + order = [] + @octokit.expect(:add_comment, nil) do |repo, number, comment| + order << :comment + repo == REPO && number == 99 && comment == "no longer needed" + end + @octokit.expect(:close_pull_request, nil) do |repo, number| + order << :close + repo == REPO && number == 99 + end + @client.close_pr(number: 99, comment: "no longer needed") + assert_equal [:comment, :close], order + end + + def test_close_pr_skips_comment_step_when_no_comment_provided + @octokit.expect(:close_pull_request, nil, [REPO, 99]) + @client.close_pr(number: 99) + end + + def test_close_pr_skips_comment_step_when_comment_is_empty + @octokit.expect(:close_pull_request, nil, [REPO, 99]) + @client.close_pr(number: 99, comment: "") + end + + private + + def pr_stub(number:, ref: "importmap-updates/patch", title: "PR title", body: "") + head = Struct.new(:ref).new(ref) + Struct.new(:number, :head, :title, :body).new(number, head, title, body) + end + + def label_stub(name) + Struct.new(:name).new(name) + end +end From d8420a0525e5a70663e373b90b1aeb2cf053c830 Mon Sep 17 00:00:00 2001 From: Neil Carvalho Date: Mon, 18 May 2026 09:53:20 -0300 Subject: [PATCH 2/3] Run action in its own Ruby environment This action has its own Ruby dependencies that may or may not be included in the consumer Gemfile. To avoid colliding dependencies, we should run within the same Ruby setup as the consumer application, but run our own `bundle install`, running anything that redirects to the consumer application using `Bundler.with_unbundled_env` to opt out of our Gemfile. --- .github/workflows/integration.yml | 2 +- action.yml | 16 +++++++--------- exe/importmap-update | 2 +- lib/commands.rb | 6 ++++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 802f93a..83b68fa 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -54,7 +54,7 @@ jobs: with: github-token: ${{ secrets.GITHUB_TOKEN }} dry-run: "true" - working-directory: /tmp/test-app + rails-root: /tmp/test-app - name: Assert all outdated packages appear in dry-run output run: | diff --git a/action.yml b/action.yml index 65f46ff..7c9498a 100644 --- a/action.yml +++ b/action.yml @@ -32,7 +32,7 @@ inputs: description: "Git author email for commits." required: false default: "github-actions[bot]@users.noreply.github.com" - working-directory: + rails-root: description: "Directory containing the Rails app to run against. Useful when the app lives in a subdirectory or for integration testing." required: false default: "." @@ -40,16 +40,12 @@ inputs: runs: using: "composite" steps: - - name: Set up Ruby - uses: ruby/setup-ruby@v1 - with: - bundler-cache: true - ruby-version: ${{ inputs.ruby-version }} - - name: Run importmap-update shell: bash - working-directory: ${{ inputs.working-directory }} + working-directory: ${{ github.action_path }} env: + BUNDLE_GEMFILE: ${{ github.action_path }}/Gemfile + RAILS_ROOT: ${{ inputs.rails-root }} GITHUB_TOKEN: ${{ inputs.github-token }} GITHUB_REPOSITORY: ${{ github.repository }} INPUT_CONFIG_FILE: ${{ inputs.config-file }} @@ -57,4 +53,6 @@ runs: IMPORTMAP_DRY_RUN: ${{ inputs.dry-run }} IMPORTMAP_AUTHOR_NAME: ${{ inputs.author-name }} IMPORTMAP_AUTHOR_EMAIL: ${{ inputs.author-email }} - run: ${{ github.action_path }}/exe/importmap-update 2>&1 | tee "${IMPORTMAP_RUN_LOG:-/dev/null}" + run: | + bundle install + bundle exec exe/importmap-update 2>&1 | tee "${IMPORTMAP_RUN_LOG:-/dev/null}" diff --git a/exe/importmap-update b/exe/importmap-update index 9eef128..00d0bf4 100755 --- a/exe/importmap-update +++ b/exe/importmap-update @@ -162,7 +162,7 @@ when :run exit 2 end - runner = Importmap::Update::Commands::ShellRunner.new + runner = Importmap::Update::Commands::ShellRunner.new(cwd: ENV.fetch("RAILS_ROOT", ".")) gh = Importmap::Update::GitHubClient.new(repo: repo, token: token) git = Importmap::Update::GitClient.new( runner: runner, diff --git a/lib/commands.rb b/lib/commands.rb index ec06b45..c33534d 100644 --- a/lib/commands.rb +++ b/lib/commands.rb @@ -47,8 +47,10 @@ def initialize(cwd: nil, env: nil) def run(*argv) opts = {} opts[:chdir] = @cwd if @cwd - stdout, stderr, status = Open3.capture3(@env, *argv, **opts) - Result.new(stdout: stdout, stderr: stderr, exit_code: status.exitstatus) + Bundler.with_unbundled_env do + stdout, stderr, status = Open3.capture3(@env, *argv, opts) + Result.new(stdout: stdout, stderr: stderr, exit_code: status.exitstatus) + end end # Raises on non-zero exit. Use when you have no recovery strategy From 5af7f12368d54ad50feab298518e1f333741032c Mon Sep 17 00:00:00 2001 From: Neil Carvalho Date: Mon, 25 May 2026 10:26:01 -0300 Subject: [PATCH 3/3] Include Ruby setup in the README --- .tool-versions | 2 +- README.md | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.tool-versions b/.tool-versions index 6e03b21..aac7389 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1 +1 @@ -ruby 4.0.1 +ruby 4.0.5 diff --git a/README.md b/README.md index 5b55d1c..d0dcba7 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ Another option is to use a dedicated GitHub token for this action, which can be set as a secret in the repository's settings. This token should have the `repo` scope enabled. -Then, add a workflow file: +Then, add a workflow file that sets up your Rails environment and runs the action: ```yaml # .github/workflows/importmap-updates.yml @@ -38,7 +38,11 @@ jobs: update: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 + - uses: ruby/setup-ruby@v1 + with: + bundler-cache: true + - uses: thoughtbot/importmap-update@v1 with: github-token: ${{ secrets.GITHUB_TOKEN }}