diff --git a/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter.rb b/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter.rb index be706734..baf741c4 100644 --- a/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter.rb +++ b/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter.rb @@ -7,7 +7,7 @@ class GithubPrCommenter include BranchWorkerMixin def self.batch_workers - [DiffContentChecker, DiffFilenameChecker] + [DiffContentChecker, DiffFilenameChecker, CommitMetadataChecker] end def self.handled_branch_modes diff --git a/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker.rb b/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker.rb new file mode 100644 index 00000000..5f2f0f17 --- /dev/null +++ b/app/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker.rb @@ -0,0 +1,62 @@ +require 'rugged' + +module CommitMonitorHandlers::CommitRange + class GithubPrCommenter::CommitMetadataChecker + include Sidekiq::Worker + sidekiq_options :queue => :miq_bot_glacial + + include BatchEntryWorkerMixin + include BranchWorkerMixin + + def perform(batch_entry_id, branch_id, new_commits) + return unless find_batch_entry(batch_entry_id) + return skip_batch_entry unless find_branch(branch_id, :pr) + + complete_batch_entry(:result => process_commits(new_commits)) + end + + private + + def process_commits(new_commits) + @offenses = [] + + new_commits.each do |commit_sha, data| + check_for_usernames_in(commit_sha, data["message"]) + end + + @offenses + end + + # From https://github.com/join + # + # "Username may only contain alphanumeric characters or single hyphens, + # and cannot begin or end with a hyphen." + # + # For the beginning and and, we do a positive lookbehind at the beginning + # to get the `@`, and a positive lookhead at the end to confirm their is + # either a period or a whitespace following the "var" (instance_variable) + # + # Since there can't be underscores in Github usernames, this makes it so we + # rule out partial matches of variables (@database_records having a + # username lookup of `database`), but still catch full variable names + # without underscores (`@foobarbaz`). + # + USERNAME_REGEXP = / + (?<=@) # must start with a '@' (don't capture) + [a-zA-Z0-9] # first character must be alphanumeric + [a-zA-Z0-9\-]* # middle chars may be alphanumeric or hyphens + [a-zA-Z0-9] # last character must be alphanumeric + (?=[\.\s]) # allow only variables without "_" (not captured) + /x.freeze + + def check_for_usernames_in(commit, message) + message.scan(USERNAME_REGEXP).each do |potential_username| + next unless GithubService.username_lookup(potential_username) + + group = ::Branch.github_commit_uri(fq_repo_name, commit) + message = "Username `@#{potential_username}` detected in commit message. Consider removing." + @offenses << OffenseMessage::Entry.new(:low, message, group) + end + end + end +end diff --git a/lib/github_service.rb b/lib/github_service.rb index f51dd7db..8e17597d 100644 --- a/lib/github_service.rb +++ b/lib/github_service.rb @@ -113,6 +113,21 @@ def refresh_assignees(fq_name) assignees_cache.delete(fq_name) end + def username_lookup(username) + if username_lookup_cache.key?(username) + username_lookup_cache[username] + else + username_lookup_cache[username] ||= begin + case Net::HTTP.new("github.com", 443).tap { |h| h.use_ssl = true }.request_head("/#{username}") + when Net::HTTPNotFound then nil # invalid username + when Net::HTTPOK then service.user(username)[:id] + else + raise "Error on GitHub with username lookup" + end + end + end + end + private def service @@ -151,6 +166,10 @@ def assignees_cache @assignees_cache ||= {} end + def username_lookup_cache + @username_lookup_cache ||= {} + end + def respond_to_missing?(method_name, include_private = false) service.respond_to?(method_name, include_private) end diff --git a/spec/lib/github_service_spec.rb b/spec/lib/github_service_spec.rb new file mode 100644 index 00000000..efac6e8a --- /dev/null +++ b/spec/lib/github_service_spec.rb @@ -0,0 +1,63 @@ +describe GithubService do + describe "#username_lookup" do + let(:lookup_username) { "NickLaMuro" } + let(:lookup_status) { 200 } + + before do + # HTTP lookup + stub_request(:head, "https://github.com/#{lookup_username}") + .with(:headers => {'Accept' => '*/*', 'User-Agent' => 'Ruby'}) + .to_return(:status => lookup_status, :body => "", :headers => {}) + end + + after do + lookup_cache.delete(lookup_username) + end + + def lookup_cache + described_class.send(:username_lookup_cache) + end + + context "for a valid user" do + before do + github_service_add_stub :url => "/users/#{lookup_username}", + :response_body => {'id' => 123}.to_json + end + + it "looks up a user and stores the user's ID in the cache" do + expect(described_class.username_lookup(lookup_username)).to eq(123) + expect(lookup_cache).to eq("NickLaMuro" => 123) + end + end + + context "for a user that is not found" do + let(:lookup_status) { 404 } + + it "looks up a user and stores the user's ID in the cache" do + expect(described_class.username_lookup(lookup_username)).to eq(nil) + expect(lookup_cache).to eq("NickLaMuro" => nil) + end + + it "does a lookup call only once" do + http_instance = Net::HTTP.new("github.com", 443) + fake_not_found = Net::HTTPNotFound.new(nil, nil, nil) + expect(Net::HTTP).to receive(:new).and_return(http_instance) + expect(http_instance).to receive(:request_head).once.and_return(fake_not_found) + + expect(described_class.username_lookup(lookup_username)).to eq(nil) + expect(described_class.username_lookup(lookup_username)).to eq(nil) + end + end + + context "when GitHub is having a bad time..." do + let(:lookup_status) { 500 } + + it "looks up a user and does not stores the username in the cache" do + expect do + described_class.username_lookup(lookup_username) + end.to raise_error(RuntimeError, "Error on GitHub with username lookup") + expect(lookup_cache).to eq({}) + end + end + end +end diff --git a/spec/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker_spec.rb b/spec/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker_spec.rb new file mode 100644 index 00000000..3286b46d --- /dev/null +++ b/spec/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker_spec.rb @@ -0,0 +1,98 @@ +describe CommitMonitorHandlers::CommitRange::GithubPrCommenter::CommitMetadataChecker do + let(:batch_entry) { BatchEntry.create!(:job => BatchJob.create!) } + let(:branch) { create(:pr_branch) } + let(:commit_message_1) { "Adds SUPER FEATURE. BREAKS NOTHING!" } + let(:commit_message_2) { "fix tests" } + let(:commit_message_3) { "fix moar tests" } + + let(:commits) do + { + "abcd123" => {"message" => commit_message_1, "files" => ["app/models/super.rb"]}, + "abcd234" => {"message" => commit_message_2, "files" => ["spec/models/super_spec.rb"]}, + "abcd345" => {"message" => commit_message_3, "files" => ["spec/models/normal_spec.rb"]} + } + end + + let(:username_lookup_cache) do + { + "Fryguy" => 123, # valid user + "NickLaMuro" => 234, # valid user + "NickLaMura" => nil, # mispelled invalid user + "Nick-LaMuro" => nil, # invalid user with hyphen + "booksandauthors" => nil # not a user (surprisingly) + } + end + + before do + stub_sidekiq_logger + stub_job_completion + + allow(GithubService).to receive(:username_lookup_cache).and_return(username_lookup_cache) + end + + context "with basic commit messages" do + it "doesn't create any offenses" do + described_class.new.perform(batch_entry.id, branch.id, commits) + + batch_entry.reload + expect(batch_entry.result.length).to eq(0) + end + end + + context "with multiline commit messages" do + let(:commit_message_1) do + <<~COMMIT_MSG + In which the hero adds the greatest feature... + + With this change, I made it so that by changing this: + + @database_records = Books.all + + Avoids a N+1 on authors by adding an .includes call + + @booksandauthors = Books.all.includes(:author) + + Saving us 100s of queries for our tiny bookstore app. + + Thanks to @Fryguy and @Nick-LaMuro for this suggestion! + COMMIT_MSG + end + + let(:commit_message_2) do + <<~COMMIT_MSG + fixes tests + + Forgot that we stubbed things... + + cc @Fryguy @NickLaMuro + COMMIT_MSG + end + + let(:commit_message_3) do + <<~COMMIT_MSG + fixes moar tests + + I forget how to rebase... #dealWithIt @NickLaMura + COMMIT_MSG + end + + it "returns one offense for each valid username" do + described_class.new.perform(batch_entry.id, branch.id, commits) + + batch_entry.reload + expect(batch_entry.result.length).to eq(3) + expect(batch_entry.result.first).to have_attributes( + :group => "https://github.com/#{branch.fq_repo_name}/commit/abcd123", + :message => "Username `@Fryguy` detected in commit message. Consider removing." + ) + expect(batch_entry.result.second).to have_attributes( + :group => "https://github.com/#{branch.fq_repo_name}/commit/abcd234", + :message => "Username `@Fryguy` detected in commit message. Consider removing." + ) + expect(batch_entry.result.third).to have_attributes( + :group => "https://github.com/#{branch.fq_repo_name}/commit/abcd234", + :message => "Username `@NickLaMuro` detected in commit message. Consider removing." + ) + end + end +end