Skip to content
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

Use an Octokit client without tokens when support_tokenless_auth is true #1385

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

makimoto
Copy link

Currently, if support_tokenless_auth is true (mainly, invoked with danger local), the client method returns nil and results in NoMethodError.

% bundle exec danger local
Running your Dangerfile against this PR - https://github.com/danger/danger/pull/1384
Turning on --verbose

bundler: failed to load command: danger (/Users/makimoto/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/bin/danger)
NoMethodError: undefined method `pull_request' for nil:NilClass
  /Users/makimoto/.ghq/github.com/danger/danger/lib/danger/request_sources/github/github.rb:117:in `fetch_details'
  /Users/makimoto/.ghq/github.com/danger/danger/lib/danger/commands/local_helpers/local_setup.rb:37:in `setup'
  /Users/makimoto/.ghq/github.com/danger/danger/lib/danger/commands/local.rb:55:in `run'
  /Users/makimoto/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/claide-1.1.0/lib/claide/command.rb:334:in `run'
  /Users/makimoto/.ghq/github.com/danger/danger/bin/danger:5:in `<top (required)>'
  /Users/makimoto/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/bin/danger:23:in `load'
  /Users/makimoto/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/bin/danger:23:in `<top (required)>'

Therefore, in this case, the client method should return a client without tokens to access resources of public repositories.

Definitely, the patch should include a test for the case, but I couldn't see where I should add it. Could you please advise me about it?
Thank you.

Currently, if support_tokenless_auth is true (mainly, invoked with
`danger local`), the client method returns nil and results in
NoMethodError.
Therefore, in this case, the client method should return a client
without tokens to access resources of public repositories.
@manicmaniac
Copy link
Member

@makimoto

Definitely, the patch should include a test for the case, but I couldn't see where I should add it. Could you please advise me about it?

The only spec that tests Danger::RequestSources::GitHub#client seems here.

describe "bearer token" do
context "with DANGER_GITHUB_BEARER_TOKEN" do
let(:client) { double('client') }
it 'creates Octokit client with bearer token' do
allow(Octokit::Client).to receive(:new).and_return(client)
gh_env = { "DANGER_GITHUB_BEARER_TOKEN" => "hi" }
expect(Octokit::Client).to receive(:new).with(hash_including(:bearer_token))
Danger::RequestSources::GitHub.new(stub_ci, gh_env).client
end
end
end

How about adding a new test case around there?

@manicmaniac
Copy link
Member

📝 Although danger local is deprecated in favor of danger pr, danger pr hasn't support tokenless auth yet.

@manicmaniac manicmaniac self-requested a review November 12, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants