From 9e37518dd1fbbf5e59eeb89eb145627a64c21995 Mon Sep 17 00:00:00 2001 From: Flipez Date: Tue, 30 Jul 2019 19:34:45 +0200 Subject: [PATCH 1/6] Please the linter Signed-off-by: Flipez --- config/initializers/array.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/array.rb b/config/initializers/array.rb index c81d1511..3ada4bc5 100644 --- a/config/initializers/array.rb +++ b/config/initializers/array.rb @@ -1,5 +1,5 @@ class Array def all_i - self.map(&:to_i) + map(&:to_i) end end From a388c816b45eb0fc64f6ed588b360d665fb8959e Mon Sep 17 00:00:00 2001 From: Flipez Date: Tue, 30 Jul 2019 20:31:31 +0200 Subject: [PATCH 2/6] Add pull request event handling Signed-off-by: Flipez --- .rubocop_todo.yml | 15 ++-- app/controllers/incoming_controller.rb | 7 +- app/lib/github_event.rb | 19 ++++ app/lib/github_event/base.rb | 13 +++ app/lib/github_event/pull_request.rb | 13 +++ app/models/label.rb | 5 ++ app/models/pull_request.rb | 87 ++++++++++++++++++- app/models/repository.rb | 43 +++++---- app/workers/validate_pull_request_worker.rb | 12 +++ ...rgeable_and_gh_repo_id_to_pull_requests.rb | 7 ++ db/schema.rb | 5 +- 11 files changed, 198 insertions(+), 28 deletions(-) create mode 100644 app/lib/github_event.rb create mode 100644 app/lib/github_event/base.rb create mode 100644 app/lib/github_event/pull_request.rb create mode 100644 app/workers/validate_pull_request_worker.rb create mode 100644 db/migrate/20190730094710_add_mergeable_and_gh_repo_id_to_pull_requests.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 0f7981e8..d2cf8493 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2019-07-20 16:07:34 +0200 using RuboCop version 0.73.0. +# on 2019-07-30 20:31:00 +0200 using RuboCop version 0.73.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -118,7 +118,7 @@ Layout/Tab: Exclude: - 'config/initializers/omniauth.rb' -# Offense count: 6 +# Offense count: 5 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle. # SupportedStyles: final_newline, final_blank_line @@ -129,7 +129,6 @@ Layout/TrailingBlankLines: - 'app/controllers/sessions_controller.rb' - 'app/models/user.rb' - 'config/initializers/sidekiq.rb' - - 'config/initializers/voxpupuli.rb' # Offense count: 2 # Cop supports --auto-correct. @@ -140,7 +139,7 @@ Layout/TrailingWhitespace: # Offense count: 6 Metrics/AbcSize: - Max: 111 + Max: 118 # Offense count: 1 Metrics/CyclomaticComplexity: @@ -253,11 +252,11 @@ Style/CommentAnnotation: Exclude: - 'app/workers/repo_status_worker.rb' -# Offense count: 18 +# Offense count: 23 Style/Documentation: Enabled: false -# Offense count: 59 +# Offense count: 64 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle. # SupportedStyles: always, never @@ -333,7 +332,7 @@ Style/TrailingCommaInArguments: Exclude: - 'config/initializers/semantic_logger.rb' -# Offense count: 3 +# Offense count: 1 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle, MinSize, WordRegex. # SupportedStyles: percent, brackets @@ -341,7 +340,7 @@ Style/WordArray: Exclude: - 'config/initializers/voxpupuli.rb' -# Offense count: 58 +# Offense count: 69 # Cop supports --auto-correct. # Configuration parameters: AutoCorrect, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. # URISchemes: http, https diff --git a/app/controllers/incoming_controller.rb b/app/controllers/incoming_controller.rb index b14d1f22..e41e3882 100644 --- a/app/controllers/incoming_controller.rb +++ b/app/controllers/incoming_controller.rb @@ -2,10 +2,15 @@ class IncomingController < ApplicationController skip_before_action :verify_authenticity_token skip_before_action :logged_in + ## + # Parse the whole payload from Github and hand it over to the GithubEvent handler def github - # parse the JSON payload, that we get as string, to a hash useable_body = JSON.parse(request.body.read).to_h + # For debug purposes we currently log the requets into Sentry + # TODO: Remove this line since the GithubEvent handler reports unknown events. Raven.capture_message('Received hook', extra: useable_body) + + GithubEvent.new(useable_body, request.headers['X-GitHub-Event']) end end diff --git a/app/lib/github_event.rb b/app/lib/github_event.rb new file mode 100644 index 00000000..0dbcf801 --- /dev/null +++ b/app/lib/github_event.rb @@ -0,0 +1,19 @@ +class GithubEvent + attr_reader :processor + delegate :process, to: :processor + + ## + # The GithubEvent handler checks which type of event we are dealing with. + # + # If it is a known event, create a specific handler and let it take care of + # the next steps. If not kick off a Sentry error. + + def initialize(payload, type) + case type + when 'pull_request' + @processor = GithubEvent::PullRequest.new(payload) + else + Raven.capture_message('Unknown Hook Received', extra: useable_body) + end + end +end diff --git a/app/lib/github_event/base.rb b/app/lib/github_event/base.rb new file mode 100644 index 00000000..6cc62608 --- /dev/null +++ b/app/lib/github_event/base.rb @@ -0,0 +1,13 @@ +class GithubEvent + class Base + attr_reader :payload + + ## + # Receive the payload and start processing it + + def initialize(payload) + @payload = payload + process + end + end +end diff --git a/app/lib/github_event/pull_request.rb b/app/lib/github_event/pull_request.rb new file mode 100644 index 00000000..07349621 --- /dev/null +++ b/app/lib/github_event/pull_request.rb @@ -0,0 +1,13 @@ +class GithubEvent + class PullRequest < GithubEvent::Base + attr_reader :gh_pull_request + + ## + # Currently we only care about syncing our database with GitHub as our validation + # will take action if necessary. + + def process + ::PullRequest.update_with_github(payload['pull_request']) + end + end +end diff --git a/app/models/label.rb b/app/models/label.rb index ebb81bad..476b41bf 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -1,3 +1,8 @@ class Label < ApplicationRecord has_and_belongs_to_many :pull_requests + + # TODO: Find out how to manage predefined labels + def self.needs_rebase + find_or_create_by(name: 'needs-rebase', color: '207de5') + end end diff --git a/app/models/pull_request.rb b/app/models/pull_request.rb index 546d1f89..8d07569c 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -1,4 +1,89 @@ class PullRequest < ApplicationRecord - belongs_to :repository + # It is easier overall to use the GitHub ID for relation management. + # It allows us to maintain, update or the Repository or PullRequest without + # the counterpart. + belongs_to :repository, primary_key: :github_id, foreign_key: :gh_repository_id, inverse_of: :pull_requests + has_and_belongs_to_many :labels + after_save :queue_validation + + ## + # This method is used for updating our database with the current state of the PullRequest + # in GitHub. Therefore it's used with the payload of the webhook content processed by + # GithubEvent handler and by the content from the api fetched by the periodic check. + + def self.update_with_github(gh_pull_request) + PullRequest.where(github_id: gh_pull_request['id']).first_or_initialize.tap do |pull_request| + pull_request.number = gh_pull_request['number'] + pull_request.state = gh_pull_request['state'] + pull_request.title = gh_pull_request['title'] + pull_request.body = gh_pull_request['body'] + pull_request.gh_created_at = gh_pull_request['created_at'] + pull_request.gh_updated_at = gh_pull_request['updated_at'] + pull_request.gh_repository_id = gh_pull_request['base']['repo']['id'] + pull_request.closed_at = gh_pull_request['closed_at'] + pull_request.merged_at = gh_pull_request['merged_at'] + pull_request.mergeable = gh_pull_request['mergeable'] + pull_request.save + + gh_pull_request['labels'].each do |label| + pull_request.labels << Label.find_or_create_by(name: label['name'], color: label['color']) + end + end + end + + ## + # Ensure that the Label is attached to the PullRequest + # + # Therefore ensure that the Label exists for the corresponding repository + # + # Then ensure the Label is attached to this PullRequest by checking the list + # of attached Labels. Unfortunately this seems to be the only option + # + # If the list does not include the given Label we attach it + + def ensure_label_is_attached(label) + repository.ensure_label_exists(label) + + attached_labels = Github.client.labels_for_issue(gh_repository_id, number) + return if attached_labels.any? { |attached_label| attached_label['name'] == label.name } + + Github.client.add_labels_to_an_issue(gh_repository_id, number, [label.name]) + end + + ## + # We simply remove the given Label if it exists + + def ensure_label_is_detached(label) + Github.client.remove_label(gh_repository_id, number, label.name) + end + + ## + # if the PullRequest is mergeable we need to check if the 'needs-rebase' Label + # is attached. If so, we need to remove it. + # + # If the PullRequest is not yet mergeable we need to attach the 'needs-rebase' + # Label. Therefore we also need to check if the Label exists on repository level. + # + # The saved_changes variable includes all the stuff that has changed. + # We currently don't care about them + + def validate(_saved_changes) + if mergeable + ensure_label_is_detached(Label.needs_rebase) + else + repository.ensure_label_exists(Label.needs_rebase) + ensure_label_is_attached(label) + end + end + + private + + ## + # Since we want to be a fancy responsive application we to all the validation + # stuff which might result in some new querys and api requests asyncronously. + + def queue_validation + ValidatePullRequestWorker.perform_async(id, saved_changes) if saved_changes? + end end diff --git a/app/models/repository.rb b/app/models/repository.rb index 7dafbc1a..9a28ce07 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1,6 +1,9 @@ class Repository < ApplicationRecord - has_many :pull_requests - has_many :open_pull_requests, -> { where(state: 'open') }, class_name: 'PullRequest' + # It is easier overall to use the GitHub ID for relation management. + # It allows us to maintain, update or the Repository or PullRequest without + # the counterpart. + has_many :pull_requests, primary_key: :github_id, foreign_key: :gh_repository_id, inverse_of: :repository + has_many :open_pull_requests, -> { where(state: 'open') }, class_name: 'PullRequest', primary_key: :github_id, foreign_key: :gh_repository_id def actions_needed @action_needed ||= begin @@ -19,25 +22,31 @@ def github_url 'https://github.com/' + full_name end + ## + # Check if the Label exists in the Repository + # If we get a 404 create the Label. + + def ensure_label_exists(label) + GitHub.client.label(github_id, label.name) + rescue Octokit::NotFound + Github.client.add_label(github_id, label.name, label.color) + end + + ## + # Delete the given Label from the Repository + # + def ensure_label_missing(label) + GitHub.client.delete_label!(github_id, label.name) + end + + ## + # Fetch all open and closed PullRequests and sync our database with them + # def update_pull_requests open_pull_requests = Github.client.pull_requests("voxpupuli/#{name}") closed_pull_requests = Github.client.pull_requests("voxpupuli/#{name}", state: :closed) (open_pull_requests + closed_pull_requests).each do |gh_pull_request| - PullRequest.where(repository_id: id, number: gh_pull_request.number).first_or_initialize.tap do |pull_request| - pull_request.number = gh_pull_request.number - pull_request.state = gh_pull_request.state - pull_request.title = gh_pull_request.title - pull_request.body = gh_pull_request.body - pull_request.gh_created_at = gh_pull_request.created_at - pull_request.gh_updated_at = gh_pull_request.updated_at - pull_request.closed_at = gh_pull_request.closed_at - pull_request.merged_at = gh_pull_request.merged_at - pull_request.save - - gh_pull_request.labels.each do |label| - pull_request.labels << Label.find_or_create_by(name: label.name, color: label.color) - end - end + PullRequest.update_with_github(gh_pull_request) end pull_requests.count diff --git a/app/workers/validate_pull_request_worker.rb b/app/workers/validate_pull_request_worker.rb new file mode 100644 index 00000000..a2ab2112 --- /dev/null +++ b/app/workers/validate_pull_request_worker.rb @@ -0,0 +1,12 @@ +class ValidatePullRequestWorker + include Sidekiq::Worker + + ## + # As validation may take a bit more time we do it async. + # This worker is not meant to hold any logic but only trigger the + # validation in its asyncronous context. + + def perform(id, saved_changes) + PullRequest.find(id).validate(saved_changes) + end +end diff --git a/db/migrate/20190730094710_add_mergeable_and_gh_repo_id_to_pull_requests.rb b/db/migrate/20190730094710_add_mergeable_and_gh_repo_id_to_pull_requests.rb new file mode 100644 index 00000000..82807c92 --- /dev/null +++ b/db/migrate/20190730094710_add_mergeable_and_gh_repo_id_to_pull_requests.rb @@ -0,0 +1,7 @@ +class AddMergeableAndGhRepoIdToPullRequests < ActiveRecord::Migration[5.2] + def change + add_column :pull_requests, :mergeable, :boolean + add_column :pull_requests, :gh_repository_id, :integer + add_column :pull_requests, :github_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index e6006242..db3f77e1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_06_20_115024) do +ActiveRecord::Schema.define(version: 2019_07_30_094710) do create_table "labels", force: :cascade do |t| t.string "name" @@ -38,6 +38,9 @@ t.integer "repository_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "mergeable" + t.integer "gh_repository_id" + t.integer "github_id" t.index ["repository_id"], name: "index_pull_requests_on_repository_id" end From 235caf12f3ace533a9c15f853d41e20cabf7ff5b Mon Sep 17 00:00:00 2001 From: Flipez Date: Tue, 30 Jul 2019 21:19:49 +0200 Subject: [PATCH 3/6] use correct variable --- app/lib/github_event.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/github_event.rb b/app/lib/github_event.rb index 0dbcf801..bfe2ec9b 100644 --- a/app/lib/github_event.rb +++ b/app/lib/github_event.rb @@ -13,7 +13,7 @@ def initialize(payload, type) when 'pull_request' @processor = GithubEvent::PullRequest.new(payload) else - Raven.capture_message('Unknown Hook Received', extra: useable_body) + Raven.capture_message('Unknown Hook Received', extra: payload) end end end From d29b8c5a6916f418ae8b32e8b352c1bcf7c1fca1 Mon Sep 17 00:00:00 2001 From: Flipez Date: Tue, 30 Jul 2019 21:28:39 +0200 Subject: [PATCH 4/6] please the linter --- app/workers/repo_status_worker.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/workers/repo_status_worker.rb b/app/workers/repo_status_worker.rb index e5fdffc7..739466e2 100644 --- a/app/workers/repo_status_worker.rb +++ b/app/workers/repo_status_worker.rb @@ -11,7 +11,6 @@ def save_data_to_redis(data) # Categorizes repository based on their locally cached information # implements https://github.com/voxpupuli/modulesync_config/blob/master/bin/get_all_the_diffs # TODO: clean up stuff and make it less shitty - # rubocop:disable Metrics/AbcSize def perform repos = Repository.pluck(:name) data = OpenStruct.new From d3ad05ffe912b1baa777a883c49ae0835acb4b93 Mon Sep 17 00:00:00 2001 From: Flipez Date: Tue, 30 Jul 2019 21:31:49 +0200 Subject: [PATCH 5/6] the linter wants to be pleased again --- app/workers/repo_status_worker.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/workers/repo_status_worker.rb b/app/workers/repo_status_worker.rb index 739466e2..a6801fed 100644 --- a/app/workers/repo_status_worker.rb +++ b/app/workers/repo_status_worker.rb @@ -143,5 +143,4 @@ def perform save_data_to_redis(data) end - # rubocop:enable Metrics/AbcSize end From fe6ad2999a386ebb21fbb39859a465389cff8057 Mon Sep 17 00:00:00 2001 From: Flipez Date: Tue, 30 Jul 2019 21:32:19 +0200 Subject: [PATCH 6/6] use rails; we're not in the 90s --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 81df2ec4..e6a84b39 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,7 +4,7 @@ dist: xenial language: ruby cache: bundler script: - - 'bundle exec rake $CHECK' + - 'bundle exec rails $CHECK' matrix: fast_finish: true include: