diff options
123 files changed, 2715 insertions, 1030 deletions
diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 0ee843cc604..ae9a76b9249 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -7.2.0 +8.0.0 diff --git a/app/assets/javascripts/autosave.js b/app/assets/javascripts/autosave.js index e8c59fab609..fa00a3cf386 100644 --- a/app/assets/javascripts/autosave.js +++ b/app/assets/javascripts/autosave.js @@ -53,8 +53,4 @@ export default class Autosave { return window.localStorage.removeItem(this.key); } - - dispose() { - this.field.off('input'); - } } diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index e64d5511d78..20483161033 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -30,7 +30,6 @@ export default { :render-header="false" :render-diff-file="false" :always-expanded="true" - :discussions-by-diff-order="true" /> </ul> </div> diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue index d184a76f038..ad838a32518 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -71,18 +71,13 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ diffViewType: state => state.diffs.diffViewType, diffFiles: state => state.diffs.diffFiles, }), - ...mapGetters(['isLoggedIn']), + ...mapGetters(['isLoggedIn', 'discussionsByLineCode']), lineHref() { return this.lineCode ? `#${this.lineCode}` : '#'; }, @@ -92,19 +87,24 @@ export default { this.showCommentButton && !this.isMatchLine && !this.isContextLine && - !this.isMetaLine && - !this.hasDiscussions + !this.hasDiscussions && + !this.isMetaLine ); }, + discussions() { + return this.discussionsByLineCode[this.lineCode] || []; + }, hasDiscussions() { return this.discussions.length > 0; }, shouldShowAvatarsOnGutter() { + let render = this.hasDiscussions && this.showCommentButton; + if (!this.lineType && this.linePosition === LINE_POSITION_RIGHT) { - return false; + render = false; } - return this.hasDiscussions && this.showCommentButton; + return render; }, }, methods: { @@ -189,6 +189,7 @@ export default { </button> <a v-if="lineNumber" + v-once :data-linenumber="lineNumber" :href="lineHref" > diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index cbe4551d06b..32f9516d332 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -1,17 +1,17 @@ <script> +import $ from 'jquery'; import { mapState, mapGetters, mapActions } from 'vuex'; import createFlash from '~/flash'; import { s__ } from '~/locale'; import noteForm from '../../notes/components/note_form.vue'; import { getNoteFormData } from '../store/utils'; -import autosave from '../../notes/mixins/autosave'; -import { DIFF_NOTE_TYPE } from '../constants'; +import Autosave from '../../autosave'; +import { DIFF_NOTE_TYPE, NOTE_TYPE } from '../constants'; export default { components: { noteForm, }, - mixins: [autosave], props: { diffFileHash: { type: String, @@ -41,35 +41,28 @@ export default { }, mounted() { if (this.isLoggedIn) { + const noteableData = this.getNoteableData; const keys = [ - this.noteableData.diff_head_sha, + NOTE_TYPE, + this.noteableType, + noteableData.id, + noteableData.diff_head_sha, DIFF_NOTE_TYPE, - this.noteableData.source_project_id, + noteableData.source_project_id, this.line.lineCode, ]; - this.initAutoSave(this.noteableData, keys); + this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); } }, methods: { ...mapActions('diffs', ['cancelCommentForm']), ...mapActions(['saveNote', 'refetchDiscussionById']), - handleCancelCommentForm(shouldConfirm, isDirty) { - if (shouldConfirm && isDirty) { - const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); - - // eslint-disable-next-line no-alert - if (!window.confirm(msg)) { - return; - } - } - + handleCancelCommentForm() { + this.autosave.reset(); this.cancelCommentForm({ lineCode: this.line.lineCode, }); - this.$nextTick(() => { - this.resetAutoSave(); - }); }, handleSaveNote(note) { const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash); diff --git a/app/assets/javascripts/diffs/components/diff_table_cell.vue b/app/assets/javascripts/diffs/components/diff_table_cell.vue index e8e8ddc6c5e..5962f30d9bb 100644 --- a/app/assets/javascripts/diffs/components/diff_table_cell.vue +++ b/app/assets/javascripts/diffs/components/diff_table_cell.vue @@ -67,11 +67,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapGetters(['isLoggedIn']), @@ -141,7 +136,6 @@ export default { :is-match-line="isMatchLine" :is-context-line="isContentLine" :is-meta-line="isMetaLine" - :discussions="discussions" /> </td> </template> diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index 1b5ae5e9f75..ca265dd892c 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -1,5 +1,5 @@ <script> -import { mapState } from 'vuex'; +import { mapState, mapGetters } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,22 +21,18 @@ export default { type: Number, required: true, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), + ...mapGetters(['discussionsByLineCode']), + discussions() { + return this.discussionsByLineCode[this.line.lineCode] || []; + }, className() { return this.discussions.length ? '' : 'js-temp-notes-holder'; }, - hasCommentForm() { - return this.diffLineCommentForms[this.line.lineCode]; - }, }, }; </script> @@ -57,7 +53,7 @@ export default { :discussions="discussions" /> <diff-line-note-form - v-if="hasCommentForm" + v-if="diffLineCommentForms[line.lineCode]" :diff-file-hash="diffFileHash" :line="line" :note-target-line="line" diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue index 32d65ff994f..0197a510ef1 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue @@ -33,11 +33,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, data() { return { @@ -94,7 +89,6 @@ export default { :is-bottom="isBottom" :is-hover="isHover" :show-comment-button="true" - :discussions="discussions" class="diff-line-num old_line" /> <diff-table-cell @@ -104,10 +98,10 @@ export default { :line-type="newLineType" :is-bottom="isBottom" :is-hover="isHover" - :discussions="discussions" class="diff-line-num new_line" /> <td + v-once :class="line.type" class="line_content" v-html="line.richText" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index 5f30cc57a59..9fd19b74cd7 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -20,11 +20,8 @@ export default { }, }, computed: { - ...mapGetters('diffs', [ - 'commitId', - 'shouldRenderInlineCommentRow', - 'singleDiscussionByLineCode', - ]), + ...mapGetters('diffs', ['commitId']), + ...mapGetters(['discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -38,7 +35,18 @@ export default { return window.gon.user_color_scheme; }, }, - methods: {}, + methods: { + shouldRenderCommentRow(line) { + if (this.diffLineCommentForms[line.lineCode]) return true; + + const lineDiscussions = this.discussionsByLineCode[line.lineCode]; + if (lineDiscussions === undefined) { + return false; + } + + return lineDiscussions.every(discussion => discussion.expanded); + }, + }, }; </script> @@ -57,15 +65,13 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="line.lineCode" - :discussions="singleDiscussionByLineCode(line.lineCode)" /> <inline-diff-comment-row - v-if="shouldRenderInlineCommentRow(line)" + v-if="shouldRenderCommentRow(line)" :diff-file-hash="diffFile.fileHash" :line="line" :line-index="index" :key="index" - :discussions="singleDiscussionByLineCode(line.lineCode)" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index bb9a65c83fa..cc5248c25d9 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -1,5 +1,5 @@ <script> -import { mapState } from 'vuex'; +import { mapState, mapGetters } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,51 +21,48 @@ export default { type: Number, required: true, }, - leftDiscussions: { - type: Array, - required: false, - default: () => [], - }, - rightDiscussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), + ...mapGetters(['discussionsByLineCode']), leftLineCode() { return this.line.left.lineCode; }, rightLineCode() { return this.line.right.lineCode; }, + hasDiscussion() { + const discussions = this.discussionsByLineCode; + + return discussions[this.leftLineCode] || discussions[this.rightLineCode]; + }, hasExpandedDiscussionOnLeft() { - const discussions = this.leftDiscussions; + const discussions = this.discussionsByLineCode[this.leftLineCode]; + return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasExpandedDiscussionOnRight() { - const discussions = this.rightDiscussions; + const discussions = this.discussionsByLineCode[this.rightLineCode]; + return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasAnyExpandedDiscussion() { return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight; }, shouldRenderDiscussionsOnLeft() { - return this.leftDiscussions && this.hasExpandedDiscussionOnLeft; + return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft; }, shouldRenderDiscussionsOnRight() { - return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type; - }, - showRightSideCommentForm() { - return this.line.right.type && this.diffLineCommentForms[this.rightLineCode]; + return ( + this.discussionsByLineCode[this.rightLineCode] && + this.hasExpandedDiscussionOnRight && + this.line.right.type + ); }, className() { - return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0 - ? '' - : 'js-temp-notes-holder'; + return this.hasDiscussion ? '' : 'js-temp-notes-holder'; }, }, }; @@ -83,12 +80,13 @@ export default { class="content" > <diff-discussions - v-if="leftDiscussions.length" - :discussions="leftDiscussions" + v-if="discussionsByLineCode[leftLineCode].length" + :discussions="discussionsByLineCode[leftLineCode]" /> </div> <diff-line-note-form - v-if="diffLineCommentForms[leftLineCode]" + v-if="diffLineCommentForms[leftLineCode] && + diffLineCommentForms[leftLineCode]" :diff-file-hash="diffFileHash" :line="line.left" :note-target-line="line.left" @@ -102,12 +100,13 @@ export default { class="content" > <diff-discussions - v-if="rightDiscussions.length" - :discussions="rightDiscussions" + v-if="discussionsByLineCode[rightLineCode].length" + :discussions="discussionsByLineCode[rightLineCode]" /> </div> <diff-line-note-form - v-if="showRightSideCommentForm" + v-if="diffLineCommentForms[rightLineCode] && + diffLineCommentForms[rightLineCode] && line.right.type" :diff-file-hash="diffFileHash" :line="line.right" :note-target-line="line.right" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue index d4e54c2bd00..ee5bb4d8d05 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue @@ -36,16 +36,6 @@ export default { required: false, default: false, }, - leftDiscussions: { - type: Array, - required: false, - default: () => [], - }, - rightDiscussions: { - type: Array, - required: false, - default: () => [], - }, }, data() { return { @@ -126,10 +116,10 @@ export default { :is-hover="isLeftHover" :show-comment-button="true" :diff-view-type="parallelDiffViewType" - :discussions="leftDiscussions" class="diff-line-num old_line" /> <td + v-once :id="line.left.lineCode" :class="parallelViewLeftLineType" class="line_content parallel left-side" @@ -147,10 +137,10 @@ export default { :is-hover="isRightHover" :show-comment-button="true" :diff-view-type="parallelDiffViewType" - :discussions="rightDiscussions" class="diff-line-num new_line" /> <td + v-once :id="line.right.lineCode" :class="line.right.type" class="line_content parallel right-side" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 4d97cb6d15d..32528c9e7ab 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -21,11 +21,8 @@ export default { }, }, computed: { - ...mapGetters('diffs', [ - 'commitId', - 'singleDiscussionByLineCode', - 'shouldRenderParallelCommentRow', - ]), + ...mapGetters('diffs', ['commitId']), + ...mapGetters(['discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -55,6 +52,32 @@ export default { return window.gon.user_color_scheme; }, }, + methods: { + shouldRenderCommentRow(line) { + const leftLineCode = line.left.lineCode; + const rightLineCode = line.right.lineCode; + const discussions = this.discussionsByLineCode; + const leftDiscussions = discussions[leftLineCode]; + const rightDiscussions = discussions[rightLineCode]; + const hasDiscussion = leftDiscussions || rightDiscussions; + + const hasExpandedDiscussionOnLeft = leftDiscussions + ? leftDiscussions.every(discussion => discussion.expanded) + : false; + const hasExpandedDiscussionOnRight = rightDiscussions + ? rightDiscussions.every(discussion => discussion.expanded) + : false; + + if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { + return true; + } + + const hasCommentFormOnLeft = this.diffLineCommentForms[leftLineCode]; + const hasCommentFormOnRight = this.diffLineCommentForms[rightLineCode]; + + return hasCommentFormOnLeft || hasCommentFormOnRight; + }, + }, }; </script> @@ -75,17 +98,13 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="index" - :left-discussions="singleDiscussionByLineCode(line.left.lineCode)" - :right-discussions="singleDiscussionByLineCode(line.right.lineCode)" /> <parallel-diff-comment-row - v-if="shouldRenderParallelCommentRow(line)" + v-if="shouldRenderCommentRow(line)" :key="`dcr-${index}`" :line="line" :diff-file-hash="diffFile.fileHash" :line-index="index" - :left-discussions="singleDiscussionByLineCode(line.left.lineCode)" - :right-discussions="singleDiscussionByLineCode(line.right.lineCode)" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index fc1d15b8d54..9aec117c236 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,7 +1,5 @@ import _ from 'underscore'; -import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants'; -import { getDiffRefsByLineCode } from './utils'; export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE; @@ -66,87 +64,6 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) = discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash), ) || []; -/** - * Returns an Object with discussions by their diff line code - * To avoid rendering outdated discussions on the Changes tab we should do a bunch of SHA - * comparisions. `note.position.formatter` have the current version diff refs but - * `note.original_position.formatter` will have the first version's diff refs. - * If line diff refs matches with one of them, we should render it as a discussion on Changes tab. - * - * @param {Object} diff - * @returns {Array} - */ -export const discussionsByLineCode = (state, getters, rootState, rootGetters) => { - const diffRefsByLineCode = getDiffRefsByLineCode(state.diffFiles); - - return rootGetters.discussions.reduce((acc, note) => { - const isDiffDiscussion = note.diff_discussion; - const hasLineCode = note.line_code; - const isResolvable = note.resolvable; - - if (isDiffDiscussion && hasLineCode && isResolvable) { - const diffRefs = diffRefsByLineCode[note.line_code]; - if (diffRefs) { - const refs = convertObjectPropsToCamelCase(note.position.formatter); - const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter); - - if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) { - const lineCode = note.line_code; - - if (acc[lineCode]) { - acc[lineCode].push(note); - } else { - acc[lineCode] = [note]; - } - } - } - } - - return acc; - }, {}); -}; - -export const singleDiscussionByLineCode = (state, getters) => lineCode => { - if (!lineCode) return []; - const discussions = getters.discussionsByLineCode; - return discussions[lineCode] || []; -}; - -export const shouldRenderParallelCommentRow = (state, getters) => line => { - const leftLineCode = line.left.lineCode; - const rightLineCode = line.right.lineCode; - const leftDiscussions = getters.singleDiscussionByLineCode(leftLineCode); - const rightDiscussions = getters.singleDiscussionByLineCode(rightLineCode); - const hasDiscussion = leftDiscussions.length || rightDiscussions.length; - - const hasExpandedDiscussionOnLeft = leftDiscussions.length - ? leftDiscussions.every(discussion => discussion.expanded) - : false; - const hasExpandedDiscussionOnRight = rightDiscussions.length - ? rightDiscussions.every(discussion => discussion.expanded) - : false; - - if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { - return true; - } - - const hasCommentFormOnLeft = state.diffLineCommentForms[leftLineCode]; - const hasCommentFormOnRight = state.diffLineCommentForms[rightLineCode]; - - return hasCommentFormOnLeft || hasCommentFormOnRight; -}; - -export const shouldRenderInlineCommentRow = (state, getters) => line => { - if (state.diffLineCommentForms[line.lineCode]) return true; - - const lineDiscussions = getters.singleDiscussionByLineCode(line.lineCode); - if (lineDiscussions.length === 0) { - return false; - } - - return lineDiscussions.every(discussion => discussion.expanded); -}; - // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests export const getDiffFileByHash = state => fileHash => state.diffFiles.find(file => file.fileHash === fileHash); diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 82082ac508a..d9589baa76e 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -173,24 +173,3 @@ export function trimFirstCharOfLineContent(line = {}) { return parsedLine; } - -export function getDiffRefsByLineCode(diffFiles) { - return diffFiles.reduce((acc, diffFile) => { - const { baseSha, headSha, startSha } = diffFile.diffRefs; - const { newPath, oldPath } = diffFile; - - // We can only use highlightedDiffLines to create the map of diff lines because - // highlightedDiffLines will also include every parallel diff line in it. - if (diffFile.highlightedDiffLines) { - diffFile.highlightedDiffLines.forEach(line => { - const { lineCode, oldLine, newLine } = line; - - if (lineCode) { - acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine }; - } - }); - } - - return acc; - }, {}); -} diff --git a/app/assets/javascripts/lib/utils/poll.js b/app/assets/javascripts/lib/utils/poll.js index 04a6948f1f1..198711cf427 100644 --- a/app/assets/javascripts/lib/utils/poll.js +++ b/app/assets/javascripts/lib/utils/poll.js @@ -63,7 +63,6 @@ export default class Poll { const headers = normalizeHeaders(response.headers); const pollInterval = parseInt(headers[this.intervalHeader], 10); if (pollInterval > 0 && successCodes.indexOf(response.status) !== -1 && this.canPoll) { - clearTimeout(this.timeoutID); this.timeoutID = setTimeout(() => { this.makeRequest(); }, pollInterval); diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index ad6e7cf501d..6385b75e557 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -5,20 +5,19 @@ import resolvedSvg from 'icons/_icon_status_success_solid.svg'; import mrIssueSvg from 'icons/_icon_mr_issue.svg'; import nextDiscussionSvg from 'icons/_next_discussion.svg'; import { pluralize } from '../../lib/utils/text_utility'; -import discussionNavigation from '../mixins/discussion_navigation'; +import { scrollToElement } from '../../lib/utils/common_utils'; import tooltip from '../../vue_shared/directives/tooltip'; export default { directives: { tooltip, }, - mixins: [discussionNavigation], computed: { ...mapGetters([ 'getUserData', 'getNoteableData', 'discussionCount', - 'firstUnresolvedDiscussionId', + 'unresolvedDiscussions', 'resolvedDiscussionCount', ]), isLoggedIn() { @@ -36,6 +35,11 @@ export default { resolveAllDiscussionsIssuePath() { return this.getNoteableData.create_issue_to_resolve_discussions_path; }, + firstUnresolvedDiscussionId() { + const item = this.unresolvedDiscussions[0] || {}; + + return item.id; + }, }, created() { this.resolveSvg = resolveSvg; @@ -46,10 +50,22 @@ export default { methods: { ...mapActions(['expandDiscussion']), jumpToFirstUnresolvedDiscussion() { - const diffTab = window.mrTabs.currentAction === 'diffs'; - const discussionId = this.firstUnresolvedDiscussionId(diffTab); + const discussionId = this.firstUnresolvedDiscussionId; + if (!discussionId) { + return; + } + + const el = document.querySelector(`[data-discussion-id="${discussionId}"]`); + const activeTab = window.mrTabs.currentAction; + + if (activeTab === 'commits' || activeTab === 'pipelines') { + window.mrTabs.activateTab('show'); + } - this.jumpToDiscussion(discussionId); + if (el) { + this.expandDiscussion({ discussionId }); + scrollToElement(el); + } }, }, }; diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index abcd4422d7c..26482a02e00 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -7,7 +7,7 @@ import issuableStateMixin from '../mixins/issuable_state'; import resolvable from '../mixins/resolvable'; export default { - name: 'NoteForm', + name: 'IssueNoteForm', components: { issueWarning, markdownField, diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 0fe1c16854a..bee635398b3 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -1,11 +1,11 @@ <script> +import _ from 'underscore'; import { mapActions, mapGetters } from 'vuex'; import resolveDiscussionsSvg from 'icons/_icon_mr_issue.svg'; import nextDiscussionsSvg from 'icons/_next_discussion.svg'; -import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; +import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils'; import { truncateSha } from '~/lib/utils/text_utility'; import systemNote from '~/vue_shared/components/notes/system_note.vue'; -import { s__ } from '~/locale'; import Flash from '../../flash'; import { SYSTEM_NOTE } from '../constants'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; @@ -20,7 +20,6 @@ import placeholderSystemNote from '../../vue_shared/components/notes/placeholder import autosave from '../mixins/autosave'; import noteable from '../mixins/noteable'; import resolvable from '../mixins/resolvable'; -import discussionNavigation from '../mixins/discussion_navigation'; import tooltip from '../../vue_shared/directives/tooltip'; export default { @@ -40,7 +39,7 @@ export default { directives: { tooltip, }, - mixins: [autosave, noteable, resolvable, discussionNavigation], + mixins: [autosave, noteable, resolvable], props: { discussion: { type: Object, @@ -61,11 +60,6 @@ export default { required: false, default: false, }, - discussionsByDiffOrder: { - type: Boolean, - required: false, - default: false, - }, }, data() { return { @@ -80,12 +74,7 @@ export default { 'discussionCount', 'resolvedDiscussionCount', 'allDiscussions', - 'unresolvedDiscussionsIdsByDiff', - 'unresolvedDiscussionsIdsByDate', 'unresolvedDiscussions', - 'unresolvedDiscussionsIdsOrdered', - 'nextUnresolvedDiscussionId', - 'isLastUnresolvedDiscussion', ]), transformedDiscussion() { return { @@ -136,10 +125,6 @@ export default { hasMultipleUnresolvedDiscussions() { return this.unresolvedDiscussions.length > 1; }, - showJumpToNextDiscussion() { - return this.hasMultipleUnresolvedDiscussions && - !this.isLastUnresolvedDiscussion(this.discussion.id, this.discussionsByDiffOrder); - }, shouldRenderDiffs() { const { diffDiscussion, diffFile } = this.transformedDiscussion; @@ -159,17 +144,19 @@ export default { return this.isDiffDiscussion ? '' : 'card discussion-wrapper'; }, }, - watch: { - isReplying() { - if (this.isReplying) { - this.$nextTick(() => { - // Pass an extra key to separate reply and note edit forms - this.initAutoSave(this.transformedDiscussion, ['Reply']); - }); + mounted() { + if (this.isReplying) { + this.initAutoSave(this.transformedDiscussion); + } + }, + updated() { + if (this.isReplying) { + if (!this.autosave) { + this.initAutoSave(this.transformedDiscussion); } else { - this.disposeAutoSave(); + this.setAutoSave(); } - }, + } }, created() { this.resolveDiscussionsSvg = resolveDiscussionsSvg; @@ -207,18 +194,16 @@ export default { showReplyForm() { this.isReplying = true; }, - cancelReplyForm(shouldConfirm, isDirty) { - if (shouldConfirm && isDirty) { - const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); - + cancelReplyForm(shouldConfirm) { + if (shouldConfirm && this.$refs.noteForm.isDirty) { // eslint-disable-next-line no-alert - if (!window.confirm(msg)) { + if (!window.confirm('Are you sure you want to cancel creating this comment?')) { return; } } - this.isReplying = false; this.resetAutoSave(); + this.isReplying = false; }, saveReply(noteText, form, callback) { const postData = { @@ -256,10 +241,21 @@ Please check your network connection and try again.`; }); }, jumpToNextDiscussion() { - const nextId = - this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder); + const discussionIds = this.allDiscussions.map(d => d.id); + const unresolvedIds = this.unresolvedDiscussions.map(d => d.id); + const currentIndex = discussionIds.indexOf(this.discussion.id); + const remainingAfterCurrent = discussionIds.slice(currentIndex + 1); + const nextIndex = _.findIndex(remainingAfterCurrent, id => unresolvedIds.indexOf(id) > -1); + + if (nextIndex > -1) { + const nextId = remainingAfterCurrent[nextIndex]; + const el = document.querySelector(`[data-discussion-id="${nextId}"]`); - this.jumpToDiscussion(nextId); + if (el) { + this.expandDiscussion({ discussionId: nextId }); + scrollToElement(el); + } + } }, }, }; @@ -401,7 +397,7 @@ Please check your network connection and try again.`; </a> </div> <div - v-if="showJumpToNextDiscussion" + v-if="hasMultipleUnresolvedDiscussions" class="btn-group" role="group"> <button @@ -424,8 +420,7 @@ Please check your network connection and try again.`; :is-editing="false" save-button-title="Comment" @handleFormUpdate="saveReply" - @cancelForm="cancelReplyForm" - /> + @cancelForm="cancelReplyForm" /> <note-signed-out-widget v-if="!canReply" /> </div> </div> diff --git a/app/assets/javascripts/notes/mixins/autosave.js b/app/assets/javascripts/notes/mixins/autosave.js index 4f45f912479..36cc8d5d056 100644 --- a/app/assets/javascripts/notes/mixins/autosave.js +++ b/app/assets/javascripts/notes/mixins/autosave.js @@ -4,18 +4,12 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility'; export default { methods: { - initAutoSave(noteable, extraKeys = []) { - let keys = [ + initAutoSave(noteable) { + this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), [ 'Note', - capitalizeFirstCharacter(noteable.noteable_type || noteable.noteableType), + capitalizeFirstCharacter(noteable.noteable_type), noteable.id, - ]; - - if (extraKeys) { - keys = keys.concat(extraKeys); - } - - this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); + ]); }, resetAutoSave() { this.autosave.reset(); @@ -23,8 +17,5 @@ export default { setAutoSave() { this.autosave.save(); }, - disposeAutoSave() { - this.autosave.dispose(); - }, }, }; diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js deleted file mode 100644 index f7c4deee1f8..00000000000 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ /dev/null @@ -1,29 +0,0 @@ -import { scrollToElement } from '~/lib/utils/common_utils'; - -export default { - methods: { - jumpToDiscussion(id) { - if (id) { - const activeTab = window.mrTabs.currentAction; - const selector = - activeTab === 'diffs' - ? `ul.notes[data-discussion-id="${id}"]` - : `div.discussion[data-discussion-id="${id}"]`; - const el = document.querySelector(selector); - - if (activeTab === 'commits' || activeTab === 'pipelines') { - window.mrTabs.activateTab('show'); - } - - if (el) { - this.expandDiscussion({ discussionId: id }); - - scrollToElement(el); - return true; - } - } - - return false; - }, - }, -}; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 0d8d197bf71..5c65e1c3bb5 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -28,6 +28,18 @@ export const notesById = state => return acc; }, {}); +export const discussionsByLineCode = state => + state.discussions.reduce((acc, note) => { + if (note.diff_discussion && note.line_code && note.resolvable) { + // For context about line notes: there might be multiple notes with the same line code + const items = acc[note.line_code] || []; + items.push(note); + + Object.assign(acc, { [note.line_code]: items }); + } + return acc; + }, {}); + export const noteableType = state => { const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants; @@ -70,9 +82,6 @@ export const allDiscussions = (state, getters) => { return Object.values(resolved).concat(unresolved); }; -export const allResolvableDiscussions = (state, getters) => - getters.allDiscussions.filter(d => !d.individual_note && d.resolvable); - export const resolvedDiscussionsById = state => { const map = {}; @@ -89,51 +98,6 @@ export const resolvedDiscussionsById = state => { return map; }; -// Gets Discussions IDs ordered by the date of their initial note -export const unresolvedDiscussionsIdsByDate = (state, getters) => - getters.allResolvableDiscussions - .filter(d => !d.resolved) - .sort((a, b) => { - const aDate = new Date(a.notes[0].created_at); - const bDate = new Date(b.notes[0].created_at); - - if (aDate < bDate) { - return -1; - } - - return aDate === bDate ? 0 : 1; - }) - .map(d => d.id); - -// Gets Discussions IDs ordered by their position in the diff -// -// Sorts the array of resolvable yet unresolved discussions by -// comparing file names first. If file names are the same, compares -// line numbers. -export const unresolvedDiscussionsIdsByDiff = (state, getters) => - getters.allResolvableDiscussions - .filter(d => !d.resolved) - .sort((a, b) => { - if (!a.diff_file || !b.diff_file) { - return 0; - } - - // Get file names comparison result - const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path); - - // Get the line numbers, to compare within the same file - const aLines = [a.position.formatter.new_line, a.position.formatter.old_line]; - const bLines = [b.position.formatter.new_line, b.position.formatter.old_line]; - - return filenameComparison < 0 || - (filenameComparison === 0 && - // .max() because one of them might be zero (if removed/added) - Math.max(aLines[0], aLines[1]) < Math.max(bLines[0], bLines[1])) - ? -1 - : 1; - }) - .map(d => d.id); - export const resolvedDiscussionCount = (state, getters) => { const resolvedMap = getters.resolvedDiscussionsById; @@ -150,42 +114,5 @@ export const discussionTabCounter = state => { return all.length; }; -// Returns the list of discussion IDs ordered according to given parameter -// @param {Boolean} diffOrder - is ordered by diff? -export const unresolvedDiscussionsIdsOrdered = (state, getters) => diffOrder => { - if (diffOrder) { - return getters.unresolvedDiscussionsIdsByDiff; - } - return getters.unresolvedDiscussionsIdsByDate; -}; - -// Checks if a given discussion is the last in the current order (diff or date) -// @param {Boolean} discussionId - id of the discussion -// @param {Boolean} diffOrder - is ordered by diff? -export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const lastDiscussionId = idsOrdered[idsOrdered.length - 1]; - - return lastDiscussionId === discussionId; -}; - -// Gets the ID of the discussion following the one provided, respecting order (diff or date) -// @param {Boolean} discussionId - id of the current discussion -// @param {Boolean} diffOrder - is ordered by diff? -export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const currentIndex = idsOrdered.indexOf(discussionId); - - return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0]; -}; - -// @param {Boolean} diffOrder - is ordered by diff? -export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => { - if (diffOrder) { - return getters.unresolvedDiscussionsIdsByDiff[0]; - } - return getters.unresolvedDiscussionsIdsByDate[0]; -}; - // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index e1b159142c9..ab6a95e2601 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -174,19 +174,27 @@ export default { [types.UPDATE_NOTE](state, note) { const noteObj = utils.findNoteObjectById(state.discussions, note.discussion_id); + if (noteObj.individual_note) { noteObj.notes.splice(0, 1, note); } else { const comment = utils.findNoteObjectById(noteObj.notes, note.id); - Object.assign(comment, note); + noteObj.notes.splice(noteObj.notes.indexOf(comment), 1, note); } }, [types.UPDATE_DISCUSSION](state, noteData) { const note = noteData; - const selectedDiscussion = state.discussions.find(n => n.id === note.id); + let index = 0; + + state.discussions.forEach((n, i) => { + if (n.id === note.id) { + index = i; + } + }); + note.expanded = true; // override expand flag to prevent collapse - Object.assign(selectedDiscussion, note); + state.discussions.splice(index, 1, note); }, [types.CLOSE_ISSUE](state) { @@ -207,9 +215,12 @@ export default { [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) { const discussion = utils.findNoteObjectById(state.discussions, discussionId); + const index = state.discussions.indexOf(discussion); - Object.assign(discussion, { + const discussionWithDiffLines = Object.assign({}, discussion, { truncated_diff_lines: diffLines, }); + + state.discussions.splice(index, 1, discussionWithDiffLines); }, }; diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js index c4a812c5af4..a0e096ebfaf 100644 --- a/app/assets/javascripts/notes/stores/utils.js +++ b/app/assets/javascripts/notes/stores/utils.js @@ -2,11 +2,13 @@ import AjaxCache from '~/lib/utils/ajax_cache'; const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm; -export const findNoteObjectById = (notes, id) => notes.find(n => n.id === id); +export const findNoteObjectById = (notes, id) => + notes.filter(n => n.id === id)[0]; export const getQuickActionText = note => { let text = 'Applying command'; - const quickActions = AjaxCache.get(gl.GfmAutoComplete.dataSources.commands) || []; + const quickActions = + AjaxCache.get(gl.GfmAutoComplete.dataSources.commands) || []; const executedCommands = quickActions.filter(command => { const commandRegex = new RegExp(`/${command.name}`); @@ -27,4 +29,5 @@ export const getQuickActionText = note => { export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note); -export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim(); +export const stripQuickActions = note => + note.replace(REGEX_QUICK_ACTIONS, '').trim(); diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb index 3cb9e46b548..d172aee5436 100644 --- a/app/controllers/jwt_controller.rb +++ b/app/controllers/jwt_controller.rb @@ -54,6 +54,22 @@ class JwtController < ApplicationController end def auth_params - params.permit(:service, :scope, :account, :client_id) + params.permit(:service, :account, :client_id) + .merge(additional_params) + end + + def additional_params + { scopes: scopes_param }.compact + end + + # We have to parse scope here, because Docker Client does not send an array of scopes, + # but rather a flat list and we loose second scope when being processed by Rails: + # scope=scopeA&scope=scopeB + # + # This method makes to always return an array of scopes + def scopes_param + return unless params[:scope].present? + + Array(Rack::Utils.parse_query(request.query_string)['scope']) end end diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb index ce03b2d8d1d..8a2bce6e7b5 100644 --- a/app/controllers/projects/labels_controller.rb +++ b/app/controllers/projects/labels_controller.rb @@ -160,7 +160,10 @@ class Projects::LabelsController < Projects::ApplicationController def find_labels @available_labels ||= - LabelsFinder.new(current_user, project_id: @project.id, include_ancestor_groups: params[:include_ancestor_groups]).execute + LabelsFinder.new(current_user, + project_id: @project.id, + include_ancestor_groups: params[:include_ancestor_groups], + search: params[:search]).execute end def authorize_admin_labels! diff --git a/app/controllers/projects/lfs_api_controller.rb b/app/controllers/projects/lfs_api_controller.rb index c64ccc3d473..a01351ba292 100644 --- a/app/controllers/projects/lfs_api_controller.rb +++ b/app/controllers/projects/lfs_api_controller.rb @@ -1,6 +1,8 @@ class Projects::LfsApiController < Projects::GitHttpClientController include LfsRequest + LFS_TRANSFER_CONTENT_TYPE = 'application/octet-stream'.freeze + skip_before_action :lfs_check_access!, only: [:deprecated] before_action :lfs_check_batch_operation!, only: [:batch] @@ -86,7 +88,10 @@ class Projects::LfsApiController < Projects::GitHttpClientController upload: { href: "#{project.http_url_to_repo}/gitlab-lfs/objects/#{object[:oid]}/#{object[:size]}", header: { - Authorization: request.headers['Authorization'] + Authorization: request.headers['Authorization'], + # git-lfs v2.5.0 sets the Content-Type based on the uploaded file. This + # ensures that Workhorse can intercept the request. + 'Content-Type': LFS_TRANSFER_CONTENT_TYPE }.compact } } diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb index afd1f824b32..1d05bf28438 100644 --- a/app/finders/labels_finder.rb +++ b/app/finders/labels_finder.rb @@ -14,6 +14,7 @@ class LabelsFinder < UnionFinder @skip_authorization = skip_authorization items = find_union(label_ids, Label) || Label.none items = with_title(items) + items = by_search(items) sort(items) end @@ -63,6 +64,12 @@ class LabelsFinder < UnionFinder items.where(title: title) end + def by_search(labels) + return labels unless search? + + labels.search(params[:search]) + end + # Gets redacted array of group ids # which can include the ancestors and descendants of the requested group. def group_ids_for(group) @@ -106,6 +113,10 @@ class LabelsFinder < UnionFinder params[:only_group_labels] end + def search? + params[:search].present? + end + def title params[:title] || params[:name] end diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 9c1cac22acf..a9499140f8a 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -238,15 +238,15 @@ module ApplicationSettingsHelper :signup_enabled, :terminal_max_session_time, :terms, - :throttle_unauthenticated_enabled, - :throttle_unauthenticated_requests_per_period, - :throttle_unauthenticated_period_in_seconds, - :throttle_authenticated_web_enabled, - :throttle_authenticated_web_requests_per_period, - :throttle_authenticated_web_period_in_seconds, :throttle_authenticated_api_enabled, - :throttle_authenticated_api_requests_per_period, :throttle_authenticated_api_period_in_seconds, + :throttle_authenticated_api_requests_per_period, + :throttle_authenticated_web_enabled, + :throttle_authenticated_web_period_in_seconds, + :throttle_authenticated_web_requests_per_period, + :throttle_unauthenticated_enabled, + :throttle_unauthenticated_period_in_seconds, + :throttle_unauthenticated_requests_per_period, :two_factor_grace_period, :unique_ips_limit_enabled, :unique_ips_limit_per_user, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 0eb65d1cf6c..17297769e7e 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -227,8 +227,8 @@ class ApplicationSetting < ActiveRecord::Base def self.defaults { after_sign_up_text: nil, - allow_local_requests_from_hooks_and_services: false, akismet_enabled: false, + allow_local_requests_from_hooks_and_services: false, authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand container_registry_token_expire_delay: 5, default_artifacts_expire_in: '30 days', diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 35b20bc1e0b..93bbee49c09 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -8,8 +8,6 @@ module Ci include Importable include Gitlab::Utils::StrongMemoize - MissingDependenciesError = Class.new(StandardError) - belongs_to :project, inverse_of: :builds belongs_to :runner belongs_to :trigger_request @@ -17,6 +15,10 @@ module Ci has_many :deployments, as: :deployable + RUNNER_FEATURES = { + upload_multiple_artifacts: -> (build) { build.publishes_artifacts_reports? } + }.freeze + has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id @@ -174,10 +176,6 @@ module Ci end end - before_transition any => [:running] do |build| - build.validates_dependencies! unless Feature.enabled?('ci_disable_validates_dependencies') - end - after_transition pending: :running do |build| build.ensure_metadata.update_timeout_state end @@ -581,10 +579,10 @@ module Ci options[:dependencies]&.empty? end - def validates_dependencies! - dependencies.each do |dependency| - raise MissingDependenciesError unless dependency.valid_dependency? - end + def has_valid_build_dependencies? + return true if Feature.enabled?('ci_disable_validates_dependencies') + + dependencies.all?(&:valid_dependency?) end def valid_dependency? @@ -594,6 +592,24 @@ module Ci true end + def runner_required_feature_names + strong_memoize(:runner_required_feature_names) do + RUNNER_FEATURES.select do |feature, method| + method.call(self) + end.keys + end + end + + def supported_runner?(features) + runner_required_feature_names.all? do |feature_name| + features&.dig(feature_name) + end + end + + def publishes_artifacts_reports? + options&.dig(:artifacts, :reports)&.any? + end + def hide_secrets(trace) return unless trace diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 97516079b66..8b1093655b7 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -46,7 +46,8 @@ class CommitStatus < ActiveRecord::Base api_failure: 2, stuck_or_timeout_failure: 3, runner_system_failure: 4, - missing_dependency_failure: 5 + missing_dependency_failure: 5, + runner_unsupported: 6 } ## diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 4fef615e6e3..5e39676b24b 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -35,16 +35,21 @@ module AtomicInternalId define_method("ensure_#{scope}_#{column}!") do scope_value = association(scope).reader + value = read_attribute(column) - if read_attribute(column).blank? && scope_value - scope_attrs = { scope_value.class.table_name.singularize.to_sym => scope_value } - usage = self.class.table_name.to_sym + return value unless scope_value - new_iid = InternalId.generate_next(self, scope_attrs, usage, init) - write_attribute(column, new_iid) + scope_attrs = { scope_value.class.table_name.singularize.to_sym => scope_value } + usage = self.class.table_name.to_sym + + if value.present? + InternalId.track_greatest(self, scope_attrs, usage, value, init) + else + value = InternalId.generate_next(self, scope_attrs, usage, init) + write_attribute(column, value) end - read_attribute(column) + value end end end diff --git a/app/models/concerns/label_eventable.rb b/app/models/concerns/label_eventable.rb new file mode 100644 index 00000000000..d22d93448e4 --- /dev/null +++ b/app/models/concerns/label_eventable.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +# == LabelEventable concern +# +# Contains functionality related to objects that support adding/removing labels. +# +# This concern is not used yet, it will be used for: +# https://gitlab.com/gitlab-org/gitlab-ce/issues/48483 + +module LabelEventable + extend ActiveSupport::Concern + + included do + has_many :resource_label_events + end +end diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index f50f28deffe..e5d0f94073c 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -1,6 +1,9 @@ # An InternalId is a strictly monotone sequence of integers # generated for a given scope and usage. # +# The monotone sequence may be broken if an ID is explicitly provided +# to `.track_greatest_and_save!` or `#track_greatest`. +# # For example, issues use their project to scope internal ids: # In that sense, scope is "project" and usage is "issues". # Generated internal ids for an issue are unique per project. @@ -25,13 +28,34 @@ class InternalId < ActiveRecord::Base # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL). # As such, the increment is atomic and safe to be called concurrently. def increment_and_save! + update_and_save { self.last_value = (last_value || 0) + 1 } + end + + # Increments #last_value with new_value if it is greater than the current, + # and saves the record + # + # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL). + # As such, the increment is atomic and safe to be called concurrently. + def track_greatest_and_save!(new_value) + update_and_save { self.last_value = [last_value || 0, new_value].max } + end + + private + + def update_and_save(&block) lock! - self.last_value = (last_value || 0) + 1 + yield save! last_value end class << self + def track_greatest(subject, scope, usage, new_value, init) + return new_value unless available? + + InternalIdGenerator.new(subject, scope, usage, init).track_greatest(new_value) + end + def generate_next(subject, scope, usage, init) # Shortcut if `internal_ids` table is not available (yet) # This can be the case in other (unrelated) migration specs @@ -94,6 +118,16 @@ class InternalId < ActiveRecord::Base end end + # Create a record in internal_ids if one does not yet exist + # and set its new_value if it is higher than the current last_value + # + # Note this will acquire a ROW SHARE lock on the InternalId record + def track_greatest(new_value) + subject.transaction do + (lookup || create_record).track_greatest_and_save!(new_value) + end + end + private # Retrieve InternalId record for (project, usage) combination, if it exists diff --git a/app/models/issue.rb b/app/models/issue.rb index 4715d942c8d..e4ed06f9a69 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -12,6 +12,7 @@ class Issue < ActiveRecord::Base include TimeTrackable include ThrottledTouch include IgnorableColumn + include LabelEventable ignore_column :assignee_id, :branch_name, :deleted_at diff --git a/app/models/label.rb b/app/models/label.rb index 7bbcaa121ca..7b08547fa6e 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -2,6 +2,7 @@ class Label < ActiveRecord::Base include CacheMarkdownField include Referable include Subscribable + include Gitlab::SQL::Pattern # Represents a "No Label" state used for filtering Issues and Merge # Requests that have no label assigned. @@ -103,6 +104,17 @@ class Label < ActiveRecord::Base nil end + # Searches for labels with a matching title or description. + # + # This method uses ILIKE on PostgreSQL and LIKE on MySQL. + # + # query - The search query as a String. + # + # Returns an ActiveRecord::Relation. + def self.search(query) + fuzzy_search(query, [:title, :description]) + end + def open_issues_count(user = nil) issues_count(user, state: 'opened') end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b4090fd8baf..124ff6b3f91 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -10,6 +10,7 @@ class MergeRequest < ActiveRecord::Base include EachBatch include ThrottledTouch include Gitlab::Utils::StrongMemoize + include LabelEventable ignore_column :locked_at, :ref_fetched, diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb new file mode 100644 index 00000000000..42c255fcd1e --- /dev/null +++ b/app/models/resource_label_event.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +# This model is not used yet, it will be used for: +# https://gitlab.com/gitlab-org/gitlab-ce/issues/48483 +class ResourceLabelEvent < ActiveRecord::Base + belongs_to :user + belongs_to :issue + belongs_to :merge_request + belongs_to :label + + validates :user, presence: true, on: :create + validates :label, presence: true, on: :create + validate :exactly_one_issuable + + enum action: { + add: 1, + remove: 2 + } + + def self.issuable_columns + %i(issue_id merge_request_id).freeze + end + + def issuable + issue || merge_request + end + + private + + def exactly_one_issuable + if self.class.issuable_columns.count { |attr| self[attr] } != 1 + errors.add(:base, "Exactly one of #{self.class.issuable_columns.join(', ')} is required") + end + end +end diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb index 3a9088cfcb8..a08f34e2335 100644 --- a/app/presenters/commit_status_presenter.rb +++ b/app/presenters/commit_status_presenter.rb @@ -2,17 +2,19 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated CALLOUT_FAILURE_MESSAGES = { - unknown_failure: 'There is an unknown failure, please try again', - api_failure: 'There has been an API failure, please try again', - stuck_or_timeout_failure: 'There has been a timeout failure or the job got stuck. Check your timeout limits or try again', - runner_system_failure: 'There has been a runner system failure, please try again', - missing_dependency_failure: 'There has been a missing dependency failure' + unknown_failure: 'There is an unknown failure, please try again', + script_failure: nil, + api_failure: 'There has been an API failure, please try again', + stuck_or_timeout_failure: 'There has been a timeout failure or the job got stuck. Check your timeout limits or try again', + runner_system_failure: 'There has been a runner system failure, please try again', + missing_dependency_failure: 'There has been a missing dependency failure', + runner_unsupported: 'Your runner is outdated, please upgrade your runner' }.freeze presents :build def callout_failure_message - CALLOUT_FAILURE_MESSAGES[failure_reason.to_sym] + CALLOUT_FAILURE_MESSAGES.fetch(failure_reason.to_sym) end def recoverable? diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index 6f95e6f9ca1..b8321037fa5 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -6,7 +6,6 @@ class DiscussionEntity < Grape::Entity expose :id, :reply_id expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } - expose :original_position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } expose :line_code, if: -> (d, _) { d.diff_discussion? } expose :expanded?, as: :expanded expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? } diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 81857d0cb4c..893b37b831a 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -9,11 +9,11 @@ module Auth return error('UNAVAILABLE', status: 404, message: 'registry not enabled') unless registry.enabled - unless scope || current_user || project + unless scopes.any? || current_user || project return error('DENIED', status: 403, message: 'access forbidden') end - { token: authorized_token(scope).encoded } + { token: authorized_token(*scopes).encoded } end def self.full_access_token(*names) @@ -47,10 +47,12 @@ module Auth end end - def scope - return unless params[:scope] + def scopes + return [] unless params[:scopes] - @scope ||= process_scope(params[:scope]) + @scopes ||= params[:scopes].map do |scope| + process_scope(scope) + end.compact end def process_scope(scope) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index f7ccec3a700..11f85627faf 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -41,16 +41,10 @@ module Ci begin # In case when 2 runners try to assign the same build, second runner will be declined # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method. - begin - build.runner_id = runner.id - build.runner_session_attributes = params[:session] if params[:session].present? - - build.run! + if assign_runner!(build, params) register_success(build) return Result.new(build, true) # rubocop:disable Cop/AvoidReturnFromBlocks - rescue Ci::Build::MissingDependenciesError - build.drop!(:missing_dependency_failure) end rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError # We are looping to find another build that is not conflicting @@ -72,6 +66,24 @@ module Ci private + def assign_runner!(build, params) + build.runner_id = runner.id + build.runner_session_attributes = params[:session] if params[:session].present? + + unless build.has_valid_build_dependencies? + build.drop!(:missing_dependency_failure) + return false + end + + unless build.supported_runner?(params.dig(:info, :features)) + build.drop!(:runner_unsupported) + return false + end + + build.run! + true + end + def builds_for_shared_runner new_builds. # don't run projects which have not enabled shared runners and builds diff --git a/app/services/clusters/applications/check_installation_progress_service.rb b/app/services/clusters/applications/check_installation_progress_service.rb index a1165b0ab28..35f5cff0e0c 100644 --- a/app/services/clusters/applications/check_installation_progress_service.rb +++ b/app/services/clusters/applications/check_installation_progress_service.rb @@ -35,7 +35,7 @@ module Clusters def check_timeout if timeouted? begin - app.make_errored!('Installation timeouted') + app.make_errored!('Installation timed out') ensure remove_installation_pod end diff --git a/app/services/projects/create_from_template_service.rb b/app/services/projects/create_from_template_service.rb index f5c48e56880..8306d43ca7c 100644 --- a/app/services/projects/create_from_template_service.rb +++ b/app/services/projects/create_from_template_service.rb @@ -2,21 +2,27 @@ module Projects class CreateFromTemplateService < BaseService + include Gitlab::Utils::StrongMemoize + def initialize(user, params) @current_user, @params = user, params.dup end def execute - template_name = params.delete(:template_name) - file = Gitlab::ProjectTemplate.find(template_name).file + file = Gitlab::ProjectTemplate.find(template_name)&.file override_params = params.dup params[:file] = file GitlabProjectsImportService.new(current_user, params, override_params).execute - ensure file&.close end + + def template_name + strong_memoize(:template_name) do + params.delete(:template_name).presence + end + end end end diff --git a/app/services/projects/gitlab_projects_import_service.rb b/app/services/projects/gitlab_projects_import_service.rb index bc6e9caebb8..615dccc4685 100644 --- a/app/services/projects/gitlab_projects_import_service.rb +++ b/app/services/projects/gitlab_projects_import_service.rb @@ -5,6 +5,9 @@ # The latter will under the hood just import an archive supplied by GitLab. module Projects class GitlabProjectsImportService + include Gitlab::Utils::StrongMemoize + include Gitlab::TemplateHelper + attr_reader :current_user, :params def initialize(user, import_params, override_params = nil) @@ -12,39 +15,17 @@ module Projects end def execute - FileUtils.mkdir_p(File.dirname(import_upload_path)) - - file = params.delete(:file) - FileUtils.copy_entry(file.path, import_upload_path) - - @overwrite = params.delete(:overwrite) - data = {} - data[:override_params] = @override_params if @override_params - - if overwrite_project? - data[:original_path] = params[:path] - params[:path] += "-#{tmp_filename}" - end + prepare_template_environment(template_file&.path) - params[:import_type] = 'gitlab_project' - params[:import_source] = import_upload_path - params[:import_data] = { data: data } if data.present? + prepare_import_params ::Projects::CreateService.new(current_user, params).execute end private - def import_upload_path - @import_upload_path ||= Gitlab::ImportExport.import_upload_path(filename: tmp_filename) - end - - def tmp_filename - SecureRandom.hex - end - def overwrite_project? - @overwrite && project_with_same_full_path? + overwrite? && project_with_same_full_path? end def project_with_same_full_path? @@ -52,7 +33,38 @@ module Projects end def current_namespace - @current_namespace ||= Namespace.find_by(id: params[:namespace_id]) + strong_memoize(:current_namespace) do + Namespace.find_by(id: params[:namespace_id]) + end + end + + def overwrite? + strong_memoize(:overwrite) do + params.delete(:overwrite) + end + end + + def template_file + strong_memoize(:template_file) do + params.delete(:file) + end + end + + def prepare_import_params + data = {} + data[:override_params] = @override_params if @override_params + + if overwrite_project? + data[:original_path] = params[:path] + params[:path] += "-#{tmp_filename}" + end + + if template_file + params[:import_type] = 'gitlab_project' + params[:import_source] = import_upload_path + end + + params[:import_data] = { data: data } if data.present? end end end diff --git a/app/services/resource_events/change_labels_service.rb b/app/services/resource_events/change_labels_service.rb new file mode 100644 index 00000000000..8edb0ddb3ed --- /dev/null +++ b/app/services/resource_events/change_labels_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +# This service is not used yet, it will be used for: +# https://gitlab.com/gitlab-org/gitlab-ce/issues/48483 +module ResourceEvents + class ChangeLabelsService + attr_reader :resource, :user + + def initialize(resource, user) + @resource, @user = resource, user + end + + def execute(added_labels: [], removed_labels: []) + label_hash = { + resource_column(resource) => resource.id, + user_id: user.id, + created_at: Time.now + } + + labels = added_labels.map do |label| + label_hash.merge(label_id: label.id, action: ResourceLabelEvent.actions['add']) + end + labels += removed_labels.map do |label| + label_hash.merge(label_id: label.id, action: ResourceLabelEvent.actions['remove']) + end + + Gitlab::Database.bulk_insert(ResourceLabelEvent.table_name, labels) + end + + private + + def resource_column(resource) + case resource + when Issue + :issue_id + when MergeRequest + :merge_request_id + else + raise ArgumentError, "Unknown resource type #{resource.class.name}" + end + end + end +end diff --git a/app/views/projects/labels/index.html.haml b/app/views/projects/labels/index.html.haml index fb5b0fc15c9..768ce9bd103 100644 --- a/app/views/projects/labels/index.html.haml +++ b/app/views/projects/labels/index.html.haml @@ -2,32 +2,45 @@ - page_title "Labels" - can_admin_label = can?(current_user, :admin_label, @project) - hide_class = '' +- search = params[:search] - if can_admin_label - content_for(:header_content) do .nav-controls = link_to _('New label'), new_project_label_path(@project), class: "btn btn-new" -- if @labels.exists? || @prioritized_labels.exists? +- if @labels.exists? || @prioritized_labels.exists? || search.present? #promote-label-modal %div{ class: container_class } .top-area.adjust .nav-text = _('Labels can be applied to issues and merge requests.') - - if can_admin_label - = _('Star a label to make it a priority label. Order the prioritized labels to change their relative priority, by dragging.') - .labels-container.prepend-top-5 + .nav-controls + = form_tag project_labels_path(@project), method: :get do + .input-group + = search_field_tag :search, params[:search], { placeholder: _('Filter'), id: 'label-search', class: 'form-control search-text-input input-short', spellcheck: false } + %span.input-group-append + %button.btn.btn-default{ type: "submit", "aria-label" => _('Submit search') } + = icon("search") + + .labels-container.prepend-top-10 - if can_admin_label + - if search.blank? + %p.text-muted + = _('Star a label to make it a priority label. Order the prioritized labels to change their relative priority, by dragging.') -# Only show it in the first page - hide = @available_labels.empty? || (params[:page].present? && params[:page] != '1') .prioritized-labels{ class: ('hide' if hide) } %h5.prepend-top-10= _('Prioritized Labels') .content-list.manage-labels-list.js-prioritized-labels{ "data-url" => set_priorities_project_labels_path(@project) } - #js-priority-labels-empty-state.priority-labels-empty-state{ class: "#{'hidden' unless @prioritized_labels.empty?}" } + #js-priority-labels-empty-state.priority-labels-empty-state{ class: "#{'hidden' unless @prioritized_labels.empty? && search.blank?}" } = render 'shared/empty_states/priority_labels' - if @prioritized_labels.present? = render partial: 'shared/label', subject: @project, collection: @prioritized_labels, as: :label, locals: { force_priority: true } + - elsif search.present? + .nothing-here-block + = _('No prioritised labels with such name or description') - if @labels.present? .other-labels @@ -36,6 +49,18 @@ .content-list.manage-labels-list.js-other-labels = render partial: 'shared/label', subject: @project, collection: @labels, as: :label = paginate @labels, theme: 'gitlab' + - elsif search.present? + .other-labels + - if @available_labels.any? + %h5 + = _('Other Labels') + .nothing-here-block + = _('No other labels with such name or description') + - else + .nothing-here-block + = _('No labels with such name or description') + + - else = render 'shared/empty_states/labels' diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index 8c64c513c74..82189a3c9f5 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -7,9 +7,9 @@ class RepositoryImportWorker include ProjectImportOptions def perform(project_id) - project = Project.find(project_id) + @project = Project.find(project_id) - return unless start_import(project) + return unless start_import Gitlab::Metrics.add_event(:import_repository) @@ -21,7 +21,7 @@ class RepositoryImportWorker return if service.async? if result[:status] == :error - fail_import(project, result[:message]) if project.gitlab_project_import? + fail_import(result[:message]) if template_import? raise result[:message] end @@ -31,14 +31,20 @@ class RepositoryImportWorker private - def start_import(project) + attr_reader :project + + def start_import return true if start(project) Rails.logger.info("Project #{project.full_path} was in inconsistent state (#{project.import_status}) while importing.") false end - def fail_import(project, message) + def fail_import(message) project.mark_import_as_failed(message) end + + def template_import? + project.gitlab_project_import? + end end diff --git a/changelogs/unreleased/1756-set-iid-via-api.yml b/changelogs/unreleased/1756-set-iid-via-api.yml new file mode 100644 index 00000000000..680a9464ab4 --- /dev/null +++ b/changelogs/unreleased/1756-set-iid-via-api.yml @@ -0,0 +1,5 @@ +--- +title: Allow issues API to receive an internal ID (iid) on create +merge_request: 20626 +author: Jamie Schembri +type: fixed diff --git a/changelogs/unreleased/34572-ssh-certificates.yml b/changelogs/unreleased/34572-ssh-certificates.yml new file mode 100644 index 00000000000..76a08a188de --- /dev/null +++ b/changelogs/unreleased/34572-ssh-certificates.yml @@ -0,0 +1,5 @@ +--- +title: Add support for SSH certificate authentication +merge_request: 19911 +author: Ævar Arnfjörð Bjarmason +type: added diff --git a/changelogs/unreleased/dz-labels-search.yml b/changelogs/unreleased/dz-labels-search.yml new file mode 100644 index 00000000000..49c1b6c1a86 --- /dev/null +++ b/changelogs/unreleased/dz-labels-search.yml @@ -0,0 +1,5 @@ +--- +title: Search for labels by title or description on project labels page +merge_request: 20749 +author: +type: added diff --git a/changelogs/unreleased/fix-multiple-scopes.yml b/changelogs/unreleased/fix-multiple-scopes.yml new file mode 100644 index 00000000000..24e5172d9a1 --- /dev/null +++ b/changelogs/unreleased/fix-multiple-scopes.yml @@ -0,0 +1,5 @@ +--- +title: Support multiple scopes when authing container registry scopes +merge_request: 20617 +author: +type: fixed diff --git a/changelogs/unreleased/floating-avarage-commit-numbers.yml b/changelogs/unreleased/floating-avarage-commit-numbers.yml new file mode 100644 index 00000000000..7f91ab16af4 --- /dev/null +++ b/changelogs/unreleased/floating-avarage-commit-numbers.yml @@ -0,0 +1,5 @@ +--- +title: Show one digit after dot in commit_per_day value in charts page. +merge_request: +author: msdundar +type: changed diff --git a/changelogs/unreleased/jprovazn-resource-events.yml b/changelogs/unreleased/jprovazn-resource-events.yml new file mode 100644 index 00000000000..05643150f16 --- /dev/null +++ b/changelogs/unreleased/jprovazn-resource-events.yml @@ -0,0 +1,5 @@ +--- +title: Add new model for tracking label events. +merge_request: +author: +type: added diff --git a/changelogs/unreleased/mk-add-local-project-uploads-cleanup-task.yml b/changelogs/unreleased/mk-add-local-project-uploads-cleanup-task.yml new file mode 100644 index 00000000000..9d38b353a41 --- /dev/null +++ b/changelogs/unreleased/mk-add-local-project-uploads-cleanup-task.yml @@ -0,0 +1,5 @@ +--- +title: Add local project uploads cleanup task +merge_request: 20863 +author: +type: added diff --git a/changelogs/unreleased/rails5-fix-flaky-spec-user-uses-shortcuts.yml b/changelogs/unreleased/rails5-fix-flaky-spec-user-uses-shortcuts.yml new file mode 100644 index 00000000000..5f2504c604d --- /dev/null +++ b/changelogs/unreleased/rails5-fix-flaky-spec-user-uses-shortcuts.yml @@ -0,0 +1,5 @@ +--- +title: 'Rails5: fix flaky spec' +merge_request: 20953 +author: Jasper Maes +type: fixed diff --git a/changelogs/unreleased/runner-features.yml b/changelogs/unreleased/runner-features.yml new file mode 100644 index 00000000000..c5e0fff5a18 --- /dev/null +++ b/changelogs/unreleased/runner-features.yml @@ -0,0 +1,5 @@ +--- +title: Verify runner feature set +merge_request: 20664 +author: +type: added diff --git a/changelogs/unreleased/sh-lfs-fix-content-type.yml b/changelogs/unreleased/sh-lfs-fix-content-type.yml new file mode 100644 index 00000000000..a839be9b3ae --- /dev/null +++ b/changelogs/unreleased/sh-lfs-fix-content-type.yml @@ -0,0 +1,5 @@ +--- +title: Fix LFS uploads not working with git-lfs 2.5.0 +merge_request: 20923 +author: +type: fixed diff --git a/db/migrate/20180726172057_create_resource_label_events.rb b/db/migrate/20180726172057_create_resource_label_events.rb new file mode 100644 index 00000000000..2ef7078d898 --- /dev/null +++ b/db/migrate/20180726172057_create_resource_label_events.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreateResourceLabelEvents < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :resource_label_events, id: :bigserial do |t| + t.integer :action, null: false + t.references :issue, null: true, index: true, foreign_key: { on_delete: :cascade } + t.references :merge_request, null: true, index: true, foreign_key: { on_delete: :cascade } + t.references :label, index: true, foreign_key: { on_delete: :nullify } + t.references :user, index: true, foreign_key: { on_delete: :nullify } + t.datetime_with_timezone :created_at, null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 1d6896b7669..905786172a2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180722103201) do +ActiveRecord::Schema.define(version: 20180726172057) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1788,6 +1788,20 @@ ActiveRecord::Schema.define(version: 20180722103201) do add_index "remote_mirrors", ["last_successful_update_at"], name: "index_remote_mirrors_on_last_successful_update_at", using: :btree add_index "remote_mirrors", ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree + create_table "resource_label_events", id: :bigserial, force: :cascade do |t| + t.integer "action", null: false + t.integer "issue_id" + t.integer "merge_request_id" + t.integer "label_id" + t.integer "user_id" + t.datetime_with_timezone "created_at", null: false + end + + add_index "resource_label_events", ["issue_id"], name: "index_resource_label_events_on_issue_id", using: :btree + add_index "resource_label_events", ["label_id"], name: "index_resource_label_events_on_label_id", using: :btree + add_index "resource_label_events", ["merge_request_id"], name: "index_resource_label_events_on_merge_request_id", using: :btree + add_index "resource_label_events", ["user_id"], name: "index_resource_label_events_on_user_id", using: :btree + create_table "routes", force: :cascade do |t| t.integer "source_id", null: false t.string "source_type", null: false @@ -2338,6 +2352,10 @@ ActiveRecord::Schema.define(version: 20180722103201) do add_foreign_key "push_event_payloads", "events", name: "fk_36c74129da", on_delete: :cascade add_foreign_key "releases", "projects", name: "fk_47fe2a0596", on_delete: :cascade add_foreign_key "remote_mirrors", "projects", on_delete: :cascade + add_foreign_key "resource_label_events", "issues", on_delete: :cascade + add_foreign_key "resource_label_events", "labels", on_delete: :nullify + add_foreign_key "resource_label_events", "merge_requests", on_delete: :cascade + add_foreign_key "resource_label_events", "users", on_delete: :nullify add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade add_foreign_key "snippets", "projects", name: "fk_be41fd4bb7", on_delete: :cascade add_foreign_key "subscriptions", "projects", on_delete: :cascade diff --git a/doc/administration/index.md b/doc/administration/index.md index 88190b2df5f..112d14652af 100644 --- a/doc/administration/index.md +++ b/doc/administration/index.md @@ -106,6 +106,7 @@ created in snippets, wikis, and repos. - [Gitaly](gitaly/index.md): Configuring Gitaly, GitLab's Git repository storage service. - [Default labels](../user/admin_area/labels.html): Create labels that will be automatically added to every new project. - [Restrict the use of public or internal projects](../public_access/public_access.md#restricting-the-use-of-public-or-internal-projects): Restrict the use of visibility levels for users when they create a project or a snippet. +- [Custom project templates](https://docs.gitlab.com/ee/user/admin_area/custom_project_templates.html): Configure a set of projects to be used as custom templates when creating a new project. **[PREMIUM ONLY]** ### Repository settings diff --git a/doc/administration/operations/fast_ssh_key_lookup.md b/doc/administration/operations/fast_ssh_key_lookup.md index 89331238ce4..752a2774bd7 100644 --- a/doc/administration/operations/fast_ssh_key_lookup.md +++ b/doc/administration/operations/fast_ssh_key_lookup.md @@ -1,3 +1,10 @@ +# Consider using SSH certificates instead of, or in addition to this + +This document describes a drop-in replacement for the +`authorized_keys` file for normal (non-deploy key) users. Consider +using [ssh certificates](ssh_certificates.md), they are even faster, +but are not is not a drop-in replacement. + # Fast lookup of authorized SSH keys in the database > [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/1631) in diff --git a/doc/administration/operations/index.md b/doc/administration/operations/index.md index 5655b7efec6..e9cad99c4b0 100644 --- a/doc/administration/operations/index.md +++ b/doc/administration/operations/index.md @@ -14,4 +14,7 @@ that to prioritize important jobs. - [Sidekiq MemoryKiller](sidekiq_memory_killer.md): Configure Sidekiq MemoryKiller to restart Sidekiq. - [Unicorn](unicorn.md): Understand Unicorn and unicorn-worker-killer. -- [Speed up SSH operations](fast_ssh_key_lookup.md): Authorize SSH users via a fast, indexed lookup to the GitLab database. +- Speed up SSH operations by [Authorizing SSH users via a fast, +indexed lookup to the GitLab database](fast_ssh_key_lookup.md), and/or +by [doing away with user SSH keys stored on GitLab entirely in favor +of SSH certificates](ssh_certificates.md). diff --git a/doc/administration/operations/ssh_certificates.md b/doc/administration/operations/ssh_certificates.md new file mode 100644 index 00000000000..8968afba01b --- /dev/null +++ b/doc/administration/operations/ssh_certificates.md @@ -0,0 +1,165 @@ +# User lookup via OpenSSH's AuthorizedPrincipalsCommand + +> [Available in](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19911) GitLab +> Community Edition 11.2. + +GitLab's default SSH authentication requires users to upload their ssh +public keys before they can use the SSH transport. + +In centralized (e.g. corporate) environments this can be a hassle +operationally, particularly if the SSH keys are temporary keys issued +to the user, e.g. ones that expire 24 hours after issuing. + +In such setups some external automated process is needed to constantly +upload the new keys to GitLab. + +> **Warning:** OpenSSH version 6.9+ is required because that version +introduced the `AuthorizedPrincipalsCommand` configuration option. If +using CentOS 6, you can [follow these +instructions](fast_ssh_key_lookup.html#compiling-a-custom-version-of-openssh-for-centos-6) +to compile an up-to-date version. + +## Why use OpenSSH certificates? + +By using OpenSSH certificates all the information about what user on +GitLab owns the key is encoded in the key itself, and OpenSSH itself +guarantees that users can't fake this, since they'd need to have +access to the private CA signing key. + +When correctly set up, this does away with the requirement of +uploading user SSH keys to GitLab entirely. + +## Setting up SSH certificate lookup via GitLab Shell + +How to fully setup SSH certificates is outside the scope of this +document. See [OpenSSH's +PROTOCOL.certkeys](https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.certkeys?annotate=HEAD) +for how it works, and e.g. [RedHat's documentation about +it](https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/sec-using_openssh_certificate_authentication). + +We assume that you already have SSH certificates set up, and have +added the `TrustedUserCAKeys` of your CA to your `sshd_config`, e.g.: + +``` +TrustedUserCAKeys /etc/security/mycompany_user_ca.pub +``` + +Usually `TrustedUserCAKeys` would not be scoped under a `Match User +git` in such a setup, since it would also be used for system logins to +the GitLab server itself, but your setup may vary. If the CA is only +used for GitLab consider putting this in the `Match User git` section +(described below). + +The SSH certificates being issued by that CA **MUST** have a "key id" +corresponding to that user's username on GitLab, e.g. (some output +omitted for brevity): + +``` +$ ssh-add -L | grep cert | ssh-keygen -L -f - +(stdin):1: + Type: ssh-rsa-cert-v01@openssh.com user certificate + Public key: RSA-CERT SHA256:[...] + Signing CA: RSA SHA256:[...] + Key ID: "aearnfjord" + Serial: 8289829611021396489 + Valid: from 2018-07-18T09:49:00 to 2018-07-19T09:50:34 + Principals: + sshUsers + [...] + [...] +``` + +Technically that's not strictly true, e.g. it could be +`prod-aearnfjord` if it's a SSH certificate you'd normally log in to +servers as the `prod-aearnfjord` user, but then you must specify your +own `AuthorizedPrincipalsCommand` to do that mapping instead of using +our provided default. + +The important part is that the `AuthorizedPrincipalsCommand` must be +able to map from the "key id" to a GitLab username in some way, the +default command we ship assumes there's a 1=1 mapping between the two, +since the whole point of this is to allow us to extract a GitLab +username from the key itself, instead of relying on something like the +default public key to username mapping. + +Then, in your `sshd_config` set up `AuthorizedPrincipalsCommand` for +the `git` user. Hopefully you can use the default one shipped with +GitLab: + +``` +Match User git + AuthorizedPrincipalsCommandUser root + AuthorizedPrincipalsCommand /opt/gitlab/embedded/service/gitlab-shell/bin/gitlab-shell-authorized-principals-check %i sshUsers +``` + +This command will emit output that looks something like: + +``` +command="/opt/gitlab/embedded/service/gitlab-shell/bin/gitlab-shell username-{KEY_ID}",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty {PRINCIPAL} +``` + +Where `{KEY_ID}` is the `%i` argument passed to the script +(e.g. `aeanfjord`), and `{PRINCIPAL}` is the principal passed to it +(e.g. `sshUsers`). + +You will need to customize the `sshUsers` part of that. It should be +some principal that's guaranteed to be part of the key for all users +who can log in to GitLab, or you must provide a list of principals, +one of which is going to be present for the user, e.g.: + +``` + [...] + AuthorizedPrincipalsCommand /opt/gitlab/embedded/service/gitlab-shell/bin/gitlab-shell-authorized-principals-check %i sshUsers windowsUsers +``` + +## Principals and security + +You can supply as many principals as you want, these will be turned +into multiple lines of `authorized_keys` output, as described in the +`AuthorizedPrincipalsFile` documentation in `sshd_config(5)`. + +Normally when using the `AuthorizedKeysCommand` with OpenSSH the +principal is some "group" that's allowed to log into that +server. However with GitLab it's only used to appease OpenSSH's +requirement for it, we effectively only care about the "key id" being +correct. Once that's extracted GitLab will enforce its own ACLs for +that user (e.g. what projects the user can access). + +So it's OK to e.g. be overly generous in what you accept, since if the +user e.g. has no access to GitLab at all it'll just error out with a +message about this being an invalid user. + +## Interaction with the `authorized_keys` file + +SSH certificates can be used in conjunction with the `authorized_keys` +file, and if setup as configured above the `authorized_keys` file will +still serve as a fallback. + +This is because if the `AuthorizedPrincipalsCommand` can't +authenticate the user, OpenSSH will fall back on +`~/.ssh/authorized_keys` (or the `AuthorizedKeysCommand`). + +Therefore there may still be a reason to use the ["Fast lookup of +authorized SSH keys in the database"](fast_ssh_key_lookup.html) method +in conjunction with this. Since you'll be using SSH certificates for +all your normal users, and relying on the `~/.ssh/authorized_keys` +fallback for deploy keys, if you make use of those. + +But you may find that there's no reason to do that, since all your +normal users will use the fast `AuthorizedPrincipalsCommand` path, and +only automated deployment key access will fall back on +`~/.ssh/authorized_keys`, or that you have a lot more keys for normal +users (especially if they're renewed) than you have deploy keys. + +## Other security caveats + +Users can still bypass SSH certificate authentication by manually +uploading an SSH public key to their profile, relying on the +`~/.ssh/authorized_keys` fallback to authenticate it. There's +currently no feature to prevent this, [but there's an open request for +adding it](https://gitlab.com/gitlab-org/gitlab-ce/issues/49218). + +Such a restriction can currently be hacked in by e.g. providing a +custom `AuthorizedKeysCommand` which checks if the discovered key-ID +returned from `gitlab-shell-authorized-keys-check` is a deploy key or +not (all non-deploy keys should be refused). diff --git a/doc/api/issues.md b/doc/api/issues.md index 92fb3e9c307..103eaa5655f 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -463,6 +463,7 @@ POST /projects/:id/issues | Attribute | Type | Required | Description | |-------------------------------------------|----------------|----------|--------------| | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `iid` | integer/string | no | The internal ID of the project's issue (requires admin or project owner rights) | | `title` | string | yes | The title of an issue | | `description` | string | no | The description of an issue | | `confidential` | boolean | no | Set an issue to be confidential. Default is `false`. | diff --git a/doc/ci/examples/test-and-deploy-python-application-to-heroku.md b/doc/ci/examples/test-and-deploy-python-application-to-heroku.md index a433cd5a5dd..087b317ab73 100644 --- a/doc/ci/examples/test-and-deploy-python-application-to-heroku.md +++ b/doc/ci/examples/test-and-deploy-python-application-to-heroku.md @@ -46,7 +46,7 @@ This project has three jobs: ## Store API keys -You'll need to create two variables in `Project > Variables`: +You'll need to create two variables in `Settings > CI/CD > Variables` on your GitLab project settings: 1. `HEROKU_STAGING_API_KEY` - Heroku API key used to deploy staging app, 2. `HEROKU_PRODUCTION_API_KEY` - Heroku API key used to deploy production app. diff --git a/doc/gitlab-basics/create-project.md b/doc/gitlab-basics/create-project.md index dd8d95a3bca..2517908e5b1 100644 --- a/doc/gitlab-basics/create-project.md +++ b/doc/gitlab-basics/create-project.md @@ -43,7 +43,7 @@ When you create a new repo locally, instead of going to GitLab to manually create a new project and then push the repo, you can directly push it to GitLab to create the new project, all without leaving your terminal. If you have access to that -namespace, we will automatically create a new project under that GitLab namespace with its +namespace, we will automatically create a new project under that GitLab namespace with its visibility set to Private by default (you can later change it in the [project's settings](../public_access/public_access.md#how-to-change-project-visibility)). This can be done by using either SSH or HTTP: diff --git a/doc/install/installation.md b/doc/install/installation.md index 8c7f80fd8e8..ea01d88d85f 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -12,7 +12,7 @@ Since installations from source don't have Runit, Sidekiq can't be terminated an ## Select Version to Install -Make sure you view [this installation guide](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/install/installation.md) from the branch (version) of GitLab you would like to install (e.g., `11-1-stable`). +Make sure you view [this installation guide](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/install/installation.md) from the branch (version) of GitLab you would like to install (e.g., `11-2-stable`). You can select the branch in the version dropdown in the top left corner of GitLab (below the menu bar). If the highest number stable branch is unclear please check the [GitLab Blog](https://about.gitlab.com/blog/) for installation guide links by version. @@ -300,9 +300,9 @@ sudo usermod -aG redis git ### Clone the Source # Clone GitLab repository - sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-ce.git -b 11-1-stable gitlab + sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-ce.git -b 11-2-stable gitlab -**Note:** You can change `11-1-stable` to `master` if you want the *bleeding edge* version, but never install master on a production server! +**Note:** You can change `11-2-stable` to `master` if you want the *bleeding edge* version, but never install master on a production server! ### Configure It diff --git a/doc/raketasks/cleanup.md b/doc/raketasks/cleanup.md index cf891cd90ad..e2eb342361a 100644 --- a/doc/raketasks/cleanup.md +++ b/doc/raketasks/cleanup.md @@ -22,3 +22,34 @@ sudo gitlab-rake gitlab:cleanup:repos # installation from source bundle exec rake gitlab:cleanup:repos RAILS_ENV=production ``` + +Clean up local project upload files if they don't exist in GitLab database. The +task attempts to fix the file if it can find its project, otherwise it moves the +file to a lost and found directory. + +``` +# omnibus-gitlab +sudo gitlab-rake gitlab:cleanup:project_uploads + +# installation from source +bundle exec rake gitlab:cleanup:project_uploads RAILS_ENV=production +``` + +Example output: + +``` +$ sudo gitlab-rake gitlab:cleanup:project_uploads +I, [2018-07-27T12:08:27.671559 #89817] INFO -- : Looking for orphaned project uploads to clean up. Dry run... +D, [2018-07-27T12:08:28.293568 #89817] DEBUG -- : Processing batch of 500 project upload file paths, starting with /opt/gitlab/embedded/service/gitlab-rails/public/uploads/test.out +I, [2018-07-27T12:08:28.689869 #89817] INFO -- : Can move to lost and found /opt/gitlab/embedded/service/gitlab-rails/public/uploads/test.out -> /opt/gitlab/embedded/service/gitlab-rails/public/uploads/-/project-lost-found/test.out +I, [2018-07-27T12:08:28.755624 #89817] INFO -- : Can fix /opt/gitlab/embedded/service/gitlab-rails/public/uploads/foo/bar/89a0f7b0b97008a4a18cedccfdcd93fb/foo.txt -> /opt/gitlab/embedded/service/gitlab-rails/public/uploads/qux/foo/bar/89a0f7b0b97008a4a18cedccfdcd93fb/foo.txt +I, [2018-07-27T12:08:28.760257 #89817] INFO -- : Can move to lost and found /opt/gitlab/embedded/service/gitlab-rails/public/uploads/foo/bar/1dd6f0f7eefd2acc4c2233f89a0f7b0b/image.png -> /opt/gitlab/embedded/service/gitlab-rails/public/uploads/-/project-lost-found/foo/bar/1dd6f0f7eefd2acc4c2233f89a0f7b0b/image.png +I, [2018-07-27T12:08:28.764470 #89817] INFO -- : To cleanup these files run this command with DRY_RUN=false + +$ sudo gitlab-rake gitlab:cleanup:project_uploads DRY_RUN=false +I, [2018-07-27T12:08:32.944414 #89936] INFO -- : Looking for orphaned project uploads to clean up... +D, [2018-07-27T12:08:33.293568 #89817] DEBUG -- : Processing batch of 500 project upload file paths, starting with /opt/gitlab/embedded/service/gitlab-rails/public/uploads/test.out +I, [2018-07-27T12:08:33.689869 #89817] INFO -- : Did move to lost and found /opt/gitlab/embedded/service/gitlab-rails/public/uploads/test.out -> /opt/gitlab/embedded/service/gitlab-rails/public/uploads/-/project-lost-found/test.out +I, [2018-07-27T12:08:33.755624 #89817] INFO -- : Did fix /opt/gitlab/embedded/service/gitlab-rails/public/uploads/foo/bar/89a0f7b0b97008a4a18cedccfdcd93fb/foo.txt -> /opt/gitlab/embedded/service/gitlab-rails/public/uploads/qux/foo/bar/89a0f7b0b97008a4a18cedccfdcd93fb/foo.txt +I, [2018-07-27T12:08:33.760257 #89817] INFO -- : Did move to lost and found /opt/gitlab/embedded/service/gitlab-rails/public/uploads/foo/bar/1dd6f0f7eefd2acc4c2233f89a0f7b0b/image.png -> /opt/gitlab/embedded/service/gitlab-rails/public/uploads/-/project-lost-found/foo/bar/1dd6f0f7eefd2acc4c2233f89a0f7b0b/image.png +```
\ No newline at end of file diff --git a/doc/update/11.1-to-11.2.md b/doc/update/11.1-to-11.2.md new file mode 100644 index 00000000000..3edc7e6923e --- /dev/null +++ b/doc/update/11.1-to-11.2.md @@ -0,0 +1,378 @@ +--- +comments: false +--- + +# From 11.1 to 11.2 + +Make sure you view this update guide from the branch (version) of GitLab you would +like to install (e.g., `11-2-stable`. You can select the branch in the version +dropdown at the top left corner of GitLab (below the menu bar). + +If the highest number stable branch is unclear please check the +[GitLab Blog](https://about.gitlab.com/blog/archives.html) for installation +guide links by version. + +### 1. Stop server + +```bash +sudo service gitlab stop +``` + +### 2. Backup + +NOTE: If you installed GitLab from source, make sure `rsync` is installed. + +```bash +cd /home/git/gitlab + +sudo -u git -H bundle exec rake gitlab:backup:create RAILS_ENV=production +``` + +### 3. Update Ruby + +NOTE: GitLab 11.0 and higher only support Ruby 2.4.x and dropped support for Ruby 2.3.x. Be +sure to upgrade your interpreter if necessary. + +You can check which version you are running with `ruby -v`. + +Download Ruby and compile it: + +```bash +mkdir /tmp/ruby && cd /tmp/ruby +curl --remote-name --progress https://cache.ruby-lang.org/pub/ruby/2.4/ruby-2.4.4.tar.gz +echo 'ec82b0d53bd0adad9b19e6b45e44d54e9ec3f10c ruby-2.4.4.tar.gz' | shasum -c - && tar xzf ruby-2.4.4.tar.gz +cd ruby-2.4.4 + +./configure --disable-install-rdoc +make +sudo make install +``` + +Install Bundler: + +```bash +sudo gem install bundler --no-ri --no-rdoc +``` + +### 4. Update Node + +GitLab utilizes [webpack](http://webpack.js.org) to compile frontend assets. +This requires a minimum version of node v6.0.0. + +You can check which version you are running with `node -v`. If you are running +a version older than `v6.0.0` you will need to update to a newer version. You +can find instructions to install from community maintained packages or compile +from source at the nodejs.org website. + +<https://nodejs.org/en/download/> + +GitLab also requires the use of yarn `>= v1.2.0` to manage JavaScript +dependencies. + +```bash +curl --silent --show-error https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add - +echo "deb https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list +sudo apt-get update +sudo apt-get install yarn +``` + +More information can be found on the [yarn website](https://yarnpkg.com/en/docs/install). + +### 5. Update Go + +NOTE: GitLab 11.0 and higher only supports Go 1.9.x and newer, and dropped support for Go +1.5.x through 1.8.x. Be sure to upgrade your installation if necessary. + +You can check which version you are running with `go version`. + +Download and install Go: + +```bash +# Remove former Go installation folder +sudo rm -rf /usr/local/go + +curl --remote-name --progress https://dl.google.com/go/go1.10.3.linux-amd64.tar.gz +echo 'fa1b0e45d3b647c252f51f5e1204aba049cde4af177ef9f2181f43004f901035 go1.10.3.linux-amd64.tar.gz' | shasum -a256 -c - && \ + sudo tar -C /usr/local -xzf go1.10.3.linux-amd64.tar.gz +sudo ln -sf /usr/local/go/bin/{go,godoc,gofmt} /usr/local/bin/ +rm go1.10.3.linux-amd64.tar.gz +``` + +### 6. Get latest code + +```bash +cd /home/git/gitlab + +sudo -u git -H git fetch --all --prune +sudo -u git -H git checkout -- db/schema.rb # local changes will be restored automatically +sudo -u git -H git checkout -- locale +``` + +For GitLab Community Edition: + +```bash +cd /home/git/gitlab + +sudo -u git -H git checkout 11-2-stable +``` + +OR + +For GitLab Enterprise Edition: + +```bash +cd /home/git/gitlab + +sudo -u git -H git checkout 11-2-stable-ee +``` + +### 7. Update gitlab-shell + +```bash +cd /home/git/gitlab-shell + +sudo -u git -H git fetch --all --tags --prune +sudo -u git -H git checkout v$(</home/git/gitlab/GITLAB_SHELL_VERSION) +sudo -u git -H bin/compile +``` + +### 8. Update gitlab-workhorse + +Install and compile gitlab-workhorse. GitLab-Workhorse uses +[GNU Make](https://www.gnu.org/software/make/). +If you are not using Linux you may have to run `gmake` instead of +`make` below. + +```bash +cd /home/git/gitlab-workhorse + +sudo -u git -H git fetch --all --tags --prune +sudo -u git -H git checkout v$(</home/git/gitlab/GITLAB_WORKHORSE_VERSION) +sudo -u git -H make +``` + +### 9. Update Gitaly + +#### New Gitaly configuration options required + +In order to function Gitaly needs some additional configuration information. Below we assume you installed Gitaly in `/home/git/gitaly` and GitLab Shell in `/home/git/gitlab-shell`. + +```shell +echo ' +[gitaly-ruby] +dir = "/home/git/gitaly/ruby" + +[gitlab-shell] +dir = "/home/git/gitlab-shell" +' | sudo -u git tee -a /home/git/gitaly/config.toml +``` + +#### Check Gitaly configuration + +Due to a bug in the `rake gitlab:gitaly:install` script your Gitaly +configuration file may contain syntax errors. The block name +`[[storages]]`, which may occur more than once in your `config.toml` +file, should be `[[storage]]` instead. + +```shell +sudo -u git -H sed -i.pre-10.1 's/\[\[storages\]\]/[[storage]]/' /home/git/gitaly/config.toml +``` + +#### Compile Gitaly + +```shell +cd /home/git/gitaly +sudo -u git -H git fetch --all --tags --prune +sudo -u git -H git checkout v$(</home/git/gitlab/GITALY_SERVER_VERSION) +sudo -u git -H make +``` + +### 10. Update gitlab-pages + +#### Only needed if you use GitLab Pages. + +Install and compile gitlab-pages. GitLab-Pages uses +[GNU Make](https://www.gnu.org/software/make/). +If you are not using Linux you may have to run `gmake` instead of +`make` below. + +```bash +cd /home/git/gitlab-pages + +sudo -u git -H git fetch --all --tags --prune +sudo -u git -H git checkout v$(</home/git/gitlab/GITLAB_PAGES_VERSION) +sudo -u git -H make +``` + +### 11. Update MySQL permissions + +If you are using MySQL you need to grant the GitLab user the necessary +permissions on the database: + +```bash +mysql -u root -p -e "GRANT TRIGGER ON \`gitlabhq_production\`.* TO 'git'@'localhost';" +``` + +If you use MySQL with replication, or just have MySQL configured with binary logging, +you will need to also run the following on all of your MySQL servers: + +```bash +mysql -u root -p -e "SET GLOBAL log_bin_trust_function_creators = 1;" +``` + +You can make this setting permanent by adding it to your `my.cnf`: + +``` +log_bin_trust_function_creators=1 +``` + +### 12. Update configuration files + +#### New configuration options for `gitlab.yml` + +There might be configuration options available for [`gitlab.yml`][yaml]. View them with the command below and apply them manually to your current `gitlab.yml`: + +```sh +cd /home/git/gitlab + +git diff origin/11-1-stable:config/gitlab.yml.example origin/11-2-stable:config/gitlab.yml.example +``` + +#### Nginx configuration + +Ensure you're still up-to-date with the latest NGINX configuration changes: + +```sh +cd /home/git/gitlab + +# For HTTPS configurations +git diff origin/11-1-stable:lib/support/nginx/gitlab-ssl origin/11-2-stable:lib/support/nginx/gitlab-ssl + +# For HTTP configurations +git diff origin/11-1-stable:lib/support/nginx/gitlab origin/11-2-stable:lib/support/nginx/gitlab +``` + +If you are using Strict-Transport-Security in your installation to continue using it you must enable it in your Nginx +configuration as GitLab application no longer handles setting it. + +If you are using Apache instead of NGINX please see the updated [Apache templates]. +Also note that because Apache does not support upstreams behind Unix sockets you +will need to let gitlab-workhorse listen on a TCP port. You can do this +via [/etc/default/gitlab]. + +[Apache templates]: https://gitlab.com/gitlab-org/gitlab-recipes/tree/master/web-server/apache +[/etc/default/gitlab]: https://gitlab.com/gitlab-org/gitlab-ce/blob/11-2-stable/lib/support/init.d/gitlab.default.example#L38 + +#### SMTP configuration + +If you're installing from source and use SMTP to deliver mail, you will need to add the following line +to config/initializers/smtp_settings.rb: + +```ruby +ActionMailer::Base.delivery_method = :smtp +``` + +See [smtp_settings.rb.sample] as an example. + +[smtp_settings.rb.sample]: https://gitlab.com/gitlab-org/gitlab-ce/blob/11-2-stable/config/initializers/smtp_settings.rb.sample#L13 + +#### Init script + +There might be new configuration options available for [`gitlab.default.example`][gl-example]. View them with the command below and apply them manually to your current `/etc/default/gitlab`: + +```sh +cd /home/git/gitlab + +git diff origin/11-1-stable:lib/support/init.d/gitlab.default.example origin/11-2-stable:lib/support/init.d/gitlab.default.example +``` + +Ensure you're still up-to-date with the latest init script changes: + +```bash +cd /home/git/gitlab + +sudo cp lib/support/init.d/gitlab /etc/init.d/gitlab +``` + +For Ubuntu 16.04.1 LTS: + +```bash +sudo systemctl daemon-reload +``` + +### 13. Install libs, migrations, etc. + +```bash +cd /home/git/gitlab + +# MySQL installations (note: the line below states '--without postgres') +sudo -u git -H bundle install --without postgres development test --deployment + +# PostgreSQL installations (note: the line below states '--without mysql') +sudo -u git -H bundle install --without mysql development test --deployment + +# Optional: clean up old gems +sudo -u git -H bundle clean + +# Run database migrations +sudo -u git -H bundle exec rake db:migrate RAILS_ENV=production + +# Compile GetText PO files + +sudo -u git -H bundle exec rake gettext:compile RAILS_ENV=production + +# Update node dependencies and recompile assets +sudo -u git -H bundle exec rake yarn:install gitlab:assets:clean gitlab:assets:compile RAILS_ENV=production NODE_ENV=production + +# Clean up cache +sudo -u git -H bundle exec rake cache:clear RAILS_ENV=production +``` + +**MySQL installations**: Run through the `MySQL strings limits` and `Tables and data conversion to utf8mb4` [tasks](../install/database_mysql.md). + +### 14. Start application + +```bash +sudo service gitlab start +sudo service nginx restart +``` + +### 15. Check application status + +Check if GitLab and its environment are configured correctly: + +```bash +cd /home/git/gitlab + +sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production +``` + +To make sure you didn't miss anything run a more thorough check: + +```bash +cd /home/git/gitlab + +sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production +``` + +If all items are green, then congratulations, the upgrade is complete! + +## Things went south? Revert to previous version (11.1) + +### 1. Revert the code to the previous version + +Follow the [upgrade guide from 11.0 to 11.1](11.0-to-11.1.md), except for the +database migration (the backup is already migrated to the previous version). + +### 2. Restore from the backup + +```bash +cd /home/git/gitlab + +sudo -u git -H bundle exec rake gitlab:backup:restore RAILS_ENV=production +``` + +If you have more than one backup `*.tar` file(s) please add `BACKUP=timestamp_of_backup` to the command above. + +[yaml]: https://gitlab.com/gitlab-org/gitlab-ce/blob/11-2-stable/config/gitlab.yml.example +[gl-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/11-2-stable/lib/support/init.d/gitlab.default.example diff --git a/lib/api/internal.rb b/lib/api/internal.rb index a9803be9f69..516f25db15b 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -11,7 +11,8 @@ module API # # Params: # key_id - ssh key id for Git over SSH - # user_id - user id for Git over HTTP + # user_id - user id for Git over HTTP or over SSH in keyless SSH CERT mode + # username - user name for Git over SSH in keyless SSH cert mode # protocol - Git access protocol being used, e.g. HTTP or SSH # project - project full_path (not path on disk) # action - git action (git-upload-pack or git-receive-pack) @@ -28,6 +29,8 @@ module API Key.find_by(id: params[:key_id]) elsif params[:user_id] User.find_by(id: params[:user_id]) + elsif params[:username] + User.find_by_username(params[:username]) end protocol = params[:protocol] @@ -58,6 +61,7 @@ module API { status: true, gl_repository: gl_repository, + gl_id: Gitlab::GlId.gl_id(user), gl_username: user&.username, # This repository_path is a bogus value but gitlab-shell still requires @@ -71,10 +75,17 @@ module API post "/lfs_authenticate" do status 200 - key = Key.find(params[:key_id]) - key.update_last_used_at + if params[:key_id] + actor = Key.find(params[:key_id]) + actor.update_last_used_at + elsif params[:user_id] + actor = User.find_by(id: params[:user_id]) + raise ActiveRecord::RecordNotFound.new("No such user id!") unless actor + else + raise ActiveRecord::RecordNotFound.new("No key_id or user_id passed!") + end - token_handler = Gitlab::LfsToken.new(key) + token_handler = Gitlab::LfsToken.new(actor) { username: token_handler.actor_name, @@ -100,7 +111,7 @@ module API end # - # Discover user by ssh key or user id + # Discover user by ssh key, user id or username # get "/discover" do if params[:key_id] @@ -108,6 +119,8 @@ module API user = key.user elsif params[:user_id] user = User.find_by(id: params[:user_id]) + elsif params[:username] + user = User.find_by(username: params[:username]) end present user, with: Entities::UserSafe @@ -141,22 +154,30 @@ module API post '/two_factor_recovery_codes' do status 200 - key = Key.find_by(id: params[:key_id]) + if params[:key_id] + key = Key.find_by(id: params[:key_id]) - if key - key.update_last_used_at - else - break { 'success' => false, 'message' => 'Could not find the given key' } - end + if key + key.update_last_used_at + else + break { 'success' => false, 'message' => 'Could not find the given key' } + end - if key.is_a?(DeployKey) - break { success: false, message: 'Deploy keys cannot be used to retrieve recovery codes' } - end + if key.is_a?(DeployKey) + break { success: false, message: 'Deploy keys cannot be used to retrieve recovery codes' } + end + + user = key.user - user = key.user + unless user + break { success: false, message: 'Could not find a user for the given key' } + end + elsif params[:user_id] + user = User.find_by(id: params[:user_id]) - unless user - break { success: false, message: 'Could not find a user for the given key' } + unless user + break { success: false, message: 'Could not find the given user' } + end end unless user.two_factor_enabled? diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 25185d6edc8..bda05d1795b 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -162,6 +162,9 @@ module API desc: 'The IID of a merge request for which to resolve discussions' optional :discussion_to_resolve, type: String, desc: 'The ID of a discussion to resolve, also pass `merge_request_to_resolve_discussions_of`' + optional :iid, type: Integer, + desc: 'The internal ID of a project issue. Available only for admins and project owners.' + use :issue_params end post ':id/issues' do @@ -169,9 +172,10 @@ module API authorize! :create_issue, user_project - # Setting created_at time only allowed for admins and project owners + # Setting created_at time or iid only allowed for admins and project owners unless current_user.admin? || user_project.owner == current_user params.delete(:created_at) + params.delete(:iid) end issue_params = declared_params(include_missing: false) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index c7595493e11..c9931c2d603 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -80,7 +80,15 @@ module API params do requires :token, type: String, desc: %q(Runner's authentication token) optional :last_update, type: String, desc: %q(Runner's queue last_update token) - optional :info, type: Hash, desc: %q(Runner's metadata) + optional :info, type: Hash, desc: %q(Runner's metadata) do + optional :name, type: String, desc: %q(Runner's name) + optional :version, type: String, desc: %q(Runner's version) + optional :revision, type: String, desc: %q(Runner's revision) + optional :platform, type: String, desc: %q(Runner's platform) + optional :architecture, type: String, desc: %q(Runner's architecture) + optional :executor, type: String, desc: %q(Runner's executor) + optional :features, type: Hash, desc: %q(Runner's features) + end optional :session, type: Hash, desc: %q(Runner's session data) do optional :url, type: String, desc: %q(Session's url) optional :certificate, type: String, desc: %q(Session's certificate) diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 8d586f3594c..897010217dc 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -62,7 +62,7 @@ module API requires :housekeeping_incremental_repack_period, type: Integer, desc: "Number of Git pushes after which an incremental 'git repack' is run." end optional :html_emails_enabled, type: Boolean, desc: 'By default GitLab sends emails in HTML and plain text formats so mail clients can choose what format to use. Disable this option if you only want to send emails in plain text format.' - optional :import_sources, type: Array[String], values: %w[github bitbucket gitlab google_code fogbugz git gitlab_project], + optional :import_sources, type: Array[String], values: %w[github bitbucket gitlab google_code fogbugz git gitlab_project manifest], desc: 'Enabled sources for code import during project creation. OmniAuth must be configured for GitHub, Bitbucket, and GitLab.com' optional :koding_enabled, type: Boolean, desc: 'Enable Koding' given koding_enabled: ->(val) { val } do @@ -76,8 +76,8 @@ module API requires :metrics_host, type: String, desc: 'The InfluxDB host' requires :metrics_method_call_threshold, type: Integer, desc: 'A method call is only tracked when it takes longer to complete than the given amount of milliseconds.' requires :metrics_packet_size, type: Integer, desc: 'The amount of points to store in a single UDP packet' - requires :metrics_port, type: Integer, desc: 'The UDP port to use for connecting to InfluxDB' requires :metrics_pool_size, type: Integer, desc: 'The amount of InfluxDB connections to open' + requires :metrics_port, type: Integer, desc: 'The UDP port to use for connecting to InfluxDB' requires :metrics_sample_interval, type: Integer, desc: 'The sampling interval in seconds' requires :metrics_timeout, type: Integer, desc: 'The amount of seconds after which an InfluxDB connection will time out' end diff --git a/lib/gitlab/ci/status/build/failed.rb b/lib/gitlab/ci/status/build/failed.rb index 155f4fc1343..703f0b9217b 100644 --- a/lib/gitlab/ci/status/build/failed.rb +++ b/lib/gitlab/ci/status/build/failed.rb @@ -4,12 +4,13 @@ module Gitlab module Build class Failed < Status::Extended REASONS = { - 'unknown_failure' => 'unknown failure', - 'script_failure' => 'script failure', - 'api_failure' => 'API failure', - 'stuck_or_timeout_failure' => 'stuck or timeout failure', - 'runner_system_failure' => 'runner system failure', - 'missing_dependency_failure' => 'missing dependency failure' + unknown_failure: 'unknown failure', + script_failure: 'script failure', + api_failure: 'API failure', + stuck_or_timeout_failure: 'stuck or timeout failure', + runner_system_failure: 'runner system failure', + missing_dependency_failure: 'missing dependency failure', + runner_unsupported: 'unsupported runner' }.freeze def status_tooltip @@ -31,7 +32,11 @@ module Gitlab end def description - "<br> (#{REASONS[subject.failure_reason]})" + "<br> (#{failure_reason_message})" + end + + def failure_reason_message + REASONS.fetch(subject.failure_reason.to_sym) end end end diff --git a/lib/gitlab/cleanup/project_upload_file_finder.rb b/lib/gitlab/cleanup/project_upload_file_finder.rb new file mode 100644 index 00000000000..2ee8b60e76a --- /dev/null +++ b/lib/gitlab/cleanup/project_upload_file_finder.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Gitlab + module Cleanup + class ProjectUploadFileFinder + FIND_BATCH_SIZE = 500 + ABSOLUTE_UPLOAD_DIR = FileUploader.root.freeze + EXCLUDED_SYSTEM_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/-/*".freeze + EXCLUDED_HASHED_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/@hashed/*".freeze + EXCLUDED_TMP_UPLOADS_PATH = "#{ABSOLUTE_UPLOAD_DIR}/tmp/*".freeze + + # Paths are relative to the upload directory + def each_file_batch(batch_size: FIND_BATCH_SIZE, &block) + cmd = build_find_command(ABSOLUTE_UPLOAD_DIR) + + Open3.popen2(*cmd) do |stdin, stdout, status_thread| + yield_paths_in_batches(stdout, batch_size, &block) + + raise "Find command failed" unless status_thread.value.success? + end + end + + private + + def yield_paths_in_batches(stdout, batch_size, &block) + paths = [] + + stdout.each_line("\0") do |line| + paths << line.chomp("\0") + + if paths.size >= batch_size + yield(paths) + paths = [] + end + end + + yield(paths) if paths.any? + end + + def build_find_command(search_dir) + cmd = %W[find -L #{search_dir} + -type f + ! ( -path #{EXCLUDED_SYSTEM_UPLOADS_PATH} -prune ) + ! ( -path #{EXCLUDED_HASHED_UPLOADS_PATH} -prune ) + ! ( -path #{EXCLUDED_TMP_UPLOADS_PATH} -prune ) + -print0] + + ionice = which_ionice + cmd = %W[#{ionice} -c Idle] + cmd if ionice + + log_msg = "find command: \"#{cmd.join(' ')}\"" + Rails.logger.info log_msg + + cmd + end + + def which_ionice + Gitlab::Utils.which('ionice') + rescue StandardError + # In this case, returning false is relatively safe, + # even though it isn't very nice + false + end + end + end +end diff --git a/lib/gitlab/cleanup/project_uploads.rb b/lib/gitlab/cleanup/project_uploads.rb new file mode 100644 index 00000000000..b88e00311d5 --- /dev/null +++ b/lib/gitlab/cleanup/project_uploads.rb @@ -0,0 +1,125 @@ +# frozen_string_literal: true + +module Gitlab + module Cleanup + class ProjectUploads + LOST_AND_FOUND = File.join(ProjectUploadFileFinder::ABSOLUTE_UPLOAD_DIR, '-', 'project-lost-found') + + attr_reader :logger + + def initialize(logger: nil) + @logger = logger || Rails.logger + end + + def run!(dry_run: true) + logger.info "Looking for orphaned project uploads to clean up#{'. Dry run' if dry_run}..." + + each_orphan_file do |path, upload_path| + result = cleanup(path, upload_path, dry_run) + + logger.info result + end + end + + private + + def cleanup(path, upload_path, dry_run) + # This happened in staging: + # `find` returned a path on which `File.delete` raised `Errno::ENOENT` + return "Cannot find file: #{path}" unless File.exist?(path) + + correct_path = upload_path && find_correct_path(upload_path) + + if correct_path + move(path, correct_path, 'fix', dry_run) + else + move_to_lost_and_found(path, dry_run) + end + end + + # Accepts a path in the form of "#{hex_secret}/#{filename}" + def find_correct_path(upload_path) + upload = Upload.find_by(uploader: 'FileUploader', path: upload_path) + return unless upload && upload.local? + + upload.absolute_path + rescue => e + logger.error e.message + + # absolute_path depends on a lot of code. If it doesn't work, then it + # it doesn't matter if the upload file is in the right place. Treat it + # as uncorrectable. + # I.e. the project record might be missing, which raises an exception. + nil + end + + def move_to_lost_and_found(path, dry_run) + new_path = path.sub(/\A#{ProjectUploadFileFinder::ABSOLUTE_UPLOAD_DIR}/, LOST_AND_FOUND) + + move(path, new_path, 'move to lost and found', dry_run) + end + + def move(path, new_path, prefix, dry_run) + action = "#{prefix} #{path} -> #{new_path}" + + if dry_run + "Can #{action}" + else + begin + FileUtils.mkdir_p(File.dirname(new_path)) + FileUtils.mv(path, new_path) + + "Did #{action}" + rescue => e + "Error during #{action}: #{e.inspect}" + end + end + end + + # Yields absolute paths of project upload files that are not in the + # uploads table + def each_orphan_file + ProjectUploadFileFinder.new.each_file_batch do |file_paths| + logger.debug "Processing batch of #{file_paths.size} project upload file paths, starting with #{file_paths.first}" + + file_paths.each do |path| + pup = ProjectUploadPath.from_path(path) + + yield(path, pup.upload_path) if pup.orphan? + end + end + end + + class ProjectUploadPath + PROJECT_FULL_PATH_REGEX = %r{\A#{FileUploader.root}/(.+)/(\h+/[^/]+)\z}.freeze + + attr_reader :full_path, :upload_path + + def initialize(full_path, upload_path) + @full_path = full_path + @upload_path = upload_path + end + + def self.from_path(path) + path_matched = path.match(PROJECT_FULL_PATH_REGEX) + return new(nil, nil) unless path_matched + + new(path_matched[1], path_matched[2]) + end + + def orphan? + return true if full_path.nil? || upload_path.nil? + + # It's possible to reduce to one query, but `where_full_path_in` is complex + !Upload.exists?(path: upload_path, model_id: project_id, model_type: 'Project', uploader: 'FileUploader') + end + + private + + def project_id + @project_id ||= Project.where_full_path_in([full_path]).pluck(:id) + end + end + end + end +end diff --git a/lib/gitlab/graphs/commits.rb b/lib/gitlab/graphs/commits.rb index 3caf9036459..c4ffc19df09 100644 --- a/lib/gitlab/graphs/commits.rb +++ b/lib/gitlab/graphs/commits.rb @@ -18,7 +18,7 @@ module Gitlab end def commit_per_day - @commit_per_day ||= @commits.size / (@duration + 1) + @commit_per_day ||= (@commits.size.to_f / (@duration + 1)).round(1) end def collect_data diff --git a/lib/gitlab/import_sources.rb b/lib/gitlab/import_sources.rb index af9b880ef9e..45816bee176 100644 --- a/lib/gitlab/import_sources.rb +++ b/lib/gitlab/import_sources.rb @@ -22,24 +22,28 @@ module Gitlab class << self def options - @options ||= Hash[ImportTable.map { |importer| [importer.title, importer.name] }] + Hash[import_table.map { |importer| [importer.title, importer.name] }] end def values - @values ||= ImportTable.map(&:name) + import_table.map(&:name) end def importer_names - @importer_names ||= ImportTable.select(&:importer).map(&:name) + import_table.select(&:importer).map(&:name) end def importer(name) - ImportTable.find { |import_source| import_source.name == name }.importer + import_table.find { |import_source| import_source.name == name }.importer end def title(name) options.key(name) end + + def import_table + ImportTable + end end end end diff --git a/lib/gitlab/template_helper.rb b/lib/gitlab/template_helper.rb new file mode 100644 index 00000000000..3b8e45e0688 --- /dev/null +++ b/lib/gitlab/template_helper.rb @@ -0,0 +1,22 @@ +module Gitlab + module TemplateHelper + include Gitlab::Utils::StrongMemoize + + def prepare_template_environment(file_path) + return unless file_path.present? + + FileUtils.mkdir_p(File.dirname(import_upload_path)) + FileUtils.copy_entry(file_path, import_upload_path) + end + + def import_upload_path + strong_memoize(:import_upload_path) do + Gitlab::ImportExport.import_upload_path(filename: tmp_filename) + end + end + + def tmp_filename + SecureRandom.hex + end + end +end diff --git a/lib/tasks/gitlab/cleanup.rake b/lib/tasks/gitlab/cleanup.rake index 5e07b12ee1c..a2feb074b1d 100644 --- a/lib/tasks/gitlab/cleanup.rake +++ b/lib/tasks/gitlab/cleanup.rake @@ -7,9 +7,8 @@ namespace :gitlab do desc "GitLab | Cleanup | Clean namespaces" task dirs: :gitlab_environment do warn_user_is_not_gitlab - remove_flag = ENV['REMOVE'] - namespaces = Namespace.pluck(:path) + namespaces = Namespace.pluck(:path) namespaces << HASHED_REPOSITORY_NAME # add so that it will be ignored Gitlab.config.repositories.storages.each do |name, repository_storage| git_base_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { repository_storage.legacy_disk_path } @@ -31,7 +30,7 @@ namespace :gitlab do end all_dirs.each do |dir_path| - if remove_flag + if remove? if FileUtils.rm_rf dir_path puts "Removed...#{dir_path}".color(:red) else @@ -43,7 +42,7 @@ namespace :gitlab do end end - unless remove_flag + unless remove? puts "To cleanup this directories run this command with REMOVE=true".color(:yellow) end end @@ -104,5 +103,37 @@ namespace :gitlab do puts "To block these users run this command with BLOCK=true".color(:yellow) end end + + desc "GitLab | Cleanup | Clean orphaned project uploads" + task project_uploads: :gitlab_environment do + warn_user_is_not_gitlab + + cleaner = Gitlab::Cleanup::ProjectUploads.new(logger: logger) + cleaner.run!(dry_run: dry_run?) + + if dry_run? + logger.info "To clean up these files run this command with DRY_RUN=false".color(:yellow) + end + end + + def remove? + ENV['REMOVE'] == 'true' + end + + def dry_run? + ENV['DRY_RUN'] != 'false' + end + + def logger + return @logger if defined?(@logger) + + @logger = if Rails.env.development? || Rails.env.production? + Logger.new(STDOUT).tap do |stdout_logger| + stdout_logger.extend(ActiveSupport::Logger.broadcast(Rails.logger)) + end + else + Rails.logger + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1bc2093433f..a767c179dea 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2525,6 +2525,9 @@ msgstr "" msgid "Files (%{human_size})" msgstr "" +msgid "Filter" +msgstr "" + msgid "Filter by commit message" msgstr "" @@ -3571,12 +3574,21 @@ msgstr "" msgid "No files found." msgstr "" +msgid "No labels with such name or description" +msgstr "" + msgid "No merge requests found" msgstr "" msgid "No messages were logged" msgstr "" +msgid "No other labels with such name or description" +msgstr "" + +msgid "No prioritised labels with such name or description" +msgstr "" + msgid "No public groups" msgstr "" @@ -3619,9 +3631,6 @@ msgstr "" msgid "Note: Consider asking your GitLab administrator to configure %{github_integration_link}, which will allow login via GitHub and allow importing repositories without generating a Personal Access Token." msgstr "" -msgid "Notes|Are you sure you want to cancel creating this comment?" -msgstr "" - msgid "Notification events" msgstr "" @@ -4913,6 +4922,9 @@ msgstr "" msgid "Submit as spam" msgstr "" +msgid "Submit search" +msgstr "" + msgid "Subscribe" msgstr "" diff --git a/spec/factories/resource_label_events.rb b/spec/factories/resource_label_events.rb new file mode 100644 index 00000000000..a67ad78c098 --- /dev/null +++ b/spec/factories/resource_label_events.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :resource_label_event do + user { issue.project.creator } + action :add + label + issue + end +end diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index 2d268ecab58..bf4d5396df9 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -342,9 +342,8 @@ describe 'Merge request > User resolves diff notes and discussions', :js do end end - it 'shows jump to next discussion button, apart from the last one' do - expect(page).to have_selector('.discussion-reply-holder', count: 2) - expect(page).to have_selector('.discussion-reply-holder .discussion-next-btn', count: 1) + it 'shows jump to next discussion button' do + expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn')) end it 'displays next discussion even if hidden' do diff --git a/spec/features/projects/labels/search_labels_spec.rb b/spec/features/projects/labels/search_labels_spec.rb new file mode 100644 index 00000000000..2d5a138c3cc --- /dev/null +++ b/spec/features/projects/labels/search_labels_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Search for labels', :js do + let(:user) { create(:user) } + let(:project) { create(:project) } + let!(:label1) { create(:label, title: 'Foo', description: 'Lorem ipsum', project: project) } + let!(:label2) { create(:label, title: 'Bar', description: 'Fusce consequat', project: project) } + + before do + project.add_maintainer(user) + sign_in(user) + + visit project_labels_path(project) + end + + it 'searches for label by title' do + fill_in 'label-search', with: 'Bar' + find('#label-search').native.send_keys(:enter) + + expect(page).to have_content(label2.title) + expect(page).to have_content(label2.description) + expect(page).not_to have_content(label1.title) + expect(page).not_to have_content(label1.description) + end + + it 'searches for label by title' do + fill_in 'label-search', with: 'Lorem' + find('#label-search').native.send_keys(:enter) + + expect(page).to have_content(label1.title) + expect(page).to have_content(label1.description) + expect(page).not_to have_content(label2.title) + expect(page).not_to have_content(label2.description) + end + + it 'shows nothing found message' do + fill_in 'label-search', with: 'nonexistent' + find('#label-search').native.send_keys(:enter) + + expect(page).to have_content('No labels with such name or description') + expect(page).not_to have_content(label1.title) + expect(page).not_to have_content(label1.description) + expect(page).not_to have_content(label2.title) + expect(page).not_to have_content(label2.description) + end + + context 'priority labels' do + let!(:label_priority) { create(:label_priority, label: label1, project: project) } + + it 'searches for priority label' do + fill_in 'label-search', with: 'Foo' + find('#label-search').native.send_keys(:enter) + + page.within('.prioritized-labels') do + expect(page).to have_content(label1.title) + expect(page).to have_content(label1.description) + end + + page.within('.other-labels') do + expect(page).to have_content('No other labels with such name or description') + end + end + + it 'searches for other label' do + fill_in 'label-search', with: 'Bar' + find('#label-search').native.send_keys(:enter) + + page.within('.prioritized-labels') do + expect(page).to have_content('No prioritised labels with such name or description') + end + + page.within('.other-labels') do + expect(page).to have_content(label2.title) + expect(page).to have_content(label2.description) + end + end + end +end diff --git a/spec/features/projects/user_uses_shortcuts_spec.rb b/spec/features/projects/user_uses_shortcuts_spec.rb index df9ee69aadb..64f9a4fcd39 100644 --- a/spec/features/projects/user_uses_shortcuts_spec.rb +++ b/spec/features/projects/user_uses_shortcuts_spec.rb @@ -9,6 +9,8 @@ describe 'User uses shortcuts', :js do sign_in(user) visit(project_path(project)) + + wait_for_requests end context 'when navigating to the Project pages' do diff --git a/spec/finders/labels_finder_spec.rb b/spec/finders/labels_finder_spec.rb index 899d0d22819..eb2a4576e30 100644 --- a/spec/finders/labels_finder_spec.rb +++ b/spec/finders/labels_finder_spec.rb @@ -14,7 +14,7 @@ describe LabelsFinder do let(:project_4) { create(:project, :public) } let(:project_5) { create(:project, namespace: group_1) } - let!(:project_label_1) { create(:label, project: project_1, title: 'Label 1') } + let!(:project_label_1) { create(:label, project: project_1, title: 'Label 1', description: 'awesome label') } let!(:project_label_2) { create(:label, project: project_2, title: 'Label 2') } let!(:project_label_4) { create(:label, project: project_4, title: 'Label 4') } let!(:project_label_5) { create(:label, project: project_5, title: 'Label 5') } @@ -196,5 +196,19 @@ describe LabelsFinder do expect(finder.execute).to be_empty end end + + context 'search by title and description' do + it 'returns labels with a partially matching title' do + finder = described_class.new(user, search: '(group)') + + expect(finder.execute).to eq [group_label_1] + end + + it 'returns labels with a partially matching description' do + finder = described_class.new(user, search: 'awesome') + + expect(finder.execute).to eq [project_label_1] + end + end end end diff --git a/spec/javascripts/.eslintrc.yml b/spec/javascripts/.eslintrc.yml index 78e2f3b521f..5525c9f5bd0 100644 --- a/spec/javascripts/.eslintrc.yml +++ b/spec/javascripts/.eslintrc.yml @@ -30,7 +30,6 @@ rules: jasmine/no-spec-dupes: - warn - branch - no-console: off prefer-arrow-callback: off import/no-unresolved: - error diff --git a/spec/javascripts/autosave_spec.js b/spec/javascripts/autosave_spec.js index dcb1c781591..38ae5b7e00c 100644 --- a/spec/javascripts/autosave_spec.js +++ b/spec/javascripts/autosave_spec.js @@ -59,10 +59,12 @@ describe('Autosave', () => { Autosave.prototype.restore.call(autosave); - expect(field.trigger).toHaveBeenCalled(); + expect( + field.trigger, + ).toHaveBeenCalled(); }); - it('triggers native event', done => { + it('triggers native event', (done) => { autosave.field.get(0).addEventListener('change', () => { done(); }); @@ -79,7 +81,9 @@ describe('Autosave', () => { it('does not trigger event', () => { spyOn(field, 'trigger').and.callThrough(); - expect(field.trigger).not.toHaveBeenCalled(); + expect( + field.trigger, + ).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/javascripts/datetime_utility_spec.js b/spec/javascripts/datetime_utility_spec.js index e224ed46d18..492171684dc 100644 --- a/spec/javascripts/datetime_utility_spec.js +++ b/spec/javascripts/datetime_utility_spec.js @@ -162,7 +162,6 @@ describe('getTimeframeWindowFrom', () => { const timeframe = datetimeUtility.getTimeframeWindowFrom(startDate, 5); expect(timeframe.length).toBe(5); timeframe.forEach((timeframeItem, index) => { - console.log(timeframeItem); expect(timeframeItem.getFullYear() === mockTimeframe[index].getFullYear()).toBe(true); expect(timeframeItem.getMonth() === mockTimeframe[index].getMonth()).toBe(true); expect(timeframeItem.getDate() === mockTimeframe[index].getDate()).toBeTruthy(); diff --git a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js index bdc94131fc2..2d136a63c52 100644 --- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js +++ b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js @@ -18,12 +18,10 @@ describe('DiffLineGutterContent', () => { }; const setDiscussions = component => { component.$store.dispatch('setInitialNotes', getDiscussionsMockData()); - component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] }); }; const resetDiscussions = component => { component.$store.dispatch('setInitialNotes', []); - component.$store.commit('diffs/SET_DIFF_DATA', {}); }; describe('computed', () => { @@ -50,11 +48,7 @@ describe('DiffLineGutterContent', () => { it('should return discussions for the given lineCode', () => { const { lineCode } = getDiffFileMock().highlightedDiffLines[1]; - const component = createComponent({ - lineCode, - showCommentButton: true, - discussions: getDiscussionsMockData(), - }); + const component = createComponent({ lineCode, showCommentButton: true }); setDiscussions(component); diff --git a/spec/javascripts/diffs/components/diff_line_note_form_spec.js b/spec/javascripts/diffs/components/diff_line_note_form_spec.js index 6fe5fdaf7f9..4600aaea70b 100644 --- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js +++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js @@ -3,7 +3,6 @@ import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue'; import store from '~/mr_notes/stores'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import diffFileMockData from '../mock_data/diff_file'; -import { noteableDataMock } from '../../notes/mock_data'; describe('DiffLineNoteForm', () => { let component; @@ -22,9 +21,10 @@ describe('DiffLineNoteForm', () => { noteTargetLine: diffLines[0], }); - Object.defineProperties(component, { - noteableData: { value: noteableDataMock }, - isLoggedIn: { value: true }, + Object.defineProperty(component, 'isLoggedIn', { + get() { + return true; + }, }); component.$mount(); @@ -32,37 +32,12 @@ describe('DiffLineNoteForm', () => { describe('methods', () => { describe('handleCancelCommentForm', () => { - it('should ask for confirmation when shouldConfirm and isDirty passed as truthy', () => { - spyOn(window, 'confirm').and.returnValue(false); - - component.handleCancelCommentForm(true, true); - expect(window.confirm).toHaveBeenCalled(); - }); - - it('should ask for confirmation when one of the params false', () => { - spyOn(window, 'confirm').and.returnValue(false); - - component.handleCancelCommentForm(true, false); - expect(window.confirm).not.toHaveBeenCalled(); - - component.handleCancelCommentForm(false, true); - expect(window.confirm).not.toHaveBeenCalled(); - }); - - it('should call cancelCommentForm with lineCode', done => { - spyOn(window, 'confirm'); + it('should call cancelCommentForm with lineCode', () => { spyOn(component, 'cancelCommentForm'); - spyOn(component, 'resetAutoSave'); component.handleCancelCommentForm(); - expect(window.confirm).not.toHaveBeenCalled(); - component.$nextTick(() => { - expect(component.cancelCommentForm).toHaveBeenCalledWith({ - lineCode: diffLines[0].lineCode, - }); - expect(component.resetAutoSave).toHaveBeenCalled(); - - done(); + expect(component.cancelCommentForm).toHaveBeenCalledWith({ + lineCode: diffLines[0].lineCode, }); }); }); @@ -91,7 +66,7 @@ describe('DiffLineNoteForm', () => { describe('mounted', () => { it('should init autosave', () => { - const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; + const key = 'autosave/Note/issue///DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; expect(component.autosave).toBeDefined(); expect(component.autosave.key).toEqual(key); diff --git a/spec/javascripts/diffs/components/inline_diff_view_spec.js b/spec/javascripts/diffs/components/inline_diff_view_spec.js index 90dfa5c5a58..b02328dd359 100644 --- a/spec/javascripts/diffs/components/inline_diff_view_spec.js +++ b/spec/javascripts/diffs/components/inline_diff_view_spec.js @@ -33,7 +33,6 @@ describe('InlineDiffView', () => { it('should render discussions', done => { const el = component.$el; component.$store.dispatch('setInitialNotes', getDiscussionsMockData()); - component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] }); Vue.nextTick(() => { expect(el.querySelectorAll('.notes_holder').length).toEqual(1); diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 8cd57d2248b..41d0dfd8939 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -12,17 +12,6 @@ export default { head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13', }, }, - original_position: { - formatter: { - old_line: null, - new_line: 2, - old_path: 'CHANGELOG', - new_path: 'CHANGELOG', - base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a', - start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962', - head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13', - }, - }, line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2', expanded: true, notes: [ diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index 987c4dbcb26..7706c32d24d 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -2,7 +2,6 @@ import * as getters from '~/diffs/store/getters'; import state from '~/diffs/store/modules/diff_state'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; import discussion from '../mock_data/diff_discussions'; -import diffFile from '../mock_data/diff_file'; describe('Diffs Module Getters', () => { let localState; @@ -222,38 +221,4 @@ describe('Diffs Module Getters', () => { expect(getters.getDiffFileByHash(localState)('123')).toBeUndefined(); }); }); - - describe('discussionsByLineCode', () => { - let mockState; - - beforeEach(() => { - mockState = { diffFiles: [diffFile] }; - }); - - it('should return a map of diff lines with their line codes', () => { - const mockGetters = { discussions: [discussionMock] }; - - const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters); - expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined(); - expect(Object.keys(map).length).toEqual(1); - }); - - it('should have the diff discussion on the map if the original position matches', () => { - discussionMock.position.formatter.base_sha = 'ff9200'; - const mockGetters = { discussions: [discussionMock] }; - - const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters); - expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined(); - expect(Object.keys(map).length).toEqual(1); - }); - - it('should not add an outdated diff discussion to the returned map', () => { - discussionMock.position.formatter.base_sha = 'ff9200'; - discussionMock.original_position.formatter.base_sha = 'ff9200'; - const mockGetters = { discussions: [discussionMock] }; - - const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters); - expect(Object.keys(map).length).toEqual(0); - }); - }); }); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 8e7bd8afca4..32136d9ebff 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -207,24 +207,4 @@ describe('DiffsStoreUtils', () => { expect(utils.trimFirstCharOfLineContent()).toEqual({}); }); }); - - describe('getDiffRefsByLineCode', () => { - it('should return diffRefs for all highlightedDiffLines', () => { - const diffFile = getDiffFileMock(); - const map = utils.getDiffRefsByLineCode([diffFile]); - const { highlightedDiffLines } = diffFile; - const lineCodeCount = highlightedDiffLines.reduce( - (acc, line) => (line.lineCode ? acc + 1 : acc), - 0, - ); - - const { baseSha, headSha, startSha } = diffFile.diffRefs; - const targetLine = map[highlightedDiffLines[4].lineCode]; - - expect(Object.keys(map).length).toEqual(lineCodeCount); - expect(targetLine.baseSha).toEqual(baseSha); - expect(targetLine.headSha).toEqual(headSha); - expect(targetLine.startSha).toEqual(startSha); - }); - }); }); diff --git a/spec/javascripts/notes/components/discussion_counter_spec.js b/spec/javascripts/notes/components/discussion_counter_spec.js index d09bc5037ef..a3869cc6498 100644 --- a/spec/javascripts/notes/components/discussion_counter_spec.js +++ b/spec/javascripts/notes/components/discussion_counter_spec.js @@ -46,7 +46,7 @@ describe('DiscussionCounter component', () => { discussions, }); setFixtures(` - <div class="discussion" data-discussion-id="${firstDiscussionId}"></div> + <div data-discussion-id="${firstDiscussionId}"></div> `); vm.jumpToFirstUnresolvedDiscussion(); diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js index 2a01bd85520..7da931fd9cb 100644 --- a/spec/javascripts/notes/components/noteable_discussion_spec.js +++ b/spec/javascripts/notes/components/noteable_discussion_spec.js @@ -14,7 +14,6 @@ describe('noteable_discussion component', () => { preloadFixtures(discussionWithTwoUnresolvedNotes); beforeEach(() => { - window.mrTabs = {}; store = createStore(); store.dispatch('setNoteableData', noteableDataMock); store.dispatch('setNotesData', notesDataMock); @@ -47,15 +46,10 @@ describe('noteable_discussion component', () => { it('should toggle reply form', done => { vm.$el.querySelector('.js-vue-discussion-reply').click(); - Vue.nextTick(() => { + expect(vm.$refs.noteForm).not.toBeNull(); expect(vm.isReplying).toEqual(true); - - // There is a watcher for `isReplying` which will init autosave in the next tick - Vue.nextTick(() => { - expect(vm.$refs.noteForm).not.toBeNull(); - done(); - }); + done(); }); }); @@ -107,29 +101,33 @@ describe('noteable_discussion component', () => { describe('methods', () => { describe('jumpToNextDiscussion', () => { - it('expands next unresolved discussion', done => { - const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0]; - discussion2.resolved = false; - discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to) - vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]); - window.mrTabs.currentAction = 'show'; - - Vue.nextTick() - .then(() => { - spyOn(vm, 'expandDiscussion').and.stub(); - - const nextDiscussionId = discussion2.id; - - setFixtures(` - <div class="discussion" data-discussion-id="${nextDiscussionId}"></div> - `); + it('expands next unresolved discussion', () => { + spyOn(vm, 'expandDiscussion').and.stub(); + const discussions = [ + discussionMock, + { + ...discussionMock, + id: discussionMock.id + 1, + notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }], + }, + { + ...discussionMock, + id: discussionMock.id + 2, + notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }], + }, + ]; + const nextDiscussionId = discussionMock.id + 2; + store.replaceState({ + ...store.state, + discussions, + }); + setFixtures(` + <div data-discussion-id="${nextDiscussionId}"></div> + `); - vm.jumpToNextDiscussion(); + vm.jumpToNextDiscussion(); - expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId }); - }) - .then(done) - .catch(done.fail); + expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId }); }); }); }); diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 67f6a9629d9..be2a8ba67fe 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/spec/javascripts/notes/mock_data.js @@ -1168,87 +1168,3 @@ export const collapsedSystemNotes = [ diff_discussion: false, }, ]; - -export const discussion1 = { - id: 'abc1', - resolvable: true, - resolved: false, - diff_file: { - file_path: 'about.md', - }, - position: { - formatter: { - new_line: 50, - old_line: null, - }, - }, - notes: [ - { - created_at: '2018-07-04T16:25:41.749Z', - }, - ], -}; - -export const resolvedDiscussion1 = { - id: 'abc1', - resolvable: true, - resolved: true, - diff_file: { - file_path: 'about.md', - }, - position: { - formatter: { - new_line: 50, - old_line: null, - }, - }, - notes: [ - { - created_at: '2018-07-04T16:25:41.749Z', - }, - ], -}; - -export const discussion2 = { - id: 'abc2', - resolvable: true, - resolved: false, - diff_file: { - file_path: 'README.md', - }, - position: { - formatter: { - new_line: null, - old_line: 20, - }, - }, - notes: [ - { - created_at: '2018-07-04T12:05:41.749Z', - }, - ], -}; - -export const discussion3 = { - id: 'abc3', - resolvable: true, - resolved: false, - diff_file: { - file_path: 'README.md', - }, - position: { - formatter: { - new_line: 21, - old_line: null, - }, - }, - notes: [ - { - created_at: '2018-07-05T17:25:41.749Z', - }, - ], -}; - -export const unresolvableDiscussion = { - resolvable: false, -}; diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js index 7f8ede51508..41599e00122 100644 --- a/spec/javascripts/notes/stores/getters_spec.js +++ b/spec/javascripts/notes/stores/getters_spec.js @@ -5,11 +5,6 @@ import { noteableDataMock, individualNote, collapseNotesMock, - discussion1, - discussion2, - discussion3, - resolvedDiscussion1, - unresolvableDiscussion, } from '../mock_data'; const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json'; @@ -114,154 +109,4 @@ describe('Getters Notes Store', () => { expect(getters.isNotesFetched(state)).toBeFalsy(); }); }); - - describe('allResolvableDiscussions', () => { - it('should return only resolvable discussions in same order', () => { - const localGetters = { - allDiscussions: [ - discussion3, - unresolvableDiscussion, - discussion1, - unresolvableDiscussion, - discussion2, - ], - }; - - expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([ - discussion3, - discussion1, - discussion2, - ]); - }); - - it('should return empty array if there are no resolvable discussions', () => { - const localGetters = { - allDiscussions: [unresolvableDiscussion, unresolvableDiscussion], - }; - - expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]); - }); - }); - - describe('unresolvedDiscussionsIdsByDiff', () => { - it('should return all discussions IDs in diff order', () => { - const localGetters = { - allResolvableDiscussions: [discussion3, discussion1, discussion2], - }; - - expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([ - 'abc1', - 'abc2', - 'abc3', - ]); - }); - - it('should return empty array if all discussions have been resolved', () => { - const localGetters = { - allResolvableDiscussions: [resolvedDiscussion1], - }; - - expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]); - }); - }); - - describe('unresolvedDiscussionsIdsByDate', () => { - it('should return all discussions in date ascending order', () => { - const localGetters = { - allResolvableDiscussions: [discussion3, discussion1, discussion2], - }; - - expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([ - 'abc2', - 'abc1', - 'abc3', - ]); - }); - - it('should return empty array if all discussions have been resolved', () => { - const localGetters = { - allResolvableDiscussions: [resolvedDiscussion1], - }; - - expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([]); - }); - }); - - describe('unresolvedDiscussionsIdsOrdered', () => { - const localGetters = { - unresolvedDiscussionsIdsByDate: ['123', '456'], - unresolvedDiscussionsIdsByDiff: ['abc', 'def'], - }; - - it('should return IDs ordered by diff when diffOrder param is true', () => { - expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(true)).toEqual([ - 'abc', - 'def', - ]); - }); - - it('should return IDs ordered by date when diffOrder param is not true', () => { - expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(false)).toEqual([ - '123', - '456', - ]); - expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(undefined)).toEqual([ - '123', - '456', - ]); - }); - }); - - describe('isLastUnresolvedDiscussion', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; - - it('should return true if the discussion id provided is the last', () => { - expect(getters.isLastUnresolvedDiscussion(state, localGetters)('789')).toBe(true); - }); - - it('should return false if the discussion id provided is not the last', () => { - expect(getters.isLastUnresolvedDiscussion(state, localGetters)('123')).toBe(false); - expect(getters.isLastUnresolvedDiscussion(state, localGetters)('456')).toBe(false); - }); - }); - - describe('nextUnresolvedDiscussionId', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; - - it('should return the ID of the discussion after the ID provided', () => { - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe(undefined); - }); - }); - - describe('firstUnresolvedDiscussionId', () => { - const localGetters = { - unresolvedDiscussionsIdsByDate: ['123', '456'], - unresolvedDiscussionsIdsByDiff: ['abc', 'def'], - }; - - it('should return the first discussion id by diff when diffOrder param is true', () => { - expect(getters.firstUnresolvedDiscussionId(state, localGetters)(true)).toBe('abc'); - }); - - it('should return the first discussion id by date when diffOrder param is not true', () => { - expect(getters.firstUnresolvedDiscussionId(state, localGetters)(false)).toBe('123'); - expect(getters.firstUnresolvedDiscussionId(state, localGetters)(undefined)).toBe('123'); - }); - - it('should be falsy if all discussions are resolved', () => { - const localGettersFalsy = { - unresolvedDiscussionsIdsByDiff: [], - unresolvedDiscussionsIdsByDate: [], - }; - - expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(true)).toBeFalsy(); - expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(false)).toBeFalsy(); - }); - }); }); diff --git a/spec/javascripts/pdf/page_spec.js b/spec/javascripts/pdf/page_spec.js index 9c686748c10..ff1bfd7f650 100644 --- a/spec/javascripts/pdf/page_spec.js +++ b/spec/javascripts/pdf/page_spec.js @@ -30,7 +30,7 @@ describe('Page component', () => { done(); }) .catch((error) => { - console.error(error); + done.fail(error); }); }); diff --git a/spec/javascripts/test_bundle.js b/spec/javascripts/test_bundle.js index 59e472789e2..e77236c40ef 100644 --- a/spec/javascripts/test_bundle.js +++ b/spec/javascripts/test_bundle.js @@ -1,4 +1,6 @@ -/* eslint-disable jasmine/no-global-setup, jasmine/no-unsafe-spy, no-underscore-dangle */ +/* eslint-disable + jasmine/no-global-setup, jasmine/no-unsafe-spy, no-underscore-dangle, no-console +*/ import $ from 'jquery'; import 'vendor/jasmine-jquery'; diff --git a/spec/lib/gitlab/ci/status/build/failed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_spec.rb index cadb424ea2c..b6676b40fd3 100644 --- a/spec/lib/gitlab/ci/status/build/failed_spec.rb +++ b/spec/lib/gitlab/ci/status/build/failed_spec.rb @@ -80,4 +80,31 @@ describe Gitlab::Ci::Status::Build::Failed do end end end + + describe 'covers all failure reasons' do + let(:status) { Gitlab::Ci::Status::Failed.new(build, user) } + let(:tooltip) { subject.status_tooltip } + + CommitStatus.failure_reasons.keys.each do |failure_reason| + context failure_reason do + before do + build.failure_reason = failure_reason + end + + it "is a valid status" do + expect { tooltip }.not_to raise_error + end + end + end + + context 'invalid failure message' do + before do + expect(build).to receive(:failure_reason) { 'invalid failure message' } + end + + it "is an invalid status" do + expect { tooltip }.to raise_error(/key not found:/) + end + end + end end diff --git a/spec/lib/gitlab/graphs/commits_spec.rb b/spec/lib/gitlab/graphs/commits_spec.rb index b2084f56640..530d4a981bf 100644 --- a/spec/lib/gitlab/graphs/commits_spec.rb +++ b/spec/lib/gitlab/graphs/commits_spec.rb @@ -29,7 +29,7 @@ describe Gitlab::Graphs::Commits do context 'with commits from yesterday and today' do subject { described_class.new([commit2, commit1_yesterday]) } describe '#commit_per_day' do - it { expect(subject.commit_per_day).to eq 1 } + it { expect(subject.commit_per_day).to eq 1.0 } end describe '#duration' do diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index db5aab0cd76..c175dc1e4dd 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -7,6 +7,7 @@ issues: - updated_by - milestone - notes +- resource_label_events - label_links - labels - last_edited_by @@ -76,6 +77,7 @@ merge_requests: - updated_by - milestone - notes +- resource_label_events - label_links - labels - last_edited_by diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index e4fa04baae6..6955f7f4cd8 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2409,18 +2409,18 @@ describe Ci::Build do end end - describe 'state transition: any => [:running]' do + describe '#has_valid_build_dependencies?' do shared_examples 'validation is active' do context 'when depended job has not been completed yet' do let!(:pre_stage_job) { create(:ci_build, :manual, pipeline: pipeline, name: 'test', stage_idx: 0) } - it { expect { job.run! }.not_to raise_error } + it { expect(job).to have_valid_build_dependencies } end context 'when artifacts of depended job has been expired' do let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) } - it { expect { job.run! }.to raise_error(Ci::Build::MissingDependenciesError) } + it { expect(job).not_to have_valid_build_dependencies } end context 'when artifacts of depended job has been erased' do @@ -2430,7 +2430,7 @@ describe Ci::Build do pre_stage_job.erase end - it { expect { job.run! }.to raise_error(Ci::Build::MissingDependenciesError) } + it { expect(job).not_to have_valid_build_dependencies } end end @@ -2438,12 +2438,13 @@ describe Ci::Build do context 'when depended job has not been completed yet' do let!(:pre_stage_job) { create(:ci_build, :manual, pipeline: pipeline, name: 'test', stage_idx: 0) } - it { expect { job.run! }.not_to raise_error } + it { expect(job).to have_valid_build_dependencies } end + context 'when artifacts of depended job has been expired' do let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) } - it { expect { job.run! }.not_to raise_error } + it { expect(job).to have_valid_build_dependencies } end context 'when artifacts of depended job has been erased' do @@ -2453,7 +2454,7 @@ describe Ci::Build do pre_stage_job.erase end - it { expect { job.run! }.not_to raise_error } + it { expect(job).to have_valid_build_dependencies } end end @@ -2469,13 +2470,13 @@ describe Ci::Build do context 'when "dependencies" keyword is not defined' do let(:options) { {} } - it { expect { job.run! }.not_to raise_error } + it { expect(job).to have_valid_build_dependencies } end context 'when "dependencies" keyword is empty' do let(:options) { { dependencies: [] } } - it { expect { job.run! }.not_to raise_error } + it { expect(job).to have_valid_build_dependencies } end context 'when "dependencies" keyword is specified' do @@ -2812,4 +2813,76 @@ describe Ci::Build do end end end + + describe '#publishes_artifacts_reports?' do + let(:build) { create(:ci_build, options: options) } + + subject { build.publishes_artifacts_reports? } + + context 'when artifacts reports are defined' do + let(:options) do + { artifacts: { reports: { junit: "junit.xml" } } } + end + + it { is_expected.to be_truthy } + end + + context 'when artifacts reports missing defined' do + let(:options) do + { artifacts: { paths: ["file.txt"] } } + end + + it { is_expected.to be_falsey } + end + + context 'when options are missing' do + let(:options) { nil } + + it { is_expected.to be_falsey } + end + end + + describe '#runner_required_feature_names' do + let(:build) { create(:ci_build, options: options) } + + subject { build.runner_required_feature_names } + + context 'when artifacts reports are defined' do + let(:options) do + { artifacts: { reports: { junit: "junit.xml" } } } + end + + it { is_expected.to include(:upload_multiple_artifacts) } + end + end + + describe '#supported_runner?' do + set(:build) { create(:ci_build) } + + subject { build.supported_runner?(runner_features) } + + context 'when feature is required by build' do + before do + expect(build).to receive(:runner_required_feature_names) do + [:upload_multiple_artifacts] + end + end + + context 'when runner provides given feature' do + let(:runner_features) do + { upload_multiple_artifacts: true } + end + + it { is_expected.to be_truthy } + end + + context 'when runner does not provide given feature' do + let(:runner_features) do + {} + end + + it { is_expected.to be_falsey } + end + end + end end diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb index 581fd0293cc..20600f5fa38 100644 --- a/spec/models/internal_id_spec.rb +++ b/spec/models/internal_id_spec.rb @@ -79,6 +79,46 @@ describe InternalId do end end + describe '.track_greatest' do + let(:value) { 9001 } + subject { described_class.track_greatest(issue, scope, usage, value, init) } + + context 'in the absence of a record' do + it 'creates a record if not yet present' do + expect { subject }.to change { described_class.count }.from(0).to(1) + end + end + + it 'stores record attributes' do + subject + + described_class.first.tap do |record| + expect(record.project).to eq(project) + expect(record.usage).to eq(usage.to_s) + expect(record.last_value).to eq(value) + end + end + + context 'with existing issues' do + before do + create(:issue, project: project) + described_class.delete_all + end + + it 'still returns the last value to that of the given value' do + expect(subject).to eq(value) + end + end + + context 'when value is less than the current last_value' do + it 'returns the current last_value' do + described_class.create!(**scope, usage: usage, last_value: 10_001) + + expect(subject).to eq 10_001 + end + end + end + describe '#increment_and_save!' do let(:id) { create(:internal_id) } subject { id.increment_and_save! } @@ -103,4 +143,30 @@ describe InternalId do end end end + + describe '#track_greatest_and_save!' do + let(:id) { create(:internal_id) } + let(:new_last_value) { 9001 } + subject { id.track_greatest_and_save!(new_last_value) } + + it 'returns new last value' do + expect(subject).to eq new_last_value + end + + it 'saves the record' do + subject + + expect(id.changed?).to be_falsey + end + + context 'when new last value is lower than the max' do + it 'does not update the last value' do + id.update!(last_value: 10_001) + + subject + + expect(id.reload.last_value).to eq 10_001 + end + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 8914845ea82..99670af786a 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -139,4 +139,20 @@ describe Label do end end end + + describe '.search' do + let(:label) { create(:label, title: 'bug', description: 'incorrect behavior') } + + it 'returns labels with a partially matching title' do + expect(described_class.search(label.title[0..2])).to eq([label]) + end + + it 'returns labels with a partially matching description' do + expect(described_class.search(label.description[0..5])).to eq([label]) + end + + it 'returns nothing' do + expect(described_class.search('feature')).to be_empty + end + end end diff --git a/spec/models/resource_label_event_spec.rb b/spec/models/resource_label_event_spec.rb new file mode 100644 index 00000000000..4756caa1b97 --- /dev/null +++ b/spec/models/resource_label_event_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ResourceLabelEvent, type: :model do + subject { build(:resource_label_event) } + let(:issue) { create(:issue) } + let(:merge_request) { create(:merge_request) } + + describe 'associations' do + it { is_expected.to belong_to(:user) } + it { is_expected.to belong_to(:issue) } + it { is_expected.to belong_to(:merge_request) } + it { is_expected.to belong_to(:label) } + end + + describe 'validations' do + it { is_expected.to be_valid } + it { is_expected.to validate_presence_of(:label) } + it { is_expected.to validate_presence_of(:user) } + + describe 'Issuable validation' do + it 'is invalid if issue_id and merge_request_id are missing' do + subject.attributes = { issue: nil, merge_request: nil } + + expect(subject).to be_invalid + end + + it 'is invalid if issue_id and merge_request_id are set' do + subject.attributes = { issue: issue, merge_request: merge_request } + + expect(subject).to be_invalid + end + + it 'is valid if only issue_id is set' do + subject.attributes = { issue: issue, merge_request: nil } + + expect(subject).to be_valid + end + + it 'is valid if only merge_request_id is set' do + subject.attributes = { merge_request: merge_request, issue: nil } + + expect(subject).to be_valid + end + end + end +end diff --git a/spec/presenters/commit_status_presenter_spec.rb b/spec/presenters/commit_status_presenter_spec.rb index f81ee44e371..2b7742ddbb8 100644 --- a/spec/presenters/commit_status_presenter_spec.rb +++ b/spec/presenters/commit_status_presenter_spec.rb @@ -12,4 +12,30 @@ describe CommitStatusPresenter do it 'inherits from Gitlab::View::Presenter::Delegated' do expect(described_class.superclass).to eq(Gitlab::View::Presenter::Delegated) end + + describe 'covers all failure reasons' do + let(:message) { presenter.callout_failure_message } + + CommitStatus.failure_reasons.keys.each do |failure_reason| + context failure_reason do + before do + build.failure_reason = failure_reason + end + + it "is a valid status" do + expect { message }.not_to raise_error + end + end + end + + context 'invalid failure message' do + before do + expect(build).to receive(:failure_reason) { 'invalid failure message' } + end + + it "is an invalid status" do + expect { message }.to raise_error(/key not found:/) + end + end + end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index a2cfa706f58..b537b6e1667 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -152,7 +152,7 @@ describe API::Internal do context 'user key' do it 'returns the correct information about the key' do - lfs_auth(key.id, project) + lfs_auth_key(key.id, project) expect(response).to have_gitlab_http_status(200) expect(json_response['username']).to eq(user.username) @@ -161,8 +161,30 @@ describe API::Internal do expect(json_response['repository_http_path']).to eq(project.http_url_to_repo) end + it 'returns the correct information about the user' do + lfs_auth_user(user.id, project) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['username']).to eq(user.username) + expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).token) + + expect(json_response['repository_http_path']).to eq(project.http_url_to_repo) + end + + it 'returns a 404 when no key or user is provided' do + lfs_auth_project(project) + + expect(response).to have_gitlab_http_status(404) + end + it 'returns a 404 when the wrong key is provided' do - lfs_auth(nil, project) + lfs_auth_key(key.id + 12345, project) + + expect(response).to have_gitlab_http_status(404) + end + + it 'returns a 404 when the wrong user is provided' do + lfs_auth_user(user.id + 12345, project) expect(response).to have_gitlab_http_status(404) end @@ -172,7 +194,7 @@ describe API::Internal do let(:key) { create(:deploy_key) } it 'returns the correct information about the key' do - lfs_auth(key.id, project) + lfs_auth_key(key.id, project) expect(response).to have_gitlab_http_status(200) expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}") @@ -183,13 +205,29 @@ describe API::Internal do end describe "GET /internal/discover" do - it do + it "finds a user by key id" do get(api("/internal/discover"), key_id: key.id, secret_token: secret_token) expect(response).to have_gitlab_http_status(200) expect(json_response['name']).to eq(user.name) end + + it "finds a user by user id" do + get(api("/internal/discover"), user_id: user.id, secret_token: secret_token) + + expect(response).to have_gitlab_http_status(200) + + expect(json_response['name']).to eq(user.name) + end + + it "finds a user by username" do + get(api("/internal/discover"), username: user.username, secret_token: secret_token) + + expect(response).to have_gitlab_http_status(200) + + expect(json_response['name']).to eq(user.name) + end end describe "GET /internal/authorized_keys" do @@ -871,7 +909,15 @@ describe API::Internal do ) end - def lfs_auth(key_id, project) + def lfs_auth_project(project) + post( + api("/internal/lfs_authenticate"), + secret_token: secret_token, + project: project.full_path + ) + end + + def lfs_auth_key(key_id, project) post( api("/internal/lfs_authenticate"), key_id: key_id, @@ -879,4 +925,13 @@ describe API::Internal do project: project.full_path ) end + + def lfs_auth_user(user_id, project) + post( + api("/internal/lfs_authenticate"), + user_id: user_id, + secret_token: secret_token, + project: project.full_path + ) + end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 66eb18229fa..28ba00c7293 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -1002,6 +1002,38 @@ describe API::Issues do end end + context 'an internal ID is provided' do + context 'by an admin' do + it 'sets the internal ID on the new issue' do + post api("/projects/#{project.id}/issues", admin), + title: 'new issue', iid: 9001 + + expect(response).to have_gitlab_http_status(201) + expect(json_response['iid']).to eq 9001 + end + end + + context 'by an owner' do + it 'sets the internal ID on the new issue' do + post api("/projects/#{project.id}/issues", user), + title: 'new issue', iid: 9001 + + expect(response).to have_gitlab_http_status(201) + expect(json_response['iid']).to eq 9001 + end + end + + context 'by another user' do + it 'ignores the given internal ID' do + post api("/projects/#{project.id}/issues", user2), + title: 'new issue', iid: 9001 + + expect(response).to have_gitlab_http_status(201) + expect(json_response['iid']).not_to eq 9001 + end + end + end + it 'creates a new project issue' do post api("/projects/#{project.id}/issues", user), title: 'new issue', labels: 'label, label2', weight: 3, diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb index 6f40a02aaa9..e042d772718 100644 --- a/spec/requests/jwt_controller_spec.rb +++ b/spec/requests/jwt_controller_spec.rb @@ -70,6 +70,25 @@ describe JwtController do it { expect(service_class).to have_received(:new).with(nil, user, parameters) } + context 'when passing a flat array of scopes' do + # We use this trick to make rails to generate a query_string: + # scope=scope1&scope=scope2 + # It works because :scope and 'scope' are the same as string, but different objects + let(:parameters) do + { + :service => service_name, + :scope => 'scope1', + 'scope' => 'scope2' + } + end + + let(:service_parameters) do + { service: service_name, scopes: %w(scope1 scope2) } + end + + it { expect(service_class).to have_received(:new).with(nil, user, service_parameters) } + end + context 'when user has 2FA enabled' do let(:user) { create(:user, :two_factor) } diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index c2378646f89..e349181b794 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -732,7 +732,7 @@ describe 'Git LFS API and storage' do expect(json_response['objects'].first['oid']).to eq(sample_oid) expect(json_response['objects'].first['size']).to eq(sample_size) expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{Gitlab.config.gitlab.url}/#{project.full_path}.git/gitlab-lfs/objects/#{sample_oid}/#{sample_size}") - expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization) + expect(json_response['objects'].first['actions']['upload']['header']).to eq({ 'Authorization' => authorization, 'Content-Type' => 'application/octet-stream' }) end end @@ -761,7 +761,7 @@ describe 'Git LFS API and storage' do expect(lfs_object.projects.pluck(:id)).not_to include(project.id) expect(lfs_object.projects.pluck(:id)).to include(other_project.id) expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}") - expect(json_response['objects'].first['actions']['upload']['header']).to eq('Authorization' => authorization) + expect(json_response['objects'].first['actions']['upload']['header']).to eq({ 'Authorization' => authorization, 'Content-Type' => 'application/octet-stream' }) end end @@ -796,7 +796,7 @@ describe 'Git LFS API and storage' do expect(json_response['objects'].first['oid']).to eq("91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897") expect(json_response['objects'].first['size']).to eq(1575078) expect(json_response['objects'].first['actions']['upload']['href']).to eq("#{project.http_url_to_repo}/gitlab-lfs/objects/91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897/1575078") - expect(json_response['objects'].first['actions']['upload']['header']).to eq("Authorization" => authorization) + expect(json_response['objects'].first['actions']['upload']['header']).to eq({ 'Authorization' => authorization, 'Content-Type' => 'application/octet-stream' }) expect(json_response['objects'].last['oid']).to eq(sample_oid) expect(json_response['objects'].last['size']).to eq(sample_size) diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 037484931b8..c7f88e45c84 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -142,7 +142,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'for registry catalog' do let(:current_params) do - { scope: "registry:catalog:*" } + { scopes: ["registry:catalog:*"] } end context 'disallow browsing for users without Gitlab admin rights' do @@ -164,7 +164,7 @@ describe Auth::ContainerRegistryAuthenticationService do end let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'a pushable' @@ -177,7 +177,7 @@ describe Auth::ContainerRegistryAuthenticationService do end let(:current_params) do - { scope: "repository:#{project.full_path}:*" } + { scopes: ["repository:#{project.full_path}:*"] } end it_behaves_like 'an inaccessible' @@ -191,7 +191,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pulling from root level repository' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end it_behaves_like 'a pullable' @@ -205,7 +205,7 @@ describe Auth::ContainerRegistryAuthenticationService do end let(:current_params) do - { scope: "repository:#{project.full_path}:*" } + { scopes: ["repository:#{project.full_path}:*"] } end it_behaves_like 'an inaccessible' @@ -218,7 +218,7 @@ describe Auth::ContainerRegistryAuthenticationService do end let(:current_params) do - { scope: "repository:#{project.full_path}:push,pull" } + { scopes: ["repository:#{project.full_path}:push,pull"] } end it_behaves_like 'a pullable' @@ -231,7 +231,7 @@ describe Auth::ContainerRegistryAuthenticationService do end let(:current_params) do - { scope: "repository:#{project.full_path}:pull,push" } + { scopes: ["repository:#{project.full_path}:pull,push"] } end it_behaves_like 'an inaccessible' @@ -244,7 +244,7 @@ describe Auth::ContainerRegistryAuthenticationService do end let(:current_params) do - { scope: "repository:#{project.full_path}:*" } + { scopes: ["repository:#{project.full_path}:*"] } end it_behaves_like 'an inaccessible' @@ -257,7 +257,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'allow anyone to pull images' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end it_behaves_like 'a pullable' @@ -266,7 +266,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow anyone to push images' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'an inaccessible' @@ -275,7 +275,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow anyone to delete images' do let(:current_params) do - { scope: "repository:#{project.full_path}:*" } + { scopes: ["repository:#{project.full_path}:*"] } end it_behaves_like 'an inaccessible' @@ -284,7 +284,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when repository name is invalid' do let(:current_params) do - { scope: 'repository:invalid:push' } + { scopes: ['repository:invalid:push'] } end it_behaves_like 'an inaccessible' @@ -298,7 +298,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'for internal user' do context 'allow anyone to pull images' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end it_behaves_like 'a pullable' @@ -307,7 +307,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow anyone to push images' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'an inaccessible' @@ -316,7 +316,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow anyone to delete images' do let(:current_params) do - { scope: "repository:#{project.full_path}:*" } + { scopes: ["repository:#{project.full_path}:*"] } end it_behaves_like 'an inaccessible' @@ -328,7 +328,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow anyone to pull or push images' do let(:current_user) { create(:user, external: true) } let(:current_params) do - { scope: "repository:#{project.full_path}:pull,push" } + { scopes: ["repository:#{project.full_path}:pull,push"] } end it_behaves_like 'an inaccessible' @@ -338,7 +338,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow anyone to delete images' do let(:current_user) { create(:user, external: true) } let(:current_params) do - { scope: "repository:#{project.full_path}:*" } + { scopes: ["repository:#{project.full_path}:*"] } end it_behaves_like 'an inaccessible' @@ -364,7 +364,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'allow to delete images' do let(:current_params) do - { scope: "repository:#{current_project.full_path}:*" } + { scopes: ["repository:#{current_project.full_path}:*"] } end it_behaves_like 'a deletable' do @@ -397,7 +397,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'allow to pull and push images' do let(:current_params) do - { scope: "repository:#{current_project.full_path}:pull,push" } + { scopes: ["repository:#{current_project.full_path}:pull,push"] } end it_behaves_like 'a pullable and pushable' do @@ -411,7 +411,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow to delete images' do let(:current_params) do - { scope: "repository:#{current_project.full_path}:*" } + { scopes: ["repository:#{current_project.full_path}:*"] } end it_behaves_like 'an inaccessible' do @@ -422,7 +422,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'for other projects' do context 'when pulling' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end context 'allow for public' do @@ -489,7 +489,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pushing' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end context 'disallow for all' do @@ -523,7 +523,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'disallow when pulling' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end it_behaves_like 'an inaccessible' @@ -534,14 +534,66 @@ describe Auth::ContainerRegistryAuthenticationService do context 'registry catalog browsing authorized as admin' do let(:current_user) { create(:user, :admin) } + let(:project) { create(:project, :public) } let(:current_params) do - { scope: "registry:catalog:*" } + { scopes: ["registry:catalog:*"] } end it_behaves_like 'a browsable' end + context 'support for multiple scopes' do + let(:internal_project) { create(:project, :internal) } + let(:private_project) { create(:project, :private) } + + let(:current_params) do + { + scopes: [ + "repository:#{internal_project.full_path}:pull", + "repository:#{private_project.full_path}:pull" + ] + } + end + + context 'user has access to all projects' do + let(:current_user) { create(:user, :admin) } + + it_behaves_like 'a browsable' do + let(:access) do + [ + { 'type' => 'repository', + 'name' => internal_project.full_path, + 'actions' => ['pull'] }, + { 'type' => 'repository', + 'name' => private_project.full_path, + 'actions' => ['pull'] } + ] + end + end + end + + context 'user only has access to internal project' do + let(:current_user) { create(:user) } + + it_behaves_like 'a browsable' do + let(:access) do + [ + { 'type' => 'repository', + 'name' => internal_project.full_path, + 'actions' => ['pull'] } + ] + end + end + end + + context 'anonymous access is rejected' do + let(:current_user) { nil } + + it_behaves_like 'a forbidden' + end + end + context 'unauthorized' do context 'disallow to use scope-less authentication' do it_behaves_like 'a forbidden' @@ -550,7 +602,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'for invalid scope' do let(:current_params) do - { scope: 'invalid:aa:bb' } + { scopes: ['invalid:aa:bb'] } end it_behaves_like 'a forbidden' @@ -561,7 +613,7 @@ describe Auth::ContainerRegistryAuthenticationService do let(:project) { create(:project, :private) } let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end it_behaves_like 'a forbidden' @@ -572,7 +624,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pulling and pushing' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull,push" } + { scopes: ["repository:#{project.full_path}:pull,push"] } end it_behaves_like 'a pullable' @@ -581,7 +633,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pushing' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'a forbidden' @@ -591,7 +643,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'for registry catalog' do let(:current_params) do - { scope: "registry:catalog:*" } + { scopes: ["registry:catalog:*"] } end it_behaves_like 'a forbidden' @@ -601,7 +653,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'for deploy tokens' do let(:current_params) do - { scope: "repository:#{project.full_path}:pull" } + { scopes: ["repository:#{project.full_path}:pull"] } end context 'when deploy token has read_registry as a scope' do @@ -616,7 +668,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pushing' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'an inaccessible' @@ -632,7 +684,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pushing' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'an inaccessible' @@ -648,7 +700,7 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pushing' do let(:current_params) do - { scope: "repository:#{project.full_path}:push" } + { scopes: ["repository:#{project.full_path}:push"] } end it_behaves_like 'an inaccessible' @@ -734,4 +786,26 @@ describe Auth::ContainerRegistryAuthenticationService do end end end + + context 'user authorization' do + let(:current_user) { create(:user) } + + context 'with multiple scopes' do + let(:project) { create(:project) } + let(:project2) { create } + + context 'allow developer to push images' do + before do + project.add_developer(current_user) + end + + let(:current_params) do + { scopes: ["repository:#{project.full_path}:push"] } + end + + it_behaves_like 'a pushable' + it_behaves_like 'container repository factory' + end + end + end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index dbb5e33bbdc..a6565709641 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -351,6 +351,38 @@ module Ci end end + context 'runner feature set is verified' do + let!(:pending_job) { create(:ci_build, :pending, pipeline: pipeline) } + + before do + expect_any_instance_of(Ci::Build).to receive(:runner_required_feature_names) do + [:runner_required_feature] + end + end + + subject { execute(specific_runner, params) } + + context 'when feature is missing by runner' do + let(:params) { {} } + + it 'does not pick the build and drops the build' do + expect(subject).to be_nil + expect(pending_job.reload).to be_failed + expect(pending_job).to be_runner_unsupported + end + end + + context 'when feature is supported by runner' do + let(:params) do + { info: { features: { runner_required_feature: true } } } + end + + it 'does pick job' do + expect(subject).not_to be_nil + end + end + end + context 'when "dependencies" keyword is specified' do shared_examples 'not pick' do it 'does not pick the build and drops the build' do @@ -403,6 +435,7 @@ module Ci it { expect(subject).to eq(pending_job) } end + context 'when artifacts of depended job has been expired' do let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) } diff --git a/spec/services/clusters/applications/check_installation_progress_service_spec.rb b/spec/services/clusters/applications/check_installation_progress_service_spec.rb index 6894c1797b0..986f11410fd 100644 --- a/spec/services/clusters/applications/check_installation_progress_service_spec.rb +++ b/spec/services/clusters/applications/check_installation_progress_service_spec.rb @@ -43,7 +43,7 @@ describe Clusters::Applications::CheckInstallationProgressService do service.execute expect(application).to be_errored - expect(application.status_reason).to match(/\btimeouted\b/) + expect(application.status_reason).to match(/\btimed out\b/) end end end diff --git a/spec/services/projects/create_from_template_service_spec.rb b/spec/services/projects/create_from_template_service_spec.rb index a43da01f37e..141ccf7c4d8 100644 --- a/spec/services/projects/create_from_template_service_spec.rb +++ b/spec/services/projects/create_from_template_service_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' describe Projects::CreateFromTemplateService do let(:user) { create(:user) } + let(:template_name) { 'rails' } let(:project_params) do { path: user.to_param, - template_name: 'rails', + template_name: template_name, description: 'project description', visibility_level: Gitlab::VisibilityLevel::PUBLIC } @@ -14,7 +15,10 @@ describe Projects::CreateFromTemplateService do subject { described_class.new(user, project_params) } it 'calls the importer service' do - expect_any_instance_of(Projects::GitlabProjectsImportService).to receive(:execute) + import_service_double = double + + allow(Projects::GitlabProjectsImportService).to receive(:new).and_return(import_service_double) + expect(import_service_double).to receive(:execute) subject.execute end @@ -26,6 +30,31 @@ describe Projects::CreateFromTemplateService do expect(project.import_scheduled?).to be(true) end + context 'when template is not present' do + let(:template_name) { 'non_existent' } + let(:project) { subject.execute } + + before do + expect(project).to be_saved + end + + it 'does not set import set import type' do + expect(project.import_type).to be nil + end + + it 'does not set import set import source' do + expect(project.import_source).to be nil + end + + it 'is not scheduled' do + expect(project.import_scheduled?).to be(false) + end + + it 'repository is empty' do + expect(project.repository.empty?).to be(true) + end + end + context 'the result project' do before do perform_enqueued_jobs do diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb index 0a898e9b89b..a2061127698 100644 --- a/spec/services/projects/gitlab_projects_import_service_spec.rb +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -6,60 +6,10 @@ describe Projects::GitlabProjectsImportService do let(:file) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } let(:overwrite) { false } let(:import_params) { { namespace_id: namespace.id, path: path, file: file, overwrite: overwrite } } + subject { described_class.new(namespace.owner, import_params) } describe '#execute' do - context 'with an invalid path' do - let(:path) { '/invalid-path/' } - - it 'returns an invalid project' do - project = subject.execute - - expect(project).not_to be_persisted - expect(project).not_to be_valid - end - end - - context 'with a valid path' do - it 'creates a project' do - project = subject.execute - - expect(project).to be_persisted - expect(project).to be_valid - end - end - - context 'override params' do - it 'stores them as import data when passed' do - project = described_class - .new(namespace.owner, import_params, description: 'Hello') - .execute - - expect(project.import_data.data['override_params']['description']).to eq('Hello') - end - end - - context 'when there is a project with the same path' do - let(:existing_project) { create(:project, namespace: namespace) } - let(:path) { existing_project.path} - - it 'does not create the project' do - project = subject.execute - - expect(project).to be_invalid - expect(project).not_to be_persisted - end - - context 'when overwrite param is set' do - let(:overwrite) { true } - - it 'creates a project in a temporary full_path' do - project = subject.execute - - expect(project).to be_valid - expect(project).to be_persisted - end - end - end + it_behaves_like 'gitlab projects import validations' end end diff --git a/spec/services/resource_events/change_labels_service_spec.rb b/spec/services/resource_events/change_labels_service_spec.rb new file mode 100644 index 00000000000..41b0fb3eea3 --- /dev/null +++ b/spec/services/resource_events/change_labels_service_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ResourceEvents::ChangeLabelsService do + set(:project) { create(:project) } + set(:author) { create(:user) } + let(:resource) { create(:issue, project: project) } + + describe '.change_labels' do + subject { described_class.new(resource, author).execute(added_labels: added, removed_labels: removed) } + + let(:labels) { create_list(:label, 2, project: project) } + + def expect_label_event(event, label, action) + expect(event.user).to eq(author) + expect(event.label).to eq(label) + expect(event.action).to eq(action) + end + + context 'when adding a label' do + let(:added) { [labels[0]] } + let(:removed) { [] } + + it 'creates new label event' do + expect { subject }.to change { resource.resource_label_events.count }.from(0).to(1) + + expect_label_event(resource.resource_label_events.first, labels[0], 'add') + end + end + + context 'when removing a label' do + let(:added) { [] } + let(:removed) { [labels[1]] } + + it 'creates new label event' do + expect { subject }.to change { resource.resource_label_events.count }.from(0).to(1) + + expect_label_event(resource.resource_label_events.first, labels[1], 'remove') + end + end + + context 'when both adding and removing labels' do + let(:added) { [labels[0]] } + let(:removed) { [labels[1]] } + + it 'creates all label events in a single query' do + expect(Gitlab::Database).to receive(:bulk_insert).once.and_call_original + expect { subject }.to change { resource.resource_label_events.count }.from(0).to(2) + end + end + end +end diff --git a/spec/support/shared_examples/gitlab_projects_import_service_shared_examples.rb b/spec/support/shared_examples/gitlab_projects_import_service_shared_examples.rb new file mode 100644 index 00000000000..b8db35a6ef9 --- /dev/null +++ b/spec/support/shared_examples/gitlab_projects_import_service_shared_examples.rb @@ -0,0 +1,54 @@ +shared_examples 'gitlab projects import validations' do + context 'with an invalid path' do + let(:path) { '/invalid-path/' } + + it 'returns an invalid project' do + project = subject.execute + + expect(project).not_to be_persisted + expect(project).not_to be_valid + end + end + + context 'with a valid path' do + it 'creates a project' do + project = subject.execute + + expect(project).to be_persisted + expect(project).to be_valid + end + end + + context 'override params' do + it 'stores them as import data when passed' do + project = described_class + .new(namespace.owner, import_params, description: 'Hello') + .execute + + expect(project.import_data.data['override_params']['description']).to eq('Hello') + end + end + + context 'when there is a project with the same path' do + let(:existing_project) { create(:project, namespace: namespace) } + let(:path) { existing_project.path} + + it 'does not create the project' do + project = subject.execute + + expect(project).to be_invalid + expect(project).not_to be_persisted + end + + context 'when overwrite param is set' do + let(:overwrite) { true } + + it 'creates a project in a temporary full_path' do + project = subject.execute + + expect(project).to be_valid + expect(project).to be_persisted + end + end + end +end diff --git a/spec/support/shared_examples/models/atomic_internal_id_spec.rb b/spec/support/shared_examples/models/atomic_internal_id_spec.rb index 7ab1041d17c..c659be8f13a 100644 --- a/spec/support/shared_examples/models/atomic_internal_id_spec.rb +++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb @@ -60,6 +60,20 @@ shared_examples_for 'AtomicInternalId' do |validate_presence: true| expect { subject }.not_to change { instance.public_send(internal_id_attribute) } end + + context 'when the instance has an internal ID set' do + let(:internal_id) { 9001 } + + it 'calls InternalId.update_last_value and sets the `last_value` to that of the instance' do + instance.send("#{internal_id_attribute}=", internal_id) + + expect(InternalId) + .to receive(:track_greatest) + .with(instance, scope_attrs, usage, internal_id, any_args) + .and_return(internal_id) + subject + end + end end end end diff --git a/spec/tasks/gitlab/cleanup_rake_spec.rb b/spec/tasks/gitlab/cleanup_rake_spec.rb index 2bf873c923f..ba08ece1b4b 100644 --- a/spec/tasks/gitlab/cleanup_rake_spec.rb +++ b/spec/tasks/gitlab/cleanup_rake_spec.rb @@ -5,7 +5,7 @@ describe 'gitlab:cleanup rake tasks' do Rake.application.rake_require 'tasks/gitlab/cleanup' end - describe 'cleanup' do + describe 'cleanup namespaces and repos' do let(:storages) do { 'default' => Gitlab::GitalyClient::StorageSettings.new(@default_storage_hash.merge('path' => 'tmp/tests/default_storage')) @@ -67,4 +67,319 @@ describe 'gitlab:cleanup rake tasks' do end end end + + describe 'cleanup:project_uploads' do + context 'orphaned project upload file' do + context 'when an upload record matching the secret and filename is found' do + context 'when the project is still in legacy storage' do + let!(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) } + let!(:correct_path) { orphaned.absolute_path } + let!(:other_project) { create(:project, :legacy_storage) } + let!(:orphaned_path) { correct_path.sub(/#{orphaned.model.full_path}/, other_project.full_path) } + + before do + FileUtils.mkdir_p(File.dirname(orphaned_path)) + FileUtils.mv(correct_path, orphaned_path) + end + + it 'moves the file to its proper location' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Did fix #{orphaned_path} -> #{correct_path}") + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(correct_path)).to be_falsey + + stub_env('DRY_RUN', 'false') + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_falsey + expect(File.exist?(correct_path)).to be_truthy + end + + it 'a dry run does not move the file' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Can fix #{orphaned_path} -> #{correct_path}") + expect(Rails.logger).to receive(:info) + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(correct_path)).to be_falsey + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(correct_path)).to be_falsey + end + + context 'when the project record is missing (Upload#absolute_path raises error)' do + let!(:lost_and_found_path) { File.join(FileUploader.root, '-', 'project-lost-found', other_project.full_path, orphaned.path) } + + before do + orphaned.model.delete + end + + it 'moves the file to lost and found' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Did move to lost and found #{orphaned_path} -> #{lost_and_found_path}") + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + + stub_env('DRY_RUN', 'false') + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_falsey + expect(File.exist?(lost_and_found_path)).to be_truthy + end + + it 'a dry run does not move the file' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Can move to lost and found #{orphaned_path} -> #{lost_and_found_path}") + expect(Rails.logger).to receive(:info) + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + end + end + end + + context 'when the project was moved to hashed storage' do + let!(:orphaned) { create(:upload, :issuable_upload, :with_file) } + let!(:correct_path) { orphaned.absolute_path } + let!(:orphaned_path) { File.join(FileUploader.root, 'foo', 'bar', orphaned.path) } + + before do + FileUtils.mkdir_p(File.dirname(orphaned_path)) + FileUtils.mv(correct_path, orphaned_path) + end + + it 'moves the file to its proper location' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Did fix #{orphaned_path} -> #{correct_path}") + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(correct_path)).to be_falsey + + stub_env('DRY_RUN', 'false') + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_falsey + expect(File.exist?(correct_path)).to be_truthy + end + + it 'a dry run does not move the file' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Can fix #{orphaned_path} -> #{correct_path}") + expect(Rails.logger).to receive(:info) + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(correct_path)).to be_falsey + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(correct_path)).to be_falsey + end + end + end + + context 'when a matching upload record can not be found' do + context 'when the file path fits the known pattern' do + let!(:orphaned) { create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) } + let!(:orphaned_path) { orphaned.absolute_path } + let!(:lost_and_found_path) { File.join(FileUploader.root, '-', 'project-lost-found', orphaned.model.full_path, orphaned.path) } + + before do + orphaned.delete + end + + it 'moves the file to lost and found' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Did move to lost and found #{orphaned_path} -> #{lost_and_found_path}") + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + + stub_env('DRY_RUN', 'false') + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_falsey + expect(File.exist?(lost_and_found_path)).to be_truthy + end + + it 'a dry run does not move the file' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Can move to lost and found #{orphaned_path} -> #{lost_and_found_path}") + expect(Rails.logger).to receive(:info) + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + end + end + + context 'when the file path does not fit the known pattern' do + let!(:invalid_path) { File.join('group', 'file.jpg') } + let!(:orphaned_path) { File.join(FileUploader.root, invalid_path) } + let!(:lost_and_found_path) { File.join(FileUploader.root, '-', 'project-lost-found', invalid_path) } + + before do + FileUtils.mkdir_p(File.dirname(orphaned_path)) + FileUtils.touch(orphaned_path) + end + + after do + File.delete(orphaned_path) if File.exist?(orphaned_path) + end + + it 'moves the file to lost and found' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Did move to lost and found #{orphaned_path} -> #{lost_and_found_path}") + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + + stub_env('DRY_RUN', 'false') + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_falsey + expect(File.exist?(lost_and_found_path)).to be_truthy + end + + it 'a dry run does not move the file' do + expect(Rails.logger).to receive(:info).twice + expect(Rails.logger).to receive(:info).with("Can move to lost and found #{orphaned_path} -> #{lost_and_found_path}") + expect(Rails.logger).to receive(:info) + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_truthy + expect(File.exist?(lost_and_found_path)).to be_falsey + end + end + end + end + + context 'non-orphaned project upload file' do + it 'does not move the file' do + tracked = create(:upload, :issuable_upload, :with_file, model: build(:project, :legacy_storage)) + tracked_path = tracked.absolute_path + + expect(Rails.logger).not_to receive(:info).with(/move|fix/i) + expect(File.exist?(tracked_path)).to be_truthy + + stub_env('DRY_RUN', 'false') + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(tracked_path)).to be_truthy + end + end + + context 'ignorable cases' do + shared_examples_for 'does not move anything' do + it 'does not move even an orphan file' do + orphaned = create(:upload, :issuable_upload, :with_file, model: project) + orphaned_path = orphaned.absolute_path + orphaned.delete + + expect(File.exist?(orphaned_path)).to be_truthy + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(orphaned_path)).to be_truthy + end + end + + # Because we aren't concerned about these, and can save a lot of + # processing time by ignoring them. If we wish to cleanup hashed storage + # directories, it should simply require removing this test and modifying + # the find command. + context 'when the file is already in hashed storage' do + let(:project) { create(:project) } + + before do + stub_env('DRY_RUN', 'false') + expect(Rails.logger).not_to receive(:info).with(/move|fix/i) + end + + it_behaves_like 'does not move anything' + end + + context 'when DRY_RUN env var is unset' do + let(:project) { create(:project, :legacy_storage) } + + it_behaves_like 'does not move anything' + end + + context 'when DRY_RUN env var is true' do + let(:project) { create(:project, :legacy_storage) } + + before do + stub_env('DRY_RUN', 'true') + end + + it_behaves_like 'does not move anything' + end + + context 'when DRY_RUN env var is foo' do + let(:project) { create(:project, :legacy_storage) } + + before do + stub_env('DRY_RUN', 'foo') + end + + it_behaves_like 'does not move anything' + end + + it 'does not move any non-project (FileUploader) uploads' do + stub_env('DRY_RUN', 'false') + + paths = [] + orphaned1 = create(:upload, :personal_snippet_upload, :with_file) + orphaned2 = create(:upload, :namespace_upload, :with_file) + orphaned3 = create(:upload, :attachment_upload, :with_file) + paths << orphaned1.absolute_path + paths << orphaned2.absolute_path + paths << orphaned3.absolute_path + Upload.delete_all + + expect(Rails.logger).not_to receive(:info).with(/move|fix/i) + paths.each do |path| + expect(File.exist?(path)).to be_truthy + end + + run_rake_task('gitlab:cleanup:project_uploads') + + paths.each do |path| + expect(File.exist?(path)).to be_truthy + end + end + + it 'does not move any uploads in tmp (which would interfere with ongoing upload activity)' do + stub_env('DRY_RUN', 'false') + + path = File.join(FileUploader.root, 'tmp', 'foo.jpg') + FileUtils.mkdir_p(File.dirname(path)) + FileUtils.touch(path) + + expect(Rails.logger).not_to receive(:info).with(/move|fix/i) + expect(File.exist?(path)).to be_truthy + + run_rake_task('gitlab:cleanup:project_uploads') + + expect(File.exist?(path)).to be_truthy + end + end + end end |