diff options
33 files changed, 487 insertions, 174 deletions
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 82d1abff4a4..fb78973a727 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -301,7 +301,7 @@ For guidance on UX implementation at GitLab, please refer to our [Design System] The UX team uses labels to manage their workflow. The ~"UX" label on an issue is a signal to the UX team that it will need UX attention. -To better understand the priority by which UX tackles issues, see the [UX section](https://about.gitlab.com/handbook/ux/) of the handbook. +To better understand the priority by which UX tackles issues, see the [UX section](https://about.gitlab.com/handbook/engineering/ux) of the handbook. Once an issue has been worked on and is ready for development, a UXer applies the ~"UX ready" label to that issue. diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index f1b9cc4cd95..c64d9d48a48 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.105.0 +0.105.1 diff --git a/app/assets/javascripts/boards/models/list.js b/app/assets/javascripts/boards/models/list.js index a79dd62e2e4..1f0fe7f9e85 100644 --- a/app/assets/javascripts/boards/models/list.js +++ b/app/assets/javascripts/boards/models/list.js @@ -55,7 +55,8 @@ class List { entityType = 'assignee_id'; } - return gl.boardService.createList(this.label.id) + return gl.boardService + .createList(entity.id, entityType) .then(res => res.data) .then(data => { this.id = data.id; diff --git a/app/assets/javascripts/merge_conflicts/components/inline_conflict_lines.js b/app/assets/javascripts/merge_conflicts/components/inline_conflict_lines.js index 7f078d420c4..827cf5f478d 100644 --- a/app/assets/javascripts/merge_conflicts/components/inline_conflict_lines.js +++ b/app/assets/javascripts/merge_conflicts/components/inline_conflict_lines.js @@ -1,12 +1,14 @@ -/* eslint-disable no-param-reassign, comma-dangle */ +/* eslint-disable no-param-reassign */ import Vue from 'vue'; +import actionsMixin from '../mixins/line_conflict_actions'; +import utilsMixin from '../mixins/line_conflict_utils'; -((global) => { +(global => { global.mergeConflicts = global.mergeConflicts || {}; global.mergeConflicts.inlineConflictLines = Vue.extend({ - mixins: [global.mergeConflicts.utils, global.mergeConflicts.actions], + mixins: [utilsMixin, actionsMixin], props: { file: { type: Object, diff --git a/app/assets/javascripts/merge_conflicts/components/parallel_conflict_lines.js b/app/assets/javascripts/merge_conflicts/components/parallel_conflict_lines.js index b38ee845fe5..57e73e38d88 100644 --- a/app/assets/javascripts/merge_conflicts/components/parallel_conflict_lines.js +++ b/app/assets/javascripts/merge_conflicts/components/parallel_conflict_lines.js @@ -1,12 +1,14 @@ /* eslint-disable no-param-reassign, comma-dangle */ import Vue from 'vue'; +import actionsMixin from '../mixins/line_conflict_actions'; +import utilsMixin from '../mixins/line_conflict_utils'; ((global) => { global.mergeConflicts = global.mergeConflicts || {}; global.mergeConflicts.parallelConflictLines = Vue.extend({ - mixins: [global.mergeConflicts.utils, global.mergeConflicts.actions], + mixins: [utilsMixin, actionsMixin], props: { file: { type: Object, diff --git a/app/assets/javascripts/merge_conflicts/merge_conflict_service.js b/app/assets/javascripts/merge_conflicts/merge_conflict_service.js index c68b47c9348..64d69159222 100644 --- a/app/assets/javascripts/merge_conflicts/merge_conflict_service.js +++ b/app/assets/javascripts/merge_conflicts/merge_conflict_service.js @@ -1,23 +1,16 @@ -/* eslint-disable no-param-reassign, comma-dangle */ import axios from '../lib/utils/axios_utils'; -((global) => { - global.mergeConflicts = global.mergeConflicts || {}; - - class mergeConflictsService { - constructor(options) { - this.conflictsPath = options.conflictsPath; - this.resolveConflictsPath = options.resolveConflictsPath; - } - - fetchConflictsData() { - return axios.get(this.conflictsPath); - } +export default class MergeConflictsService { + constructor(options) { + this.conflictsPath = options.conflictsPath; + this.resolveConflictsPath = options.resolveConflictsPath; + } - submitResolveConflicts(data) { - return axios.post(this.resolveConflictsPath, data); - } + fetchConflictsData() { + return axios.get(this.conflictsPath); } - global.mergeConflicts.mergeConflictsService = mergeConflictsService; -})(window.gl || (window.gl = {})); + submitResolveConflicts(data) { + return axios.post(this.resolveConflictsPath, data); + } +} diff --git a/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js b/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js index 4abd5433bb5..326d4523cce 100644 --- a/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js +++ b/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js @@ -5,7 +5,7 @@ import Vue from 'vue'; import Flash from '../flash'; import initIssuableSidebar from '../init_issuable_sidebar'; import './merge_conflict_store'; -import './merge_conflict_service'; +import MergeConflictsService from './merge_conflict_service'; import './mixins/line_conflict_utils'; import './mixins/line_conflict_actions'; import './components/diff_file_editor'; @@ -17,7 +17,7 @@ export default function initMergeConflicts() { const INTERACTIVE_RESOLVE_MODE = 'interactive'; const conflictsEl = document.querySelector('#conflicts'); const mergeConflictsStore = gl.mergeConflicts.mergeConflictsStore; - const mergeConflictsService = new gl.mergeConflicts.mergeConflictsService({ + const mergeConflictsService = new MergeConflictsService({ conflictsPath: conflictsEl.dataset.conflictsPath, resolveConflictsPath: conflictsEl.dataset.resolveConflictsPath }); diff --git a/app/assets/javascripts/merge_conflicts/mixins/line_conflict_actions.js b/app/assets/javascripts/merge_conflicts/mixins/line_conflict_actions.js index 53e000d7e9e..364ae2b2688 100644 --- a/app/assets/javascripts/merge_conflicts/mixins/line_conflict_actions.js +++ b/app/assets/javascripts/merge_conflicts/mixins/line_conflict_actions.js @@ -1,13 +1,7 @@ -/* eslint-disable no-param-reassign, comma-dangle */ - -((global) => { - global.mergeConflicts = global.mergeConflicts || {}; - - global.mergeConflicts.actions = { - methods: { - handleSelected(file, sectionId, selection) { - gl.mergeConflicts.mergeConflictsStore.handleSelected(file, sectionId, selection); - } - } - }; -})(window.gl || (window.gl = {})); +export default { + methods: { + handleSelected(file, sectionId, selection) { + gl.mergeConflicts.mergeConflictsStore.handleSelected(file, sectionId, selection); + }, + }, +}; diff --git a/app/assets/javascripts/merge_conflicts/mixins/line_conflict_utils.js b/app/assets/javascripts/merge_conflicts/mixins/line_conflict_utils.js index 0f475f62ee6..d25032fb142 100644 --- a/app/assets/javascripts/merge_conflicts/mixins/line_conflict_utils.js +++ b/app/assets/javascripts/merge_conflicts/mixins/line_conflict_utils.js @@ -1,19 +1,13 @@ -/* eslint-disable no-param-reassign, quote-props, comma-dangle */ - -((global) => { - global.mergeConflicts = global.mergeConflicts || {}; - - global.mergeConflicts.utils = { - methods: { - lineCssClass(line) { - return { - 'head': line.isHead, - 'origin': line.isOrigin, - 'match': line.hasMatch, - 'selected': line.isSelected, - 'unselected': line.isUnselected - }; - } - } - }; -})(window.gl || (window.gl = {})); +export default { + methods: { + lineCssClass(line) { + return { + head: line.isHead, + origin: line.isOrigin, + match: line.hasMatch, + selected: line.isSelected, + unselected: line.isUnselected, + }; + }, + }, +}; diff --git a/app/assets/stylesheets/framework/mixins.scss b/app/assets/stylesheets/framework/mixins.scss index d76cf8f8182..0b645eb811b 100644 --- a/app/assets/stylesheets/framework/mixins.scss +++ b/app/assets/stylesheets/framework/mixins.scss @@ -186,6 +186,7 @@ overflow-y: hidden; -webkit-overflow-scrolling: touch; display: flex; + flex-wrap: nowrap; &::-webkit-scrollbar { display: none; diff --git a/app/assets/stylesheets/framework/secondary_navigation_elements.scss b/app/assets/stylesheets/framework/secondary_navigation_elements.scss index 847fc8c0792..4e2cc498883 100644 --- a/app/assets/stylesheets/framework/secondary_navigation_elements.scss +++ b/app/assets/stylesheets/framework/secondary_navigation_elements.scss @@ -230,6 +230,8 @@ } .scrolling-tabs-container { + position: relative; + .merge-request-tabs-container & { overflow: hidden; } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 9eceb3e9a33..f50ca677800 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -678,6 +678,7 @@ .merge-request-tabs { display: flex; + flex-wrap: nowrap; margin-bottom: 0; padding: 0; } diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 22964163e95..caafda5fb05 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -354,12 +354,6 @@ min-width: 200px; } -.deploy-keys { - .scrolling-tabs-container { - position: relative; - } -} - .deploy-key { // Ensure that the fingerprint does not overflow on small screens .fingerprint { diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index 01e38ffee20..2f1877a15c2 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -32,26 +32,25 @@ .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller .fade-left= icon('angle-left') .fade-right= icon('angle-right') - .nav-links.scrolling-tabs.nav.nav-tabs - %ul.merge-request-tabs.nav-tabs.nav - %li.notes-tab - = tab_link_for @merge_request, :show, force_link: @commit.present? do - Discussion - %span.badge.badge-pill= @merge_request.related_notes.user.count - - if @merge_request.source_project - %li.commits-tab - = tab_link_for @merge_request, :commits do - Commits - %span.badge.badge-pill= @commits_count - - if @pipelines.any? - %li.pipelines-tab - = tab_link_for @merge_request, :pipelines do - Pipelines - %span.badge.badge-pill.js-pipelines-mr-count= @pipelines.size - %li.diffs-tab - = tab_link_for @merge_request, :diffs do - Changes - %span.badge.badge-pill= @merge_request.diff_size + %ul.merge-request-tabs.nav-tabs.nav.nav-links.scrolling-tabs + %li.notes-tab + = tab_link_for @merge_request, :show, force_link: @commit.present? do + Discussion + %span.badge.badge-pill= @merge_request.related_notes.user.count + - if @merge_request.source_project + %li.commits-tab + = tab_link_for @merge_request, :commits do + Commits + %span.badge.badge-pill= @commits_count + - if @pipelines.any? + %li.pipelines-tab + = tab_link_for @merge_request, :pipelines do + Pipelines + %span.badge.badge-pill.js-pipelines-mr-count= @pipelines.size + %li.diffs-tab + = tab_link_for @merge_request, :diffs do + Changes + %span.badge.badge-pill= @merge_request.diff_size - if has_vue_discussions_cookie? #js-vue-discussion-counter diff --git a/changelogs/unreleased/mk-rake-task-verify-remote-files.yml b/changelogs/unreleased/mk-rake-task-verify-remote-files.yml new file mode 100644 index 00000000000..772aa11d89b --- /dev/null +++ b/changelogs/unreleased/mk-rake-task-verify-remote-files.yml @@ -0,0 +1,5 @@ +--- +title: Add support for verifying remote uploads, artifacts, and LFS objects in check rake tasks +merge_request: 19501 +author: +type: added diff --git a/changelogs/unreleased/sql-buckets.yml b/changelogs/unreleased/sql-buckets.yml new file mode 100644 index 00000000000..afb13d5cb20 --- /dev/null +++ b/changelogs/unreleased/sql-buckets.yml @@ -0,0 +1,5 @@ +--- +title: Adjust SQL and transaction Prometheus buckets +merge_request: +author: +type: other diff --git a/doc/administration/raketasks/check.md b/doc/administration/raketasks/check.md index 7d34d35e7d1..2649bf61d74 100644 --- a/doc/administration/raketasks/check.md +++ b/doc/administration/raketasks/check.md @@ -78,9 +78,10 @@ Example output: ## Uploaded Files Integrity -Various types of file can be uploaded to a GitLab installation by users. -Checksums are generated and stored in the database upon upload, and integrity -checks using those checksums can be run. These checks also detect missing files. +Various types of files can be uploaded to a GitLab installation by users. +These integrity checks can detect missing files. Additionally, for locally +stored files, checksums are generated and stored in the database upon upload, +and these checks will verify them against current files. Currently, integrity checks are supported for the following types of file: diff --git a/doc/development/changelog.md b/doc/development/changelog.md index a9fa5ae834f..9e0c81b3d60 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -45,6 +45,8 @@ the `author` field. GitLab team members **should not**. a changelog entry regardless of these guidelines if the contributor wants one. Example: "Fixed a typo on the search results page. (Jane Smith)" - Performance improvements **should** have a changelog entry. +- Any change that introduces a database migration **must** have a + changelog entry. ## Writing good changelog entries diff --git a/doc/development/new_fe_guide/development/testing.md b/doc/development/new_fe_guide/development/testing.md index c359bd83ed1..e1e13474b75 100644 --- a/doc/development/new_fe_guide/development/testing.md +++ b/doc/development/new_fe_guide/development/testing.md @@ -1,3 +1,135 @@ -# Testing +# Overview of Frontend Testing -> TODO: Add content +## Types of tests in our codebase + +* **RSpec** + * **[Ruby unit tests](#ruby-unit-tests-spec-rb)** for models, controllers, helpers, etc. (`/spec/**/*.rb`) + * **[Full feature tests](#full-feature-tests-spec-features-rb)** (`/spec/features/**/*.rb`) +* **[Karma](#karma-tests-spec-javascripts-js)** (`/spec/javascripts/**/*.js`) +* ~~Spinach~~ — These have been removed from our codebase in May 2018. (`/features/`) + +## RSpec: Ruby unit tests `/spec/**/*.rb` + +These tests are meant to unit test the ruby models, controllers and helpers. + +### When do we write/update these tests? + +Whenever we create or modify any Ruby models, controllers or helpers we add/update corresponding tests. + +--- + +## RSpec: Full feature tests `/spec/features/**/*.rb` + +Full feature tests will load a full app environment and allow us to test things like rendering DOM, interacting with links and buttons, testing the outcome of those interactions through multiple pages if necessary. These are also called end-to-end tests but should not be confused with QA end-to-end tests (`package-and-qa` manual pipeline job). + +### When do we write/update these tests? + +When we add a new feature, we write at least two tests covering the success and the failure scenarios. + +### Relevant notes + +A `:js` flag is added to the test to make sure the full environment is loaded. + +``` +scenario 'successfully', :js do + sign_in(create(:admin)) +end +``` + +The steps of each test are written using capybara methods ([documentation](http://www.rubydoc.info/gems/capybara/2.15.1)). + +Bear in mind <abbr title="XMLHttpRequest">XHR</abbr> calls might require you to use `wait_for_requests` in between steps, like so: + +```rspec +find('.form-control').native.send_keys(:enter) + +wait_for_requests + +expect(page).not_to have_selector('.card') +``` + +--- + +## Karma tests `/spec/javascripts/**/*.js` + +These are the more frontend-focused, at the moment. They're **faster** than `rspec` and make for very quick testing of frontend components. + +### When do we write/update these tests? + +When we add/update a method/action/mutation to Vue or Vuex, we write karma tests to ensure the logic we wrote doesn't break. We should, however, refrain from writing tests that double-test Vue's internal features. + +### Relevant notes + +Karma tests are run against a virtual DOM. + +To populate the DOM, we can use fixtures to fake the generation of HTML instead of having Rails do that. + +Be sure to check the [best practices for karma tests](../../testing_guide/frontend_testing.html#best-practices). + +### Vue and Vuex + +Test as much as possible without double-testing Vue's internal features, as mentioned above. + +Make sure to test computedProperties, mutations, actions. Run the action and test that the proper mutations are committed. + +Also check these [notes on testing Vue components](../../fe_guide/vue.html#testing-vue-components). + +#### Vuex Helper: `testAction` + +We have a helper available to make testing actions easier, as per [official documentation](https://vuex.vuejs.org/en/testing.html): + +``` +testAction( + actions.actionName, // action + { }, // params to be passed to action + state, // state + [ + { type: types.MUTATION}, + { type: types.MUTATION_1, payload: {}}, + ], // mutations committed + [ + { type: 'actionName', payload: {}}, + { type: 'actionName1', payload: {}}, + ] // actions dispatched + done, +); +``` + +Check an example in [spec/javascripts/ide/stores/actions_spec.jsspec/javascripts/ide/stores/actions_spec.js](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/javascripts/ide/stores/actions_spec.js). + +#### Vue Helper: `mountComponent` + +To make mounting a Vue component easier and more readable, we have a few helpers available in `spec/helpers/vue_mount_component_helper`. + +* `createComponentWithStore` +* `mountComponentWithStore` + +Examples of usage: + +``` +beforeEach(() => { + vm = createComponentWithStore(Component, store); + + vm.$store.state.currentBranchId = 'master'; + + vm.$mount(); +}, +``` + +``` +beforeEach(() => { + vm = mountComponentWithStore(Component, { + el: '#dummy-element', + store, + props: { badge }, + }); +}, +``` + +Don't forget to clean up: + +``` +afterEach(() => { + vm.$destroy(); +}); +``` diff --git a/doc/gitlab-basics/start-using-git.md b/doc/gitlab-basics/start-using-git.md index 42cd8bb3e48..0d9994c9925 100644 --- a/doc/gitlab-basics/start-using-git.md +++ b/doc/gitlab-basics/start-using-git.md @@ -17,112 +17,197 @@ Depending on your operating system, you will need to use a shell of your prefere Git is usually preinstalled on Mac and Linux. Type the following command and then press enter: -``` + +```bash git --version ``` -You should receive a message that will tell you which Git version you have on your computer. If you don’t receive a "Git version" message, it means that you need to [download Git](https://git-scm.com/book/en/v2/Getting-Started-Installing-Git). +You should receive a message that tells you which Git version you have on your computer. If you don’t receive a "Git version" message, it means that you need to [download Git](https://git-scm.com/book/en/v2/Getting-Started-Installing-Git). If Git doesn't automatically download, there's an option on the website to [download manually](https://git-scm.com/downloads). Then follow the steps on the installation window. -After you are finished installing, open a new shell and type "git --version" again to verify that it was correctly installed. +After you are finished installing Git, open a new shell and type `git --version` again to verify that it was correctly installed. ## Add your Git username and set your email -It is important to configure your Git username and email address as every Git commit will use this information to identify you as the author. +It is important to configure your Git username and email address, since every Git commit will use this information to identify you as the author. On your shell, type the following command to add your username: -``` + +```bash git config --global user.name "YOUR_USERNAME" ``` Then verify that you have the correct username: -``` + +```bash git config --global user.name ``` To set your email address, type the following command: -``` + +```bash git config --global user.email "your_email_address@example.com" ``` To verify that you entered your email correctly, type: -``` + +```bash git config --global user.email ``` -You'll need to do this only once as you are using the `--global` option. It tells Git to always use this information for anything you do on that system. If you want to override this with a different username or email address for specific projects, you can run the command without the `--global` option when you’re in that project. +You'll need to do this only once, since you are using the `--global` option. It tells Git to always use this information for anything you do on that system. If you want to override this with a different username or email address for specific projects, you can run the command without the `--global` option when you’re in that project. ## Check your information -To view the information that you entered, type: -``` +To view the information that you entered, along with other global options, type: + +```bash git config --global --list ``` + ## Basic Git commands ### Go to the master branch to pull the latest changes from there -``` +```bash git checkout master ``` ### Download the latest changes in the project -This is for you to work on an up-to-date copy (it is important to do every time you work on a project), while you setup tracking branches. + +This is for you to work on an up-to-date copy (it is important to do this every time you start working on a project), while you set up tracking branches. You pull from remote repositories to get all the changes made by users since the last time you cloned or pulled the project. Later, you can push your local commits to the remote repositories. + +```bash +git pull REMOTE NAME-OF-BRANCH ``` -git pull REMOTE NAME-OF-BRANCH -u + +When you first clone a repository, REMOTE is typically "origin". This is where the repository came from, and it indicates the SSH or HTTPS URL of the repository on the remote server. NAME-OF-BRANCH is usually "master", but it may be any existing branch. + +### View your remote repositories + +To view your remote repositories, type: + +```bash +git remote -v ``` -(REMOTE: origin) (NAME-OF-BRANCH: could be "master" or an existing branch) ### Create a branch -Spaces won't be recognized, so you will need to use a hyphen or underscore. -``` + +To create a branch, type the following (spaces won't be recognized in the branch name, so you will need to use a hyphen or underscore): + +```bash git checkout -b NAME-OF-BRANCH ``` -### Work on a branch that has already been created -``` +### Work on an existing branch + +To switch to an existing branch, so you can work on it: + +```bash git checkout NAME-OF-BRANCH ``` ### View the changes you've made -It's important to be aware of what's happening and what's the status of your changes. -``` + +It's important to be aware of what's happening and the status of your changes. When you add, change, or delete files/folders, Git knows about it. To check the status of your changes: + +```bash git status ``` -### Add changes to commit -You'll see your changes in red when you type "git status". +### View differences + +To view the differences between your local, unstaged changes and the repository versions that you cloned or pulled, type: + +```bash +git diff +``` + +### Add and commit local changes + +You'll see your local changes in red when you type `git status`. These changes may be new, modified, or deleted files/folders. Use `git add` to stage a local file/folder for committing. Then use `git commit` to commit the staged files: + +```bash +git add FILE OR FOLDER +git commit -m "COMMENT TO DESCRIBE THE INTENTION OF THE COMMIT" ``` -git add CHANGES IN RED -git commit -m "DESCRIBE THE INTENTION OF THE COMMIT" + +### Add all changes to commit + +To add and commit all local changes in one command: + +```bash +git add . +git commit -m "COMMENT TO DESCRIBE THE INTENTION OF THE COMMIT" ``` +NOTE: **Note:** +The `.` character typically means _all_ in Git. + ### Send changes to gitlab.com -``` + +To push all local commits to the remote repository: + +```bash git push REMOTE NAME-OF-BRANCH ``` -### Delete all changes in the Git repository, but leave unstaged things +For example, to push your local commits to the _master_ branch of the _origin_ remote: + +```bash +git push origin master ``` + +### Delete all changes in the Git repository + +To delete all local changes in the repository that have not been added to the staging area, and leave unstaged files/folders, type: + +```bash git checkout . ``` -### Delete all changes in the Git repository, including untracked files -``` +### Delete all untracked changes in the Git repository + +```bash git clean -f ``` +### Unstage all changes that have been added to the staging area + +To undo the most recent add, but not committed, files/folders: + +```bash +git reset . +``` + +### Undo most recent commit + +To undo the most recent commit, type: + +```bash +git reset HEAD~1 +``` + +This leaves the files and folders unstaged in your local repository. + +CAUTION: **Warning:** +A Git commit is mostly irreversible, particularly if you already pushed it to the remote repository. Although you can undo a commit, the best option is to avoid the situation altogether. + ### Merge created branch with master branch + You need to be in the created branch. -``` + +```bash git checkout NAME-OF-BRANCH git merge master ``` ### Merge master branch with created branch + You need to be in the master branch. -``` + +```bash git checkout master git merge NAME-OF-BRANCH ``` diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index a4cc64de80d..7f2e6441f16 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -179,6 +179,8 @@ module Gitlab end def list_commits_by_oid(oids) + return [] if oids.empty? + request = Gitaly::ListCommitsByOidRequest.new(repository: @gitaly_repo, oid: oids) response = GitalyClient.call(@repository.storage, :commit_service, :list_commits_by_oid, request, timeout: GitalyClient.medium_timeout) diff --git a/lib/gitlab/metrics/subscribers/active_record.rb b/lib/gitlab/metrics/subscribers/active_record.rb index 4b3e8d0a6a0..38f119cf06d 100644 --- a/lib/gitlab/metrics/subscribers/active_record.rb +++ b/lib/gitlab/metrics/subscribers/active_record.rb @@ -20,7 +20,7 @@ module Gitlab define_histogram :gitlab_sql_duration_seconds do docstring 'SQL time' base_labels Transaction::BASE_LABELS - buckets [0.001, 0.01, 0.1, 1.0, 10.0] + buckets [0.05, 0.1, 0.25, 0.5, 1.0, 2.5, 5.0] end def current_transaction diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index f3e48083c19..9f903e96585 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -140,7 +140,7 @@ module Gitlab define_histogram :gitlab_transaction_duration_seconds do docstring 'Transaction duration' base_labels BASE_LABELS - buckets [0.001, 0.01, 0.1, 1.0, 10.0] + buckets [0.1, 0.25, 0.5, 1.0, 2.5, 5.0] end define_histogram :gitlab_transaction_allocated_memory_bytes do diff --git a/lib/gitlab/verify/batch_verifier.rb b/lib/gitlab/verify/batch_verifier.rb index 1ef369a4b67..167ba1b3149 100644 --- a/lib/gitlab/verify/batch_verifier.rb +++ b/lib/gitlab/verify/batch_verifier.rb @@ -7,13 +7,15 @@ module Gitlab @batch_size = batch_size @start = start @finish = finish + + fix_google_api_logger end # Yields a Range of IDs and a Hash of failed verifications (object => error) def run_batches(&blk) - relation.in_batches(of: batch_size, start: start, finish: finish) do |relation| # rubocop: disable Cop/InBatches - range = relation.first.id..relation.last.id - failures = run_batch(relation) + all_relation.in_batches(of: batch_size, start: start, finish: finish) do |batch| # rubocop: disable Cop/InBatches + range = batch.first.id..batch.last.id + failures = run_batch_for(batch) yield(range, failures) end @@ -29,24 +31,56 @@ module Gitlab private - def run_batch(relation) - relation.map { |upload| verify(upload) }.compact.to_h + def run_batch_for(batch) + batch.map { |upload| verify(upload) }.compact.to_h end def verify(object) + local?(object) ? verify_local(object) : verify_remote(object) + rescue => err + failure(object, err.inspect) + end + + def verify_local(object) expected = expected_checksum(object) actual = actual_checksum(object) - raise 'Checksum missing' unless expected.present? - raise 'Checksum mismatch' unless expected == actual + return failure(object, 'Checksum missing') unless expected.present? + return failure(object, 'Checksum mismatch') unless expected == actual + + success + end + # We don't calculate checksum for remote objects, so just check existence + def verify_remote(object) + return failure(object, 'Remote object does not exist') unless remote_object_exists?(object) + + success + end + + def success nil - rescue => err - [object, err] + end + + def failure(object, message) + [object, message] + end + + # It's already set to Logger::INFO, but acts as if it is set to + # Logger::DEBUG, and this fixes it... + def fix_google_api_logger + if Object.const_defined?('Google::Apis') + Google::Apis.logger.level = Logger::INFO + end end # This should return an ActiveRecord::Relation suitable for calling #in_batches on - def relation + def all_relation + raise NotImplementedError.new + end + + # Should return true if the object is stored locally + def local?(_object) raise NotImplementedError.new end @@ -59,6 +93,11 @@ module Gitlab def actual_checksum(_object) raise NotImplementedError.new end + + # Be sure to perform a hard check of the remote object (don't just check DB value) + def remote_object_exists?(object) + raise NotImplementedError.new + end end end end diff --git a/lib/gitlab/verify/job_artifacts.rb b/lib/gitlab/verify/job_artifacts.rb index 03500a61074..dbadfbde9e3 100644 --- a/lib/gitlab/verify/job_artifacts.rb +++ b/lib/gitlab/verify/job_artifacts.rb @@ -11,10 +11,14 @@ module Gitlab private - def relation + def all_relation ::Ci::JobArtifact.all end + def local?(artifact) + artifact.local_store? + end + def expected_checksum(artifact) artifact.file_sha256 end @@ -22,6 +26,10 @@ module Gitlab def actual_checksum(artifact) Digest::SHA256.file(artifact.file.path).hexdigest end + + def remote_object_exists?(artifact) + artifact.file.file.exists? + end end end end diff --git a/lib/gitlab/verify/lfs_objects.rb b/lib/gitlab/verify/lfs_objects.rb index 970e2a7b718..d3f58a73ac7 100644 --- a/lib/gitlab/verify/lfs_objects.rb +++ b/lib/gitlab/verify/lfs_objects.rb @@ -11,8 +11,12 @@ module Gitlab private - def relation - LfsObject.with_files_stored_locally + def all_relation + LfsObject.all + end + + def local?(lfs_object) + lfs_object.local_store? end def expected_checksum(lfs_object) @@ -22,6 +26,10 @@ module Gitlab def actual_checksum(lfs_object) LfsObject.calculate_oid(lfs_object.file.path) end + + def remote_object_exists?(lfs_object) + lfs_object.file.file.exists? + end end end end diff --git a/lib/gitlab/verify/rake_task.rb b/lib/gitlab/verify/rake_task.rb index dd138e6b92b..e190eaddc79 100644 --- a/lib/gitlab/verify/rake_task.rb +++ b/lib/gitlab/verify/rake_task.rb @@ -45,7 +45,7 @@ module Gitlab return unless verbose? failures.each do |object, error| - say " - #{verifier.describe(object)}: #{error.inspect}".color(:red) + say " - #{verifier.describe(object)}: #{error}".color(:red) end end end diff --git a/lib/gitlab/verify/uploads.rb b/lib/gitlab/verify/uploads.rb index 0ffa71a6d72..01f09ab8df7 100644 --- a/lib/gitlab/verify/uploads.rb +++ b/lib/gitlab/verify/uploads.rb @@ -11,8 +11,12 @@ module Gitlab private - def relation - Upload.with_files_stored_locally + def all_relation + Upload.all + end + + def local?(upload) + upload.local? end def expected_checksum(upload) @@ -22,6 +26,10 @@ module Gitlab def actual_checksum(upload) Upload.hexdigest(upload.absolute_path) end + + def remote_object_exists?(upload) + upload.build_uploader.file.exists? + end end end end diff --git a/qa/qa/runtime/browser.rb b/qa/qa/runtime/browser.rb index a12d95683af..ecd273c6db8 100644 --- a/qa/qa/runtime/browser.rb +++ b/qa/qa/runtime/browser.rb @@ -102,19 +102,7 @@ module QA def perform(&block) visit(url) - yield if block_given? - rescue - raise if block.nil? - - # RSpec examples will take care of screenshots on their own - # - unless block.binding.receiver.is_a?(RSpec::Core::ExampleGroup) - screenshot_and_save_page - end - - raise - ensure - clear! if block_given? + yield.tap { clear! } if block_given? end ## diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 5af982c7a54..ae69a362dda 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -421,6 +421,16 @@ describe Gitlab::Git::Commit, seed_helper: true do end end + describe '#batch_by_oid' do + context 'when oids is empty' do + it 'makes no Gitaly request' do + expect(Gitlab::GitalyClient).not_to receive(:call) + + described_class.batch_by_oid(repository, []) + end + end + end + shared_examples 'extracting commit signature' do context 'when the commit is signed' do let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } diff --git a/spec/lib/gitlab/verify/job_artifacts_spec.rb b/spec/lib/gitlab/verify/job_artifacts_spec.rb index ec490bdfde2..6e916a56564 100644 --- a/spec/lib/gitlab/verify/job_artifacts_spec.rb +++ b/spec/lib/gitlab/verify/job_artifacts_spec.rb @@ -21,15 +21,38 @@ describe Gitlab::Verify::JobArtifacts do FileUtils.rm_f(artifact.file.path) expect(failures.keys).to contain_exactly(artifact) - expect(failure).to be_a(Errno::ENOENT) - expect(failure.to_s).to include(artifact.file.path) + expect(failure).to include('No such file or directory') + expect(failure).to include(artifact.file.path) end it 'fails artifacts with a mismatched checksum' do File.truncate(artifact.file.path, 0) expect(failures.keys).to contain_exactly(artifact) - expect(failure.to_s).to include('Checksum mismatch') + expect(failure).to include('Checksum mismatch') + end + + context 'with remote files' do + let(:file) { double(:file) } + + before do + stub_artifacts_object_storage + artifact.update!(file_store: ObjectStorage::Store::REMOTE) + expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file) + end + + it 'passes artifacts in object storage that exist' do + expect(file).to receive(:exists?).and_return(true) + + expect(failures).to eq({}) + end + + it 'fails artifacts in object storage that do not exist' do + expect(file).to receive(:exists?).and_return(false) + + expect(failures.keys).to contain_exactly(artifact) + expect(failure).to include('Remote object does not exist') + end end end end diff --git a/spec/lib/gitlab/verify/lfs_objects_spec.rb b/spec/lib/gitlab/verify/lfs_objects_spec.rb index 0f890e2c7ce..2feaedd6f14 100644 --- a/spec/lib/gitlab/verify/lfs_objects_spec.rb +++ b/spec/lib/gitlab/verify/lfs_objects_spec.rb @@ -21,30 +21,37 @@ describe Gitlab::Verify::LfsObjects do FileUtils.rm_f(lfs_object.file.path) expect(failures.keys).to contain_exactly(lfs_object) - expect(failure).to be_a(Errno::ENOENT) - expect(failure.to_s).to include(lfs_object.file.path) + expect(failure).to include('No such file or directory') + expect(failure).to include(lfs_object.file.path) end it 'fails LFS objects with a mismatched oid' do File.truncate(lfs_object.file.path, 0) expect(failures.keys).to contain_exactly(lfs_object) - expect(failure.to_s).to include('Checksum mismatch') + expect(failure).to include('Checksum mismatch') end context 'with remote files' do + let(:file) { double(:file) } + before do stub_lfs_object_storage + lfs_object.update!(file_store: ObjectStorage::Store::REMOTE) + expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file) end - it 'skips LFS objects in object storage' do - local_failure = create(:lfs_object) - create(:lfs_object, :object_storage) + it 'passes LFS objects in object storage that exist' do + expect(file).to receive(:exists?).and_return(true) + + expect(failures).to eq({}) + end - failures = {} - described_class.new(batch_size: 10).run_batches { |_, failed| failures.merge!(failed) } + it 'fails LFS objects in object storage that do not exist' do + expect(file).to receive(:exists?).and_return(false) - expect(failures.keys).to contain_exactly(local_failure) + expect(failures.keys).to contain_exactly(lfs_object) + expect(failure).to include('Remote object does not exist') end end end diff --git a/spec/lib/gitlab/verify/uploads_spec.rb b/spec/lib/gitlab/verify/uploads_spec.rb index 85768308edc..296866d3319 100644 --- a/spec/lib/gitlab/verify/uploads_spec.rb +++ b/spec/lib/gitlab/verify/uploads_spec.rb @@ -23,37 +23,44 @@ describe Gitlab::Verify::Uploads do FileUtils.rm_f(upload.absolute_path) expect(failures.keys).to contain_exactly(upload) - expect(failure).to be_a(Errno::ENOENT) - expect(failure.to_s).to include(upload.absolute_path) + expect(failure).to include('No such file or directory') + expect(failure).to include(upload.absolute_path) end it 'fails uploads with a mismatched checksum' do upload.update!(checksum: 'something incorrect') expect(failures.keys).to contain_exactly(upload) - expect(failure.to_s).to include('Checksum mismatch') + expect(failure).to include('Checksum mismatch') end it 'fails uploads with a missing precalculated checksum' do upload.update!(checksum: '') expect(failures.keys).to contain_exactly(upload) - expect(failure.to_s).to include('Checksum missing') + expect(failure).to include('Checksum missing') end context 'with remote files' do + let(:file) { double(:file) } + before do stub_uploads_object_storage(AvatarUploader) + upload.update!(store: ObjectStorage::Store::REMOTE) + expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file) end - it 'skips uploads in object storage' do - local_failure = create(:upload) - create(:upload, :object_storage) + it 'passes uploads in object storage that exist' do + expect(file).to receive(:exists?).and_return(true) + + expect(failures).to eq({}) + end - failures = {} - described_class.new(batch_size: 10).run_batches { |_, failed| failures.merge!(failed) } + it 'fails uploads in object storage that do not exist' do + expect(file).to receive(:exists?).and_return(false) - expect(failures.keys).to contain_exactly(local_failure) + expect(failures.keys).to contain_exactly(upload) + expect(failure).to include('Remote object does not exist') end end end |