Process up to 100 commit messages for references when pushing to a new default branch
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
-
Changelog entry -
Documentation created/updated or follow-up review issue created -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Performance and testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
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
Merge request reports
Activity
marked the checklist item Changelog entry as completed
marked the checklist item Changelog entry as incomplete
marked the checklist item Changelog entry as completed
- Resolved by Douwe Maan
@DouweM We discussed this change in https://gitlab.com/gitlab-org/gitlab-ce/issues/56683. Do you see any reason why we shouldn't merge this?
assigned to @DouweM
changed milestone to %12.0
added devopscreate label
added Community contribution 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
added 1 commit
- ec51f4fa - Test that count_commits_in_branch is returning the correct number
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!
@fapapa Looks great; thanks for your contribution!
mentioned in commit 0c4059ef
mentioned in issue gitlab-org/release/tasks#812 (closed)
mentioned in issue #56683 (closed)