New diff notes
Fixes#12732, #14731, #19375, #14783
Builds on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4110
To do:
- [x] Get it mostly working
- [x] Validate position validity
- [x] Fix: Don’t link to `#`
- [x] Fix: Base ref can be `nil`, potentially, when the MR has an oprhan source branch => Yep, doesn’t work. We need to store a `start_id`
- [x] Optimize: Fewer duplicate `git diff` compares
- [x] Optimize: Pass paths to `PositionTracer#diff` for faster diffs
- [x] Refactor: Use `head_id` in `MergeRequest`/`MergeRequestDiff` instead of `source_sha`
- [x] Refactor: Convert existing array-based diff refs to the DiffRefs model
- [x] Tweak: Use `note_type` in `Autosave` key
- [x] Tweak: Remove `line_code: note.line_code` from `link_to_reply_discussion`
- [x] Update: `SentNotifications` and reply-by-email receiver
- [x] Update: MR diff notification email
- [x] Update: API (MR, Commit note creation and entity)
- [x] Update: GitHub importer
- [x] Address any other TODO comments
- [x] Fix: Suppress "edited 4 minutes ago"
- [x] Write tests
- [x] `LineMapper`
- [x] `PositionTracer`
- [x] `Position`
- [x] `DiffPositionUpdateService`
- [x] `DiffNote`
- [x] `MergeRequests::RefreshService` / `MergeRequest#update_diff_notes_positions`
- [x] Make sure commits with diff notes don't get cleaned up, since this would prevent the diff notes from being rendered (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5062)
Future improvements:
- Display unresolved comments on files outside the diff, if the comment was added when that file _was_ part of the diff
- Allow commenting on sections between hunks, when expanding the diff using `...`
- (We'd need to generate line code based on Position if we have it, even if it falls outside bounds of diff)
- `diff_hunk` on diff note API entity
- Show diff hunk in notification email
- Resolved line notes would have a boolean, and be inactive through `notes.any? { !active? || resolved? }`
- Multi line notes would store a number of positions, and do the right thing (™) in grouping and then rendering if the first item is multiline? => true
- Image diff notes could store x,y,width,height instead of old_line,new_line for similar grouping. Does it need a reference to say if it's on old or new? These can't have line_codes, clearly. Rendering would be interesting.
- Show commit line comments in the MR diff
- Comment on specific selected words
- Comment on file header
- Unfold top of discussion diff note
- New diff notes API for commits and MRs
/cc @rspeicher
See merge request !4101
Services: code style fixes, minor refactoring
## What does this MR do?
It contains code style fixes, minor refactoring and also it removes some unnecessary comments. Every comment require a support just like a regular code line and this is why we should avoid using comments everywhere unless it's really helpful. Martin Fowler said "Find comment and refactor the code around it" :) our code is not that bad so let's not spoil it with comments.
## Why was this MR needed?
Because GitLab is a live example of awesome code. Let's keep up a good job :)
See merge request !5112
* master: (98 commits)
Enable Style/EmptyLines cop, remove redundant ones
Update CHANGELOG
Cache results from jQuery selectors to retrieve namespace name
Fix import button when import fail due the namespace already been taken
Fix snippets comments not displayed
Fix emoji paths in relative root configurations
Exclude requesters from Project#members, Group#members and User#members
Upgrade Thin from 1.6.1 to 1.7.0.
Many squashed commits
Cache autocomplete results
Upgrade Sidekiq from 4.1.2 to 4.1.4.
Upgrade seed-fu from 2.3.5 to 2.3.6
use has_many relationship with events
Support creating a todo on issuables via API
Expose target, filter by state as string
Add todos API documentation and changelog
Improve the request / withdraw access button
Metrics for Rouge::Plugins::Redcarpet and Rouge::Formatters::HTMLGitlab
Groundwork for Kerberos SPNEGO (EE feature)
Update CHANGELOG 8.9.5 for runners related fixes
...
Fix auto-MR-close text from branch name
## What does this MR do?
Fix the MR-auto-close feature if a branch name begins with an issue IID.
## Are there points in the code the reviewer needs to double check?
It's one line, so check that line thoroughly 😃
## Why was this MR needed?
When I create a new MR from a branch starting with an issue IID, the description doesn't get suffixed with the 'Closes #blahblah' that it should do.
## What are the relevant issue numbers?
Don't think there are any.
## Screenshots (if relevant)
## Does this MR meet the acceptance criteria?
- [ ] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
See merge request !4836
Resolve "Destroying a project causes post_decline_request to be executed"
## What does this MR do?
Ensure we don't send "access request declined" to access requesters when a project is deleted.
## Are there points in the code the reviewer needs to double check?
I've created a service to decouple the notification sending from the AR model.
## Why was this MR needed?
Because there was an issue.
## What are the relevant issue numbers?
Fixes#18755, #18750.
## Does this MR meet the acceptance criteria?
- [x] No CHANGELOG needed.
- [x] Tests
- [x] Added for this feature/bug
- [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4744
Rails's form helpers use the `$attr_before_type_cast` method where
available, and this value only appears to be updated on assignment, not
when the object is mutated in some other way:
[1] pry(main)> mr = MergeRequest.new
=> #<MergeRequest:0x007fcf28395d88 ...>
[2] pry(main)> mr.description = 'foo'
=> "foo"
[3] pry(main)> mr.description << ' bar'
=> "foo bar"
[4] pry(main)> mr.description
=> "foo bar"
[5] pry(main)> mr.description_before_type_cast
=> "foo"
[6] pry(main)> mr.description += ' bar'
=> "foo bar bar"
[7] pry(main)> mr.description_before_type_cast
=> "foo bar bar"
* upstream/master: (353 commits)
Put some admin settings in dropdown
Add styleguide on configuration settings documentation
Remove Duplicated keys add UNIQUE index to fingerprint
Avoid autoload issue such as 'Mail::Parsers::AddressStruct'
Move appearance settings as sub tab to application settings
use rails root join
fixed a couple of errors spotted in production
Fix RangeError exceptions when referring to issues or merge requests outside of max database values
Fix bug in `WikiLinkFilter`.
Grammar and typographic changes to artifacts documentation
Tweak grammar
Small frontend code fixes and restore 8a2d88f commit
Warn about admin privilege to disable GitHub Webhooks
Listing GH Webhooks doesn't stop import process for non GH admin users
fixup! updated docs for api endpoint award emoji
Update CHANGELOG
Ensure Todos counters doesn't count Todos for projects pending delete
Add endpoints for award emoji on notes
Sort API endpoints and implement feedback
Add endpoints for Award Emoji
...
This is to ensure we don't send unwanted notifications when deleting a
project. In other words, stop abusing AR callbacks and use services.
Signed-off-by: Rémy Coutable <remy@rymai.me>
Implement custom notification level options
part of #12758
See merge request !4389
- As todos are created/updated inside the TodoService
we repopulate the cache just there for both pending/done todos
- Todos as mark as done from the TodosController we update cache
there too
- All the added methods are kept in the User class for cohesion
* upstream/master: (337 commits)
Update CHANGELOG for !4659
Center the header logo for all Devise emails
Add previews for all customized Devise emails
Customize the Devise `unlock_instructions` email
Customize the Devise `reset_password_instructions` email
Customize the Devise `password_change` emails
Use gitlab-git 10.2.0
Use Git cached counters on project show page
Fix indentation scss-lint errors
Added title attribute to enties in tree view Closes#18353
Banzai::Filter::ExternalLinkFilter use XPath
Reduce queries in IssueReferenceFilter
Use gitlab_git 10.1.4
Fixed ordering in Project.find_with_namespace
Fix images in emails
Banzai::Filter::UploadLinkFilter use XPath
Turn Group#owners into a has_many association
Make project_id nullable
CHANGELOG [ci skip]
CHANGELOG [ci skip]
...
Improve pipeline status in case that pipeline has no jobs
## What does this MR do?
This MR resolves problem with pipeline status when there are no build in pipeline.
This can happen when builds were skipped - for example - by using `only`/`except` keyword in `.gitlab-ci.yml`.
## What are the relevant issue numbers?
Closes#17977
See merge request !4403
Fair usage of Shared Runners
## What does this MR do?
Introduces a fair usage scheduler for shared runners.
It tries to assign builds to shared runner from projects that have the lowest number of builds currently running on shared runners.
**Example 1**:
```
We have following builds in queue:
build 1 for project 1
build 2 for project 1
build 3 for project 1
build 4 for project 2
build 5 for project 2
build 6 for project 3
With the new algorithm we will assign builds in following order:
- We choose build 1, because project 1 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We choose build 4, because project 2 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We choose build 6, because project 3 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We choose build 2, because project 1 as other it runs 1 build,
- We choose build 5, because project 2 runs 1 build, where project 1 runs 2 builds now,
- We choose build 3, because project 1 and runs 2 builds.
```
**Example 2**:
```
We have following builds in queue:
build 1 for project 1
build 2 for project 1
build 3 for project 1
build 4 for project 2
build 5 for project 2
build 6 for project 3
With the new algorithm we will assign builds in following order:
- We choose build 1, because project 1 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We finish build 1,
- We choose build 2, because project 1 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We choose build 4, because project 2 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We finish build 4,
- We choose build 5, because project 2 doesn't run currently any builds and has the lowest build number from projects that doesn't run builds,
- We choose build 6, because project 3 doesn't run currently any builds,
- We choose build 3, because project 1, 2 and 3 runs exactly one build now,
```
## Why was this MR needed?
Currently, we are scheduling builds using FIFO. This is catastrophic if there are projects that create a 100-300 jobs, this basically eats most of available shared runners.
## Performance
All this logic is implemented with the help of SQL queries, because this is the fastest way to process 1k-2k pending builds in queue.
It's not the fastest SQL query, because it sorts based on number of running_builds, and this forces to calculate a number of running builds for all dependent projects. However, since we have one/two shared runners that asks every few seconds for builds this should have minimal impact on DB performance.
```
explain analyze SELECT "ci_builds".* FROM "ci_builds" JOIN (SELECT "ci_builds"."gl_project_id", count(case when status = 'running' AND runner_id = (SELECT "ci_runners"."id" FROM "ci_runners" WHERE "ci_runners"."is_shared" = 't') then 1 end) as running_builds FROM "ci_builds" INNER JOIN "projects" ON "projects"."id" = "ci_builds"."gl_project_id" AND "projects"."pending_delete" = 'f' WHERE "ci_builds"."type" IN ('Ci::Build') AND "ci_builds"."status" IN ('running', 'pending') AND "projects"."builds_enabled" = 't' AND "projects"."shared_runners_enabled" = 't' GROUP BY "ci_builds"."gl_project_id") AS projects ON ci_builds.gl_project_id=projects.gl_project_id WHERE "ci_builds"."type" IN ('Ci::Build') AND "ci_builds"."status" = 'pending' AND "ci_builds"."runner_id" IS NULL ORDER BY projects.running_builds ASC, ci_builds.id ASC;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------------------------------
-----------
Sort (cost=64777.28..64777.29 rows=1 width=1010) (actual time=301.794..302.535 rows=1537 loops=1)
Sort Key: (count(CASE WHEN (((public.ci_builds.status)::text = 'running'::text) AND (public.ci_builds.runner_id = $0)) THEN 1 ELSE NULL::integer END)), public.ci
_builds.id
Sort Method: quicksort Memory: 1423kB
-> Nested Loop (cost=63279.78..64777.27 rows=1 width=1010) (actual time=66.384..298.724 rows=1537 loops=1)
-> HashAggregate (cost=63177.15..63177.30 rows=15 width=15) (actual time=65.641..65.851 rows=187 loops=1)
InitPlan 1 (returns $0)
-> Seq Scan on ci_runners (cost=0.00..26963.66 rows=1 width=4) (actual time=1.145..34.381 rows=1 loops=1)
Filter: is_shared
Rows Removed by Filter: 6965
-> Nested Loop (cost=0.00..36186.34 rows=2715 width=15) (actual time=0.065..29.717 rows=1710 loops=1)
-> Index Scan using index_ci_builds_on_status on ci_builds (cost=0.00..8913.95 rows=3577 width=15) (actual time=0.051..12.012 rows=2583 loops
=1)
Index Cond: ((status)::text = ANY ('{running,pending}'::text[]))
Filter: ((type)::text = 'Ci::Build'::text)
Rows Removed by Filter: 1219
-> Index Scan using projects_pkey on projects (cost=0.00..7.61 rows=1 width=4) (actual time=0.003..0.004 rows=1 loops=2583)
Index Cond: (id = public.ci_builds.gl_project_id)
Filter: ((NOT pending_delete) AND builds_enabled AND shared_runners_enabled)
Rows Removed by Filter: 0
-> Bitmap Heap Scan on ci_builds (cost=102.63..106.64 rows=1 width=1002) (actual time=1.216..1.231 rows=8 loops=187)
Recheck Cond: ((gl_project_id = public.ci_builds.gl_project_id) AND ((status)::text = 'pending'::text))
Filter: ((runner_id IS NULL) AND ((type)::text = 'Ci::Build'::text))
-> BitmapAnd (cost=102.63..102.63 rows=1 width=0) (actual time=1.201..1.201 rows=0 loops=187)
-> Bitmap Index Scan on index_ci_builds_on_gl_project_id (cost=0.00..10.52 rows=241 width=0) (actual time=0.406..0.406 rows=1944 loops=187)
Index Cond: (gl_project_id = public.ci_builds.gl_project_id)
-> Bitmap Index Scan on index_ci_builds_on_status (cost=0.00..91.78 rows=3089 width=0) (actual time=0.652..0.652 rows=3362 loops=187)
Index Cond: ((status)::text = 'pending'::text)
Total runtime: 303.832 ms
```
## Specific runners
It doesn't affect the specific runners which still serve builds FIFO.
@stanhu @markpundsack @yorickpeterse What do you think?
See merge request !4634
* master: (198 commits)
Set inverse_of for Project/Services relation
Fix admin hooks spec
Prevent default disabled buttons and links.
Add index on `requested_at` to the `members` table
Rearrange order of tabs
Fix admin active tab tests
Show created_at in table column
Nest li elements directly under ul
Move builds tab to admin overview
Add monitoring link with subtabs
Add sub links to overview
Add counter for abuse reports
Remove admin layout-nav counters
Move admin nav to horizontal layout nav
Eager load project relations in IssueParser
Use validate and required for environment and project
Award Emoji can't be awarded on system notes backend
Get rid of Gitlab::ShellEnv
Update CHANGELOG.
Fix project star tooltip on the fly.
...
Conflicts:
app/services/ci/create_builds_service.rb
Grzegorz Bizon
Douwe Maan
Dmitriy Zaporozhets
Douwe Maan
Valery Sizov
Rémy Coutable
Patricio Cano
James Lopez
Paco Guzman
Tomasz Maczukin
Sean McGivern
Alejandro Rodríguez
Phil Hughes
Lin Jen-Shin
Yorick Peterse
Felipe Artur
Robert Speicher
Douglas Barbosa Alexandre
Stan Hu