Skip to content
Snippets Groups Projects

Process up to 100 commit messages for references when pushing to a new default branch

Merged Fabio Papa requested to merge fapapa/gitlab-ce:patch-issue-#56683 into master
All threads resolved!

What does this MR do?

Patch for the bug described in #56683 (closed). On first push to the default branch, we are currently not processing commits for references to issues. This is done on purpose, probably for performance reasons, to avoid processing thousands of commits. If we limit the commit processing to a reasonable number of commits, performance will not be impacted significantly, and this will cease to appear as a bug for the vast majority of users.

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Douwe Maan

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Fabio Papa marked the checklist item Changelog entry as completed

    marked the checklist item Changelog entry as completed

  • Fabio Papa marked the checklist item Changelog entry as incomplete

    marked the checklist item Changelog entry as incomplete

  • Fabio Papa added 3 commits

    added 3 commits

    • f3437a3b - Add the changelog
    • 83633bdd - Modify the branch hooks spec to expect processing of commit messages
    • 5b720d38 - Allow commit messages to be processed on any push to the repo

    Compare with previous version

  • Fabio Papa marked the checklist item Changelog entry as completed

    marked the checklist item Changelog entry as completed

  • Fabio Papa unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Fabio Papa changed the description

    changed the description

  • assigned to @DouweM

  • Stan Hu changed milestone to %12.0

    changed milestone to %12.0

  • added devopscreate label

    • Resolved by Douwe Maan

      @fapapa Thanks for your contribution! Can you please check out this job failure: https://gitlab.com/fapapa/gitlab-ce/-/jobs/229443263?

      Failures:
      
        1) Git::BranchHooksService Processing commit messages creating the default branch processes a limited number of commit messages
           Failure/Error: expect(ProcessCommitWorker).to receive(:perform_async).once
      
             (ProcessCommitWorker (class)).perform_async(*(any args))
                 expected: 1 time with any arguments
                 received: 0 times with any arguments
           # ./spec/services/git/branch_hooks_service_spec.rb:291:in `block (4 levels) in <top (required)>'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/error_generator.rb:310:in `__raise'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/error_generator.rb:82:in `raise_expectation_error'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/message_expectation.rb:488:in `generate_error'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/message_expectation.rb:446:in `verify_messages_received'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/method_double.rb:113:in `block in verify'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/method_double.rb:113:in `each'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/method_double.rb:113:in `verify'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/proxy.rb:138:in `block in verify'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/proxy.rb:138:in `each_value'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/proxy.rb:138:in `verify'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/space.rb:74:in `block in verify_all'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/space.rb:74:in `each'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks/space.rb:74:in `verify_all'
           # ./vendor/ruby/2.6.0/gems/rspec-mocks-3.7.0/lib/rspec/mocks.rb:45:in `verify'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/mocking_adapters/rspec.rb:23:in `verify_mocks_for_rspec'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:514:in `verify_mocks'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:508:in `run_after_example'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:273:in `block in run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:500:in `block in with_around_and_singleton_context_hooks'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:457:in `block in with_around_example_hooks'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:466:in `block in run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:606:in `block in run_around_example_hooks_for'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:342:in `call'
           # ./vendor/ruby/2.6.0/gems/rspec-rails-3.7.2/lib/rspec/rails/adapters.rb:127:in `block (2 levels) in <module:MinitestLifecycleAdapter>'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:447:in `instance_exec'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:447:in `instance_exec'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:375:in `execute_with'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:608:in `block (2 levels) in run_around_example_hooks_for'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:342:in `call'
           # ./vendor/ruby/2.6.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:123:in `block in run'
           # ./vendor/ruby/2.6.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `loop'
           # ./vendor/ruby/2.6.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-retry-0.6.1/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
           # ./vendor/ruby/2.6.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:37:in `block (2 levels) in setup'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:447:in `instance_exec'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:447:in `instance_exec'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:375:in `execute_with'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:608:in `block (2 levels) in run_around_example_hooks_for'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:342:in `call'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:609:in `run_around_example_hooks_for'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/hooks.rb:466:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:457:in `with_around_example_hooks'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:500:in `with_around_and_singleton_context_hooks'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example.rb:251:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:628:in `block in run_examples'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:624:in `map'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:624:in `run_examples'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:590:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:591:in `block in run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:591:in `map'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:591:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:591:in `block in run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:591:in `map'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/example_group.rb:591:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:118:in `block (3 levels) in run_specs'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:118:in `map'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:118:in `block (2 levels) in run_specs'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/configuration.rb:1926:in `with_suite_hooks'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:113:in `block in run_specs'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/reporter.rb:79:in `report'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:112:in `run_specs'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:87:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:71:in `run'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/lib/rspec/core/runner.rb:45:in `invoke'
           # ./vendor/ruby/2.6.0/gems/rspec-core-3.7.1/exe/rspec:4:in `<top (required)>'
           # ./vendor/ruby/2.6.0/bin/rspec:23:in `load'
           # ./vendor/ruby/2.6.0/bin/rspec:23:in `<top (required)>'
           # /usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
           # /usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
           # /usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
           # /usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
           # /usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
           # /usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
           # /usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
           # /usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
           # /usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
           # /usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
           # /usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in <top (required)>'
           # /usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
           # /usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `<top (required)>'
           # /usr/local/bin/bundle:23:in `load'
           # /usr/local/bin/bundle:23:in `<main>'
      
      Finished in 12 minutes 8 seconds (files took 18.35 seconds to load)
      1765 examples, 1 failure, 20 pending
      
      Failed examples:
      
      rspec ./spec/services/git/branch_hooks_service_spec.rb:290 # Git::BranchHooksService Processing commit messages creating the default branch processes a limited number of commit messages
  • Douwe Maan changed title from Patch issue #56683 to Process up to 100 commit messages for references when pushing to a new default branch

    changed title from Patch issue #56683 to Process up to 100 commit messages for references when pushing to a new default branch

  • Fabio Papa added 1 commit

    added 1 commit

    • ec51f4fa - Test that count_commits_in_branch is returning the correct number

    Compare with previous version

  • Fabio Papa added 2 commits

    added 2 commits

    • f0c16a08 - Reverted last commit
    • 017e72c6 - Fix an issue with the branch hooks service in getting the last commits

    Compare with previous version

  • Author Contributor

    Okay @DouweM, the specs now pass. Turns out there was another bug that was causing the oldest commit messages to be processed instead of the newest when pushing to a new default branch. If there were more than 100 commits, it would process the commit messages for the first 100 instead of the last 100. In the case of the failing spec, it processed the first 2 commits instead of the most recent 2 commits, and those did not have references to issues in them, causing the spec to fail when perform_async wasn't called as expected. I think it's ready to merge!

  • Douwe Maan approved this merge request

    approved this merge request

  • Douwe Maan resolved all discussions

    resolved all discussions

  • @fapapa Looks great; thanks for your contribution!

  • merged

  • Douwe Maan mentioned in commit 0c4059ef

    mentioned in commit 0c4059ef

Please register or sign in to reply
Loading