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

Handle missing Merge-Requested-By user #861

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
2 participants
@DazWorrall
Copy link
Member

DazWorrall commented Jan 9, 2019

Ref: Shopify/shipit#567

Stack trace

/artifacts/ruby/2.5.0/gems/octokit-4.12.0/lib/octokit/response/raise_error.rb:16:in `on_complete'
/artifacts/ruby/2.5.0/gems/faraday-0.12.2/lib/faraday/response.rb:9:in `block in call'
/artifacts/ruby/2.5.0/gems/faraday-0.12.2/lib/faraday/response.rb:61:in `on_complete'
/artifacts/ruby/2.5.0/gems/faraday-0.12.2/lib/faraday/response.rb:8:in `call'
/artifacts/ruby/2.5.0/bundler/gems/shipit-engine-070da14b4f44/lib/shipit/strip_cache_control.rb:4:in `call'
/artifacts/ruby/2.5.0/gems/faraday-http-cache-1.2.2/lib/faraday/http_cache.rb:303:in `fetch'
/artifacts/ruby/2.5.0/gems/faraday-http-cache-1.2.2/lib/faraday/http_cache.rb:208:in `process'
/artifacts/ruby/2.5.0/gems/faraday-http-cache-1.2.2/lib/faraday/http_cache.rb:146:in `call!'
/artifacts/ruby/2.5.0/gems/faraday-http-cache-1.2.2/lib/faraday/http_cache.rb:119:in `call'
/artifacts/ruby/2.5.0/gems/faraday-0.12.2/lib/faraday/rack_builder.rb:141:in `build_response'
/artifacts/ruby/2.5.0/gems/faraday-0.12.2/lib/faraday/connection.rb:386:in `run_request'
/artifacts/ruby/2.5.0/gems/faraday-0.12.2/lib/faraday/connection.rb:149:in `get'
/artifacts/ruby/2.5.0/gems/sawyer-0.8.1/lib/sawyer/agent.rb:94:in `call'
/artifacts/ruby/2.5.0/gems/octokit-4.12.0/lib/octokit/connection.rb:156:in `request'
/artifacts/ruby/2.5.0/gems/octokit-4.12.0/lib/octokit/connection.rb:19:in `get'
/artifacts/ruby/2.5.0/gems/octokit-4.12.0/lib/octokit/client/users.rb:34:in `user'
/artifacts/ruby/2.5.0/bundler/gems/shipit-engine-070da14b4f44/app/models/shipit/user.rb:17:in `block in find_or_create_by_login!'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/core.rb:316:in `initialize'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/inheritance.rb:66:in `new'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/inheritance.rb:66:in `new'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/persistence.rb:52:in `create!'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/relation.rb:99:in `block in create!'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/relation.rb:281:in `scoping'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/relation.rb:99:in `create!'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/relation.rb:171:in `find_or_create_by!'
/artifacts/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/querying.rb:8:in `find_or_create_by!'
/artifacts/ruby/2.5.0/bundler/gems/shipit-engine-070da14b4f44/app/models/shipit/user.rb:16:in `find_or_create_by_login!'
/artifacts/ruby/2.5.0/bundler/gems/shipit-engine-070da14b4f44/app/models/shipit/user.rb:27:in `find_or_create_author_from_github_commit'
/artifacts/ruby/2.5.0/bundler/gems/shipit-engine-070da14b4f44/app/models/shipit/commit.rb:78:in `from_github'
/artifacts/ruby/2.5.0/bundler/gems/shipit-engine-070da14b4f44/app/models/shipit/commit.rb:101:in `create_from_github!'

If the Merge-Requested-By user is renamed or deleted, this will cause an unhandled Octokit::NotFound, which will break commit fetching/syncing.

I chose not to squash this in find_or_create_by_login! as returning nil from a function like that would seem pretty unexpected, so this change explicitly handles the NotFound in the caller.

@DazWorrall DazWorrall requested a review from Shopify/pipeline Jan 9, 2019

@casperisfine
Copy link
Contributor

casperisfine left a comment

👍

But rubocop complaints are legit.

@DazWorrall DazWorrall force-pushed the missing-merge-requested-by branch from d6ff682 to 7739659 Jan 9, 2019

@DazWorrall DazWorrall merged commit 50d7169 into master Jan 9, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@DazWorrall DazWorrall deleted the missing-merge-requested-by branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment