diff options
93 files changed, 1311 insertions, 1127 deletions
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index c07850b1a4f..c6d32ffef34 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -6,6 +6,7 @@ import { __ } from '~/locale'; import createFlash from '~/flash'; import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; +import { isSingleViewStyle } from '~/helpers/diffs_helper'; import eventHub from '../../notes/event_hub'; import CompareVersions from './compare_versions.vue'; import DiffFile from './diff_file.vue'; @@ -145,6 +146,9 @@ export default { }, watch: { diffViewType() { + if (this.needsReload() || this.needsFirstLoad()) { + this.refetchDiffData(); + } this.adjustView(); }, shouldShow() { @@ -224,6 +228,16 @@ export default { { timeout: 1000 }, ); }, + needsReload() { + return ( + this.glFeatures.singleMrDiffView && + this.diffFiles.length && + isSingleViewStyle(this.diffFiles[0]) + ); + }, + needsFirstLoad() { + return this.glFeatures.singleMrDiffView && !this.diffFiles.length; + }, fetchData(toggleTree = true) { if (this.glFeatures.diffsBatchLoad) { this.fetchDiffFilesMeta() @@ -237,6 +251,13 @@ export default { }); this.fetchDiffFilesBatch() + .then(() => { + // Guarantee the discussions are assigned after the batch finishes. + // Just watching the length of the discussions or the diff files + // isn't enough, because with split diff loading, neither will + // change when loading the other half of the diff files. + this.setDiscussions(); + }) .then(() => this.startDiffRendering()) .catch(() => { createFlash(__('Something went wrong on our end. Please try again!')); @@ -250,6 +271,7 @@ export default { requestIdleCallback( () => { + this.setDiscussions(); this.startRenderDiffsQueue(); }, { timeout: 1000 }, diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 0dbff4ffcec..f5051748f10 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -4,6 +4,7 @@ import _ from 'underscore'; import { GlLoadingIcon } from '@gitlab/ui'; import { __, sprintf } from '~/locale'; import createFlash from '~/flash'; +import { hasDiff } from '~/helpers/diffs_helper'; import eventHub from '../../notes/event_hub'; import DiffFileHeader from './diff_file_header.vue'; import DiffContent from './diff_content.vue'; @@ -55,12 +56,7 @@ export default { return this.isLoadingCollapsedDiff || (!this.file.renderIt && !this.isCollapsed); }, hasDiff() { - return ( - (this.file.highlighted_diff_lines && - this.file.parallel_diff_lines && - this.file.parallel_diff_lines.length > 0) || - !this.file.blob.readable_text - ); + return hasDiff(this.file); }, isFileTooLarge() { return this.file.viewer.error === diffViewerErrors.too_large; diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 6f2467ddf71..6714f4e62b8 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -65,6 +65,10 @@ export const fetchDiffFiles = ({ state, commit }) => { w: state.showWhitespace ? '0' : '1', }; + if (state.useSingleDiffStyle) { + urlParams.view = state.diffViewType; + } + commit(types.SET_LOADING, true); worker.addEventListener('message', ({ data }) => { @@ -90,13 +94,22 @@ export const fetchDiffFiles = ({ state, commit }) => { }; export const fetchDiffFilesBatch = ({ commit, state }) => { + const urlParams = { + per_page: DIFFS_PER_PAGE, + w: state.showWhitespace ? '0' : '1', + }; + + if (state.useSingleDiffStyle) { + urlParams.view = state.diffViewType; + } + commit(types.SET_BATCH_LOADING, true); commit(types.SET_RETRIEVING_BATCHES, true); const getBatch = page => axios .get(state.endpointBatch, { - params: { page, per_page: DIFFS_PER_PAGE, w: state.showWhitespace ? '0' : '1' }, + params: { ...urlParams, page }, }) .then(({ data: { pagination, diff_files } }) => { commit(types.SET_DIFF_DATA_BATCH, { diff_files }); @@ -150,7 +163,10 @@ export const assignDiscussionsToDiff = ( { commit, state, rootState }, discussions = rootState.notes.discussions, ) => { - const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles); + const diffPositionByLineCode = getDiffPositionByLineCode( + state.diffFiles, + state.useSingleDiffStyle, + ); const hash = getLocationHash(); discussions @@ -339,24 +355,23 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => { export const toggleFileDiscussionWrappers = ({ commit }, diff) => { const discussionWrappersExpanded = allDiscussionWrappersExpanded(diff); - let linesWithDiscussions; - if (diff.highlighted_diff_lines) { - linesWithDiscussions = diff.highlighted_diff_lines.filter(line => line.discussions.length); - } - if (diff.parallel_diff_lines) { - linesWithDiscussions = diff.parallel_diff_lines.filter( - line => - (line.left && line.left.discussions.length) || - (line.right && line.right.discussions.length), - ); - } - - if (linesWithDiscussions.length) { - linesWithDiscussions.forEach(line => { + const lineCodesWithDiscussions = new Set(); + const { parallel_diff_lines: parallelLines, highlighted_diff_lines: inlineLines } = diff; + const allLines = inlineLines.concat( + parallelLines.map(line => line.left), + parallelLines.map(line => line.right), + ); + const lineHasDiscussion = line => Boolean(line?.discussions.length); + const registerDiscussionLine = line => lineCodesWithDiscussions.add(line.line_code); + + allLines.filter(lineHasDiscussion).forEach(registerDiscussionLine); + + if (lineCodesWithDiscussions.size) { + Array.from(lineCodesWithDiscussions).forEach(lineCode => { commit(types.TOGGLE_LINE_DISCUSSIONS, { fileHash: diff.file_hash, - lineCode: line.line_code, expanded: !discussionWrappersExpanded, + lineCode, }); }); } diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index a4986e26966..8cfdded1f9b 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -45,26 +45,28 @@ export default { }, [types.SET_DIFF_DATA](state, data) { + let files = state.diffFiles; + if ( - !( - gon && - gon.features && - gon.features.diffsBatchLoad && - window.location.search.indexOf('diff_id') === -1 - ) + !(gon?.features?.diffsBatchLoad && window.location.search.indexOf('diff_id') === -1) && + data.diff_files ) { - prepareDiffData(data); + files = prepareDiffData(data, files); } Object.assign(state, { ...convertObjectPropsToCamelCase(data), + diffFiles: files, }); }, [types.SET_DIFF_DATA_BATCH](state, data) { - prepareDiffData(data); + const files = prepareDiffData(data, state.diffFiles); - state.diffFiles.push(...data.diff_files); + Object.assign(state, { + ...convertObjectPropsToCamelCase(data), + diffFiles: files, + }); }, [types.RENDER_FILE](state, file) { @@ -88,11 +90,11 @@ export default { if (!diffFile) return; - if (diffFile.highlighted_diff_lines) { + if (diffFile.highlighted_diff_lines.length) { diffFile.highlighted_diff_lines.find(l => l.line_code === lineCode).hasForm = hasForm; } - if (diffFile.parallel_diff_lines) { + if (diffFile.parallel_diff_lines.length) { const line = diffFile.parallel_diff_lines.find(l => { const { left, right } = l; @@ -153,13 +155,13 @@ export default { }, [types.EXPAND_ALL_FILES](state) { - state.diffFiles = state.diffFiles.map(file => ({ - ...file, - viewer: { - ...file.viewer, - collapsed: false, - }, - })); + state.diffFiles.forEach(file => { + Object.assign(file, { + viewer: Object.assign(file.viewer, { + collapsed: false, + }), + }); + }); }, [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) { @@ -197,29 +199,29 @@ export default { }; }; - state.diffFiles = state.diffFiles.map(diffFile => { - if (diffFile.file_hash === fileHash) { - const file = { ...diffFile }; - - if (file.highlighted_diff_lines) { - file.highlighted_diff_lines = file.highlighted_diff_lines.map(line => - setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line), - ); + state.diffFiles.forEach(file => { + if (file.file_hash === fileHash) { + if (file.highlighted_diff_lines.length) { + file.highlighted_diff_lines.forEach(line => { + Object.assign( + line, + setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line), + ); + }); } - if (file.parallel_diff_lines) { - file.parallel_diff_lines = file.parallel_diff_lines.map(line => { + if (file.parallel_diff_lines.length) { + file.parallel_diff_lines.forEach(line => { const left = line.left && lineCheck(line.left); const right = line.right && lineCheck(line.right); if (left || right) { - return { - ...line, + Object.assign(line, { left: line.left ? setDiscussionsExpanded(mapDiscussions(line.left)) : null, right: line.right ? setDiscussionsExpanded(mapDiscussions(line.right, () => !left)) : null, - }; + }); } return line; @@ -227,15 +229,15 @@ export default { } if (!file.parallel_diff_lines || !file.highlighted_diff_lines) { - file.discussions = (file.discussions || []) + const newDiscussions = (file.discussions || []) .filter(d => d.id !== discussion.id) .concat(discussion); - } - return file; + Object.assign(file, { + discussions: newDiscussions, + }); + } } - - return diffFile; }); }, @@ -259,9 +261,9 @@ export default { [types.TOGGLE_LINE_DISCUSSIONS](state, { fileHash, lineCode, expanded }) { const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash); - updateLineInFile(selectedFile, lineCode, line => - Object.assign(line, { discussionsExpanded: expanded }), - ); + updateLineInFile(selectedFile, lineCode, line => { + Object.assign(line, { discussionsExpanded: expanded }); + }); }, [types.TOGGLE_FOLDER_OPEN](state, path) { diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 281a0de1fc2..b379f1fabef 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -185,6 +185,7 @@ export function addContextLines(options) { * Trims the first char of the `richText` property when it's either a space or a diff symbol. * @param {Object} line * @returns {Object} + * @deprecated */ export function trimFirstCharOfLineContent(line = {}) { // eslint-disable-next-line no-param-reassign @@ -212,79 +213,171 @@ function getLineCode({ left, right }, index) { return index; } -// This prepares and optimizes the incoming diff data from the server -// by setting up incremental rendering and removing unneeded data -export function prepareDiffData(diffData) { - const filesLength = diffData.diff_files.length; - let showingLines = 0; - for (let i = 0; i < filesLength; i += 1) { - const file = diffData.diff_files[i]; - - if (file.parallel_diff_lines) { - const linesLength = file.parallel_diff_lines.length; - for (let u = 0; u < linesLength; u += 1) { - const line = file.parallel_diff_lines[u]; - - line.line_code = getLineCode(line, u); - if (line.left) { - line.left = trimFirstCharOfLineContent(line.left); - line.left.discussions = []; - line.left.hasForm = false; - } - if (line.right) { - line.right = trimFirstCharOfLineContent(line.right); - line.right.discussions = []; - line.right.hasForm = false; - } +function diffFileUniqueId(file) { + return `${file.content_sha}-${file.file_hash}`; +} + +function combineDiffFilesWithPriorFiles(files, prior = []) { + files.forEach(file => { + const id = diffFileUniqueId(file); + const oldMatch = prior.find(oldFile => diffFileUniqueId(oldFile) === id); + + if (oldMatch) { + const missingInline = !file.highlighted_diff_lines; + const missingParallel = !file.parallel_diff_lines; + + if (missingInline) { + Object.assign(file, { + highlighted_diff_lines: oldMatch.highlighted_diff_lines, + }); } - } - if (file.highlighted_diff_lines) { - const linesLength = file.highlighted_diff_lines.length; - for (let u = 0; u < linesLength; u += 1) { - const line = file.highlighted_diff_lines[u]; - Object.assign(line, { - ...trimFirstCharOfLineContent(line), - discussions: [], - hasForm: false, + if (missingParallel) { + Object.assign(file, { + parallel_diff_lines: oldMatch.parallel_diff_lines, }); } - showingLines += file.parallel_diff_lines.length; + } + }); + + return files; +} + +function ensureBasicDiffFileLines(file) { + const missingInline = !file.highlighted_diff_lines; + const missingParallel = !file.parallel_diff_lines; + + Object.assign(file, { + highlighted_diff_lines: missingInline ? [] : file.highlighted_diff_lines, + parallel_diff_lines: missingParallel ? [] : file.parallel_diff_lines, + }); + + return file; +} + +function cleanRichText(text) { + return text ? text.replace(/^[+ -]/, '') : undefined; +} + +function prepareLine(line) { + return Object.assign(line, { + rich_text: cleanRichText(line.rich_text), + discussionsExpanded: true, + discussions: [], + hasForm: false, + text: undefined, + }); +} + +function prepareDiffFileLines(file) { + const inlineLines = file.highlighted_diff_lines; + const parallelLines = file.parallel_diff_lines; + let parallelLinesCount = 0; + + inlineLines.forEach(prepareLine); + + parallelLines.forEach((line, index) => { + Object.assign(line, { line_code: getLineCode(line, index) }); + + if (line.left) { + parallelLinesCount += 1; + prepareLine(line.left); } - const name = (file.viewer && file.viewer.name) || diffViewerModes.text; + if (line.right) { + parallelLinesCount += 1; + prepareLine(line.right); + } Object.assign(file, { - renderIt: showingLines < LINES_TO_BE_RENDERED_DIRECTLY, - collapsed: name === diffViewerModes.text && showingLines > MAX_LINES_TO_BE_RENDERED, - isShowingFullFile: false, - isLoadingFullFile: false, - discussions: [], - renderingLines: false, + inlineLinesCount: inlineLines.length, + parallelLinesCount, }); - } + }); + + return file; } -export function getDiffPositionByLineCode(diffFiles) { - return diffFiles.reduce((acc, 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.highlighted_diff_lines) { +function getVisibleDiffLines(file) { + return Math.max(file.inlineLinesCount, file.parallelLinesCount); +} + +function finalizeDiffFile(file) { + const name = (file.viewer && file.viewer.name) || diffViewerModes.text; + const lines = getVisibleDiffLines(file); + + Object.assign(file, { + renderIt: lines < LINES_TO_BE_RENDERED_DIRECTLY, + collapsed: name === diffViewerModes.text && lines > MAX_LINES_TO_BE_RENDERED, + isShowingFullFile: false, + isLoadingFullFile: false, + discussions: [], + renderingLines: false, + }); + + return file; +} + +export function prepareDiffData(diffData, priorFiles) { + return combineDiffFilesWithPriorFiles(diffData.diff_files, priorFiles) + .map(ensureBasicDiffFileLines) + .map(prepareDiffFileLines) + .map(finalizeDiffFile); +} + +export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) { + let lines = []; + const hasInlineDiffs = diffFiles.some(file => file.highlighted_diff_lines.length > 0); + + if (!useSingleDiffStyle || hasInlineDiffs) { + // In either of these cases, we can use `highlighted_diff_lines` because + // that will include all of the parallel diff lines, too + + lines = diffFiles.reduce((acc, diffFile) => { diffFile.highlighted_diff_lines.forEach(line => { - if (line.line_code) { - acc[line.line_code] = { - base_sha: diffFile.diff_refs.base_sha, - head_sha: diffFile.diff_refs.head_sha, - start_sha: diffFile.diff_refs.start_sha, - new_path: diffFile.new_path, - old_path: diffFile.old_path, - old_line: line.old_line, - new_line: line.new_line, - line_code: line.line_code, - position_type: 'text', - }; + acc.push({ file: diffFile, line }); + }); + + return acc; + }, []); + } else { + // If we're in single diff view mode and the inline lines haven't been + // loaded yet, we need to parse the parallel lines + + lines = diffFiles.reduce((acc, diffFile) => { + diffFile.parallel_diff_lines.forEach(pair => { + // It's possible for a parallel line to have an opposite line that doesn't exist + // For example: *deleted* lines will have `null` right lines, while + // *added* lines will have `null` left lines. + // So we have to check each line before we push it onto the array so we're not + // pushing null line diffs + + if (pair.left) { + acc.push({ file: diffFile, line: pair.left }); + } + + if (pair.right) { + acc.push({ file: diffFile, line: pair.right }); } }); + + return acc; + }, []); + } + + return lines.reduce((acc, { file, line }) => { + if (line.line_code) { + acc[line.line_code] = { + base_sha: file.diff_refs.base_sha, + head_sha: file.diff_refs.head_sha, + start_sha: file.diff_refs.start_sha, + new_path: file.new_path, + old_path: file.old_path, + old_line: line.old_line, + new_line: line.new_line, + line_code: line.line_code, + position_type: 'text', + }; } return acc; @@ -462,47 +555,47 @@ export const convertExpandLines = ({ export const idleCallback = cb => requestIdleCallback(cb); -export const updateLineInFile = (selectedFile, lineCode, updateFn) => { - if (selectedFile.parallel_diff_lines) { - const targetLine = selectedFile.parallel_diff_lines.find( - line => - (line.left && line.left.line_code === lineCode) || - (line.right && line.right.line_code === lineCode), - ); - if (targetLine) { - const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right'; +function getLinesFromFileByLineCode(file, lineCode) { + const parallelLines = file.parallel_diff_lines; + const inlineLines = file.highlighted_diff_lines; + const matchesCode = line => line.line_code === lineCode; - updateFn(targetLine[side]); - } - } - if (selectedFile.highlighted_diff_lines) { - const targetInlineLine = selectedFile.highlighted_diff_lines.find( - line => line.line_code === lineCode, - ); + return [ + ...parallelLines.reduce((acc, line) => { + if (line.left) { + acc.push(line.left); + } - if (targetInlineLine) { - updateFn(targetInlineLine); - } - } + if (line.right) { + acc.push(line.right); + } + + return acc; + }, []), + ...inlineLines, + ].filter(matchesCode); +} + +export const updateLineInFile = (selectedFile, lineCode, updateFn) => { + getLinesFromFileByLineCode(selectedFile, lineCode).forEach(updateFn); }; export const allDiscussionWrappersExpanded = diff => { - const discussionsExpandedArray = []; - if (diff.parallel_diff_lines) { - diff.parallel_diff_lines.forEach(line => { - if (line.left && line.left.discussions.length) { - discussionsExpandedArray.push(line.left.discussionsExpanded); - } - if (line.right && line.right.discussions.length) { - discussionsExpandedArray.push(line.right.discussionsExpanded); - } - }); - } else if (diff.highlighted_diff_lines) { - diff.highlighted_diff_lines.forEach(line => { - if (line.discussions.length) { - discussionsExpandedArray.push(line.discussionsExpanded); - } - }); - } - return discussionsExpandedArray.every(el => el); + let discussionsExpanded = true; + const changeExpandedResult = line => { + if (line && line.discussions.length) { + discussionsExpanded = discussionsExpanded && line.discussionsExpanded; + } + }; + + diff.parallel_diff_lines.forEach(line => { + changeExpandedResult(line.left); + changeExpandedResult(line.right); + }); + + diff.highlighted_diff_lines.forEach(line => { + changeExpandedResult(line); + }); + + return discussionsExpanded; }; diff --git a/app/assets/javascripts/error_tracking/components/error_details.vue b/app/assets/javascripts/error_tracking/components/error_details.vue index d4562d4a9a5..23a1ec4e367 100644 --- a/app/assets/javascripts/error_tracking/components/error_details.vue +++ b/app/assets/javascripts/error_tracking/components/error_details.vue @@ -1,6 +1,7 @@ <script> import { mapActions, mapGetters, mapState } from 'vuex'; import dateFormat from 'dateformat'; +import createFlash from '~/flash'; import { GlFormInput, GlLink, GlLoadingIcon, GlBadge } from '@gitlab/ui'; import { __, sprintf, n__ } from '~/locale'; import LoadingButton from '~/vue_shared/components/loading_button.vue'; @@ -11,6 +12,8 @@ import TrackEventDirective from '~/vue_shared/directives/track_event'; import timeagoMixin from '~/vue_shared/mixins/timeago'; import { trackClickErrorLinkToSentryOptions } from '../utils'; +import query from '../queries/details.query.graphql'; + export default { components: { LoadingButton, @@ -27,6 +30,14 @@ export default { }, mixins: [timeagoMixin], props: { + issueId: { + type: String, + required: true, + }, + projectPath: { + type: String, + required: true, + }, issueDetailsPath: { type: String, required: true, @@ -44,8 +55,28 @@ export default { required: true, }, }, + apollo: { + GQLerror: { + query, + variables() { + return { + fullPath: this.projectPath, + errorId: `gid://gitlab/Gitlab::ErrorTracking::DetailedError/${this.issueId}`, + }; + }, + pollInterval: 2000, + update: data => data.project.sentryDetailedError, + error: () => createFlash(__('Failed to load error details from Sentry.')), + result(res) { + if (res.data.project?.sentryDetailedError) { + this.$apollo.queries.GQLerror.stopPolling(); + } + }, + }, + }, data() { return { + GQLerror: null, issueCreationInProgress: false, }; }, @@ -56,26 +87,28 @@ export default { return sprintf( __('Reported %{timeAgo} by %{reportedBy}'), { - reportedBy: `<strong>${this.error.culprit}</strong>`, + reportedBy: `<strong>${this.GQLerror.culprit}</strong>`, timeAgo: this.timeFormatted(this.stacktraceData.date_received), }, false, ); }, firstReleaseLink() { - return `${this.error.external_base_url}/releases/${this.error.first_release_short_version}`; + return `${this.error.external_base_url}/releases/${this.GQLerror.firstReleaseShortVersion}`; }, lastReleaseLink() { - return `${this.error.external_base_url}releases/${this.error.last_release_short_version}`; + return `${this.error.external_base_url}releases/${this.GQLerror.lastReleaseShortVersion}`; }, showDetails() { - return Boolean(!this.loading && this.error && this.error.id); + return Boolean( + !this.loading && !this.$apollo.queries.GQLerror.loading && this.error && this.GQLerror, + ); }, showStacktrace() { return Boolean(!this.loadingStacktrace && this.stacktrace && this.stacktrace.length); }, issueTitle() { - return this.error.title; + return this.GQLerror.title; }, issueDescription() { return sprintf( @@ -84,13 +117,13 @@ export default { ), { description: '# Error Details:\n', - errorUrl: `${this.error.external_url}\n`, - firstSeen: `\n${this.error.first_seen}\n`, - lastSeen: `${this.error.last_seen}\n`, - countLabel: n__('- Event', '- Events', this.error.count), - count: `${this.error.count}\n`, - userCountLabel: n__('- User', '- Users', this.error.user_count), - userCount: `${this.error.user_count}\n`, + errorUrl: `${this.GQLerror.externalUrl}\n`, + firstSeen: `\n${this.GQLerror.firstSeen}\n`, + lastSeen: `${this.GQLerror.lastSeen}\n`, + countLabel: n__('- Event', '- Events', this.GQLerror.count), + count: `${this.GQLerror.count}\n`, + userCountLabel: n__('- User', '- Users', this.GQLerror.userCount), + userCount: `${this.GQLerror.userCount}\n`, }, false, ); @@ -119,7 +152,7 @@ export default { <template> <div> - <div v-if="loading" class="py-3"> + <div v-if="$apollo.queries.GQLerror.loading || loading" class="py-3"> <gl-loading-icon :size="3" /> </div> <div v-else-if="showDetails" class="error-details"> @@ -129,7 +162,7 @@ export default { <gl-form-input class="hidden" name="issue[title]" :value="issueTitle" /> <input name="issue[description]" :value="issueDescription" type="hidden" /> <gl-form-input - :value="error.id" + :value="GQLerror.id" class="hidden" name="issue[sentry_issue_attributes][sentry_issue_identifier]" /> @@ -145,16 +178,16 @@ export default { </form> </div> <div> - <tooltip-on-truncate :title="error.title" truncate-target="child" placement="top"> - <h2 class="text-truncate">{{ error.title }}</h2> + <tooltip-on-truncate :title="GQLerror.title" truncate-target="child" placement="top"> + <h2 class="text-truncate">{{ GQLerror.title }}</h2> </tooltip-on-truncate> <template v-if="error.tags"> - <gl-badge v-if="error.tags.level" variant="danger" class="rounded-pill mr-2">{{ - errorLevel - }}</gl-badge> - <gl-badge v-if="error.tags.logger" variant="light" class="rounded-pill">{{ - error.tags.logger - }}</gl-badge> + <gl-badge v-if="error.tags.level" variant="danger" class="rounded-pill mr-2" + >{{ errorLevel }} + </gl-badge> + <gl-badge v-if="error.tags.logger" variant="light" class="rounded-pill" + >{{ error.tags.logger }} + </gl-badge> </template> <h3>{{ __('Error details') }}</h3> @@ -168,35 +201,35 @@ export default { <li> <span class="bold">{{ __('Sentry event') }}:</span> <gl-link - v-track-event="trackClickErrorLinkToSentryOptions(error.external_url)" - :href="error.external_url" + v-track-event="trackClickErrorLinkToSentryOptions(GQLerror.externalUrl)" + :href="GQLerror.externalUrl" target="_blank" > - <span class="text-truncate">{{ error.external_url }}</span> + <span class="text-truncate">{{ GQLerror.externalUrl }}</span> <icon name="external-link" class="ml-1 flex-shrink-0" /> </gl-link> </li> - <li v-if="error.first_release_short_version"> + <li v-if="GQLerror.firstReleaseShortVersion"> <span class="bold">{{ __('First seen') }}:</span> - {{ formatDate(error.first_seen) }} + {{ formatDate(GQLerror.firstSeen) }} <gl-link :href="firstReleaseLink" target="_blank"> - <span>{{ __('Release') }}: {{ error.first_release_short_version }}</span> + <span>{{ __('Release') }}: {{ GQLerror.firstReleaseShortVersion }}</span> </gl-link> </li> - <li v-if="error.last_release_short_version"> + <li v-if="GQLerror.lastReleaseShortVersion"> <span class="bold">{{ __('Last seen') }}:</span> - {{ formatDate(error.last_seen) }} + {{ formatDate(GQLerror.lastSeen) }} <gl-link :href="lastReleaseLink" target="_blank"> - <span>{{ __('Release') }}: {{ error.last_release_short_version }}</span> + <span>{{ __('Release') }}: {{ GQLerror.lastReleaseShortVersion }}</span> </gl-link> </li> <li> <span class="bold">{{ __('Events') }}:</span> - <span>{{ error.count }}</span> + <span>{{ GQLerror.count }}</span> </li> <li> <span class="bold">{{ __('Users') }}:</span> - <span>{{ error.user_count }}</span> + <span>{{ GQLerror.userCount }}</span> </li> </ul> diff --git a/app/assets/javascripts/error_tracking/details.js b/app/assets/javascripts/error_tracking/details.js index 872cb8868a2..b9761cdf2c1 100644 --- a/app/assets/javascripts/error_tracking/details.js +++ b/app/assets/javascripts/error_tracking/details.js @@ -1,22 +1,39 @@ import Vue from 'vue'; +import VueApollo from 'vue-apollo'; +import createDefaultClient from '~/lib/graphql'; import store from './store'; import ErrorDetails from './components/error_details.vue'; import csrf from '~/lib/utils/csrf'; +Vue.use(VueApollo); + export default () => { + const apolloProvider = new VueApollo({ + defaultClient: createDefaultClient(), + }); + // eslint-disable-next-line no-new new Vue({ el: '#js-error_details', + apolloProvider, components: { ErrorDetails, }, store, render(createElement) { const domEl = document.querySelector(this.$options.el); - const { issueDetailsPath, issueStackTracePath, projectIssuesPath } = domEl.dataset; + const { + issueId, + projectPath, + issueDetailsPath, + issueStackTracePath, + projectIssuesPath, + } = domEl.dataset; return createElement('error-details', { props: { + issueId, + projectPath, issueDetailsPath, issueStackTracePath, projectIssuesPath, diff --git a/app/assets/javascripts/error_tracking/queries/details.query.graphql b/app/assets/javascripts/error_tracking/queries/details.query.graphql new file mode 100644 index 00000000000..f65bdb9b968 --- /dev/null +++ b/app/assets/javascripts/error_tracking/queries/details.query.graphql @@ -0,0 +1,18 @@ +query errorDetails($fullPath: ID!, $errorId: ID!) { + project(fullPath: $fullPath) { + sentryDetailedError(id: $errorId) { + id + sentryId + title + userCount + count + firstSeen + lastSeen + message + culprit + externalUrl + firstReleaseShortVersion + lastReleaseShortVersion + } + } +} diff --git a/app/assets/javascripts/helpers/diffs_helper.js b/app/assets/javascripts/helpers/diffs_helper.js new file mode 100644 index 00000000000..9695d01ad3d --- /dev/null +++ b/app/assets/javascripts/helpers/diffs_helper.js @@ -0,0 +1,19 @@ +export function hasInlineLines(diffFile) { + return diffFile?.highlighted_diff_lines?.length > 0; /* eslint-disable-line camelcase */ +} + +export function hasParallelLines(diffFile) { + return diffFile?.parallel_diff_lines?.length > 0; /* eslint-disable-line camelcase */ +} + +export function isSingleViewStyle(diffFile) { + return !hasParallelLines(diffFile) || !hasInlineLines(diffFile); +} + +export function hasDiff(diffFile) { + return ( + hasInlineLines(diffFile) || + hasParallelLines(diffFile) || + !diffFile?.blob?.readable_text /* eslint-disable-line camelcase */ + ); +} diff --git a/app/assets/javascripts/monitoring/stores/actions.js b/app/assets/javascripts/monitoring/stores/actions.js index 1cb82ce0083..fce89b450e4 100644 --- a/app/assets/javascripts/monitoring/stores/actions.js +++ b/app/assets/javascripts/monitoring/stores/actions.js @@ -39,7 +39,7 @@ export const requestMetricsDashboard = ({ commit }) => { }; export const receiveMetricsDashboardSuccess = ({ commit, dispatch }, { response, params }) => { commit(types.SET_ALL_DASHBOARDS, response.all_dashboards); - commit(types.RECEIVE_METRICS_DATA_SUCCESS, response.dashboard.panel_groups); + commit(types.RECEIVE_METRICS_DATA_SUCCESS, response.dashboard); return dispatch('fetchPrometheusMetrics', params); }; export const receiveMetricsDashboardFailure = ({ commit }, error) => { diff --git a/app/assets/javascripts/monitoring/stores/mutations.js b/app/assets/javascripts/monitoring/stores/mutations.js index 16a34a6c026..0b848de9562 100644 --- a/app/assets/javascripts/monitoring/stores/mutations.js +++ b/app/assets/javascripts/monitoring/stores/mutations.js @@ -84,23 +84,26 @@ export default { state.emptyState = 'loading'; state.showEmptyState = true; }, - [types.RECEIVE_METRICS_DATA_SUCCESS](state, groupData) { - state.dashboard.panel_groups = groupData.map((group, i) => { - const key = `${slugify(group.group || 'default')}-${i}`; - let { panels = [] } = group; - - // each panel has metric information that needs to be normalized - panels = panels.map(panel => ({ - ...panel, - metrics: normalizePanelMetrics(panel.metrics, panel.y_label), - })); - - return { - ...group, - panels, - key, - }; - }); + [types.RECEIVE_METRICS_DATA_SUCCESS](state, dashboard) { + state.dashboard = { + ...dashboard, + panel_groups: dashboard.panel_groups.map((group, i) => { + const key = `${slugify(group.group || 'default')}-${i}`; + let { panels = [] } = group; + + // each panel has metric information that needs to be normalized + panels = panels.map(panel => ({ + ...panel, + metrics: normalizePanelMetrics(panel.metrics, panel.y_label), + })); + + return { + ...group, + panels, + key, + }; + }), + }; if (!state.dashboard.panel_groups.length) { state.emptyState = 'noData'; diff --git a/app/helpers/projects/error_tracking_helper.rb b/app/helpers/projects/error_tracking_helper.rb index 2ec7fa91c86..f2d16c30fb4 100644 --- a/app/helpers/projects/error_tracking_helper.rb +++ b/app/helpers/projects/error_tracking_helper.rb @@ -18,9 +18,11 @@ module Projects::ErrorTrackingHelper opts = [project, issue_id, { format: :json }] { - 'project-issues-path' => project_issues_path(project), + 'issue-id' => issue_id, + 'project-path' => project.full_path, 'issue-details-path' => details_project_error_tracking_index_path(*opts), 'issue-update-path' => update_project_error_tracking_index_path(*opts), + 'project-issues-path' => project_issues_path(project), 'issue-stack-trace-path' => stack_trace_project_error_tracking_index_path(*opts) } end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 68548bd2fdc..85cb3f5b46a 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -11,7 +11,7 @@ module Ci has_many :trigger_requests validates :token, presence: true, uniqueness: true - validates :owner, presence: true, unless: :supports_legacy_tokens? + validates :owner, presence: true before_validation :set_default_values @@ -31,17 +31,8 @@ module Ci token[0...4] if token.present? end - def legacy? - self.owner_id.blank? - end - - def supports_legacy_tokens? - Feature.enabled?(:use_legacy_pipeline_triggers, project) - end - def can_access_project? - supports_legacy_tokens? && legacy? || - Ability.allowed?(self.owner, :create_build, project) + Ability.allowed?(self.owner, :create_build, project) end end end diff --git a/app/policies/ci/trigger_policy.rb b/app/policies/ci/trigger_policy.rb index 578301d7f7e..e26f96a4b2b 100644 --- a/app/policies/ci/trigger_policy.rb +++ b/app/policies/ci/trigger_policy.rb @@ -5,13 +5,12 @@ module Ci delegate { @subject.project } with_options scope: :subject, score: 0 - condition(:legacy) { @subject.supports_legacy_tokens? && @subject.legacy? } with_score 0 condition(:is_owner) { @user && @subject.owner_id == @user.id } rule { ~can?(:admin_build) }.prevent :admin_trigger - rule { legacy | is_owner }.enable :admin_trigger + rule { is_owner }.enable :admin_trigger rule { can?(:admin_build) }.enable :manage_trigger end diff --git a/app/views/errors/_footer.html.haml b/app/views/errors/_footer.html.haml index e67a3a142f6..bb9edc54b4b 100644 --- a/app/views/errors/_footer.html.haml +++ b/app/views/errors/_footer.html.haml @@ -4,7 +4,7 @@ = link_to s_('Nav|Home'), root_path %li - if current_user - = link_to s_('Nav|Sign out and sign in with a different account'), destroy_user_session_path + = link_to s_('Nav|Sign out and sign in with a different account'), destroy_user_session_path, method: :post - else = link_to s_('Nav|Sign In / Register'), new_session_path(:user, redirect_to_referer: 'yes') %li diff --git a/app/views/layouts/header/_current_user_dropdown.html.haml b/app/views/layouts/header/_current_user_dropdown.html.haml index 88803f982e8..84906c305a7 100644 --- a/app/views/layouts/header/_current_user_dropdown.html.haml +++ b/app/views/layouts/header/_current_user_dropdown.html.haml @@ -47,4 +47,4 @@ - if current_user_menu?(:sign_out) %li.divider %li - = link_to _("Sign out"), destroy_user_session_path, class: "sign-out-link", data: { qa_selector: 'sign_out_link' } + = link_to _("Sign out"), destroy_user_session_path, method: :post, class: "sign-out-link", data: { qa_selector: 'sign_out_link' } diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 9ae06ae77f1..379ba976040 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -55,7 +55,7 @@ - if Feature.enabled?(:user_mode_in_session) - if header_link?(:admin_mode) = nav_link(controller: 'admin/sessions') do - = link_to destroy_admin_session_path, class: 'd-lg-none lock-open-icon' do + = link_to destroy_admin_session_path, method: :post, class: 'd-lg-none lock-open-icon' do = _('Leave Admin Mode') - elsif current_user.admin? = nav_link(controller: 'admin/sessions') do diff --git a/app/views/projects/triggers/_content.html.haml b/app/views/projects/triggers/_content.html.haml deleted file mode 100644 index e686068657c..00000000000 --- a/app/views/projects/triggers/_content.html.haml +++ /dev/null @@ -1,9 +0,0 @@ -- if Feature.enabled?(:use_legacy_pipeline_triggers, @project) - %p.append-bottom-default - Triggers with the - %span.badge.badge-primary legacy - label do not have an associated user and only have access to the current project. - %br - = succeed '.' do - Learn more in the - = link_to 'triggers documentation', help_page_path('ci/triggers/README'), target: '_blank' diff --git a/app/views/projects/triggers/_index.html.haml b/app/views/projects/triggers/_index.html.haml index a559ce41e57..55a9234f01a 100644 --- a/app/views/projects/triggers/_index.html.haml +++ b/app/views/projects/triggers/_index.html.haml @@ -1,6 +1,5 @@ .row.prepend-top-default.append-bottom-default.triggers-container .col-lg-12 - = render "projects/triggers/content" .card .card-header Manage your project's triggers diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 60de3630bb5..d80248f7e80 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -7,12 +7,7 @@ %span= trigger.short_token .label-container - - if trigger.legacy? - - if trigger.supports_legacy_tokens? - %span.badge.badge-primary.has-tooltip{ title: "Trigger makes use of deprecated functionality" } legacy - - else - %span.badge.badge-danger.has-tooltip{ title: "Trigger is invalid due to being a legacy trigger. We recommend replacing it with a new trigger" } invalid - - elsif !trigger.can_access_project? + - unless trigger.can_access_project? %span.badge.badge-danger.has-tooltip{ title: "Trigger user has insufficient permissions to project" } invalid %td diff --git a/app/views/projects/triggers/edit.html.haml b/app/views/projects/triggers/edit.html.haml index c35df322b9d..0f74d733c06 100644 --- a/app/views/projects/triggers/edit.html.haml +++ b/app/views/projects/triggers/edit.html.haml @@ -1,9 +1,7 @@ - page_title "Trigger" .row.prepend-top-default.append-bottom-default - .col-lg-3 - = render "content" - .col-lg-9 + .col-lg-12 %h4.prepend-top-0 Update trigger = render "form", btn_text: "Save trigger" diff --git a/changelogs/unreleased/14857-activate-promethus-integration-for-projects.yml b/changelogs/unreleased/14857-activate-promethus-integration-for-projects.yml deleted file mode 100644 index a83008ee848..00000000000 --- a/changelogs/unreleased/14857-activate-promethus-integration-for-projects.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Migrate the database to activate projects prometheus service integration for projects with prometheus installed on shared k8s cluster. -merge_request: 19956 -author: -type: fixed diff --git a/changelogs/unreleased/29772-completely-removing-use_legacy_pipeline_triggers.yml b/changelogs/unreleased/29772-completely-removing-use_legacy_pipeline_triggers.yml new file mode 100644 index 00000000000..d9ac063f310 --- /dev/null +++ b/changelogs/unreleased/29772-completely-removing-use_legacy_pipeline_triggers.yml @@ -0,0 +1,5 @@ +--- +title: Remove feature flag 'use_legacy_pipeline_triggers' and remove legacy tokens +merge_request: 21732 +author: +type: removed diff --git a/changelogs/unreleased/34867-epics_to_project_import_export.yml b/changelogs/unreleased/34867-epics_to_project_import_export.yml new file mode 100644 index 00000000000..a7c2b2e3655 --- /dev/null +++ b/changelogs/unreleased/34867-epics_to_project_import_export.yml @@ -0,0 +1,5 @@ +--- +title: Add epics to project import/export +merge_request: 19883 +author: +type: added diff --git a/changelogs/unreleased/39979-grapql-for-error-details.yml b/changelogs/unreleased/39979-grapql-for-error-details.yml new file mode 100644 index 00000000000..6ae3f203f56 --- /dev/null +++ b/changelogs/unreleased/39979-grapql-for-error-details.yml @@ -0,0 +1,5 @@ +--- +title: Use GraphQL to load error tracking detail page content +merge_request: 22422 +author: +type: performance diff --git a/changelogs/unreleased/enable-parent-child-pipelines.yml b/changelogs/unreleased/enable-parent-child-pipelines.yml new file mode 100644 index 00000000000..aa38b06839a --- /dev/null +++ b/changelogs/unreleased/enable-parent-child-pipelines.yml @@ -0,0 +1,5 @@ +--- +title: Allow a pipeline (parent) to create a child pipeline as downstream pipeline within the same project +merge_request: 21830 +author: +type: added diff --git a/changelogs/unreleased/feature-split-diff-attempt-3.yml b/changelogs/unreleased/feature-split-diff-attempt-3.yml new file mode 100644 index 00000000000..8adda8be3af --- /dev/null +++ b/changelogs/unreleased/feature-split-diff-attempt-3.yml @@ -0,0 +1,5 @@ +--- +title: Load MR diff types lazily to reduce initial diff payload size +merge_request: 19930 +author: +type: added diff --git a/changelogs/unreleased/refactor-session-disable-with-post.yml b/changelogs/unreleased/refactor-session-disable-with-post.yml new file mode 100644 index 00000000000..63908947095 --- /dev/null +++ b/changelogs/unreleased/refactor-session-disable-with-post.yml @@ -0,0 +1,5 @@ +--- +title: User signout and admin mode disable use now POST instead of GET +merge_request: 22113 +author: Diego Louzán +type: other diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index 8d4c5fa382c..e1c37caaafd 100644 --- a/config/initializers/8_devise.rb +++ b/config/initializers/8_devise.rb @@ -203,7 +203,7 @@ Devise.setup do |config| config.navigational_formats = [:"*/*", "*/*", :html, :zip] # The default HTTP method used to sign out a resource. Default is :delete. - config.sign_out_via = :get + config.sign_out_via = :post # ==> OmniAuth # To configure a new OmniAuth provider copy and edit omniauth.rb.sample diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 45cca1d3191..f363823f80c 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -24,7 +24,7 @@ namespace :admin do end resource :session, only: [:new, :create] do - get 'destroy', action: :destroy, as: :destroy + post 'destroy', action: :destroy, as: :destroy end resource :impersonation, only: :destroy diff --git a/db/post_migrate/20191204114127_delete_legacy_triggers.rb b/db/post_migrate/20191204114127_delete_legacy_triggers.rb new file mode 100644 index 00000000000..82d901ae689 --- /dev/null +++ b/db/post_migrate/20191204114127_delete_legacy_triggers.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class DeleteLegacyTriggers < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + execute <<~SQL + DELETE FROM ci_triggers WHERE owner_id IS NULL + SQL + + change_column_null :ci_triggers, :owner_id, false + end + + def down + change_column_null :ci_triggers, :owner_id, true + end +end diff --git a/db/post_migrate/20191220102807_patch_prometheus_services_for_shared_cluster_applications.rb b/db/post_migrate/20191220102807_patch_prometheus_services_for_shared_cluster_applications.rb deleted file mode 100644 index 68361f7b176..00000000000 --- a/db/post_migrate/20191220102807_patch_prometheus_services_for_shared_cluster_applications.rb +++ /dev/null @@ -1,88 +0,0 @@ -# frozen_string_literal: true - -class PatchPrometheusServicesForSharedClusterApplications < ActiveRecord::Migration[5.2] - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - MIGRATION = 'ActivatePrometheusServicesForSharedClusterApplications'.freeze - BATCH_SIZE = 500 - DELAY = 2.minutes - - disable_ddl_transaction! - - module Migratable - module Applications - class Prometheus < ActiveRecord::Base - self.table_name = 'clusters_applications_prometheus' - - enum status: { - errored: -1, - installed: 3, - updated: 5 - } - end - end - - class Project < ActiveRecord::Base - self.table_name = 'projects' - include ::EachBatch - - scope :with_application_on_group_clusters, -> { - joins("INNER JOIN namespaces ON namespaces.id = projects.namespace_id") - .joins("INNER JOIN cluster_groups ON cluster_groups.group_id = namespaces.id") - .joins("INNER JOIN clusters ON clusters.id = cluster_groups.cluster_id AND clusters.cluster_type = #{Cluster.cluster_types['group_type']}") - .joins("INNER JOIN clusters_applications_prometheus ON clusters_applications_prometheus.cluster_id = clusters.id - AND clusters_applications_prometheus.status IN (#{Applications::Prometheus.statuses[:installed]}, #{Applications::Prometheus.statuses[:updated]})") - } - - scope :without_active_prometheus_services, -> { - joins("LEFT JOIN services ON services.project_id = projects.id AND services.type = 'PrometheusService'") - .where("services.id IS NULL OR (services.active = FALSE AND services.properties = '{}')") - } - end - - class Cluster < ActiveRecord::Base - self.table_name = 'clusters' - - enum cluster_type: { - instance_type: 1, - group_type: 2 - } - - def self.has_prometheus_application? - joins("INNER JOIN clusters_applications_prometheus ON clusters_applications_prometheus.cluster_id = clusters.id - AND clusters_applications_prometheus.status IN (#{Applications::Prometheus.statuses[:installed]}, #{Applications::Prometheus.statuses[:updated]})").exists? - end - end - end - - def up - projects_without_active_prometheus_service.group('projects.id').each_batch(of: BATCH_SIZE) do |batch, index| - bg_migrations_batch = batch.select('projects.id').map { |project| [MIGRATION, project.id] } - delay = index * DELAY - BackgroundMigrationWorker.bulk_perform_in(delay.seconds, bg_migrations_batch) - end - end - - def down - # no-op - end - - private - - def projects_without_active_prometheus_service - scope = Migratable::Project.without_active_prometheus_services - - return scope if migrate_instance_cluster? - - scope.with_application_on_group_clusters - end - - def migrate_instance_cluster? - if instance_variable_defined?('@migrate_instance_cluster') - @migrate_instance_cluster - else - @migrate_instance_cluster = Migratable::Cluster.instance_type.has_prometheus_application? - end - end -end diff --git a/db/schema.rb b/db/schema.rb index 689428ea4cb..56b0cbde61a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -994,7 +994,7 @@ ActiveRecord::Schema.define(version: 2020_01_08_233040) do t.datetime "created_at" t.datetime "updated_at" t.integer "project_id" - t.integer "owner_id" + t.integer "owner_id", null: false t.string "description" t.string "ref" t.index ["owner_id"], name: "index_ci_triggers_on_owner_id" diff --git a/doc/ci/img/parent_pipeline_graph_expanded_v12_6.png b/doc/ci/img/parent_pipeline_graph_expanded_v12_6.png Binary files differnew file mode 100644 index 00000000000..5c493109a54 --- /dev/null +++ b/doc/ci/img/parent_pipeline_graph_expanded_v12_6.png diff --git a/doc/ci/parent_child_pipelines.md b/doc/ci/parent_child_pipelines.md new file mode 100644 index 00000000000..269cbd75a9a --- /dev/null +++ b/doc/ci/parent_child_pipelines.md @@ -0,0 +1,86 @@ +--- +type: reference +--- + +# Parent-child pipelines + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/16094) in GitLab Starter 12.7. + +As pipelines grow more complex, a few related problems start to emerge: + +- The staged structure, where all steps in a stage must be completed before the first + job in next stage begins, causes arbitrary waits, slowing things down. +- Configuration for the single global pipeline becomes very long and complicated, + making it hard to manage. +- Imports with [`include`](yaml/README.md#include) increase the complexity of the configuration, and create the potential + for namespace collisions where jobs are unintentionally duplicated. +- Pipeline UX can become unwieldy with so many jobs and stages to work with. + +Additionally, sometimes the behavior of a pipeline needs to be more dynamic. The ability +to choose to start sub-pipelines (or not) is a powerful ability, especially if the +YAML is dynamically generated. + + + +Similarly to [multi-project pipelines](multi_project_pipelines.md), a pipeline can trigger a +set of concurrently running child pipelines, but within the same project: + +- Child pipelines still execute each of their jobs according to a stage sequence, but + would be free to continue forward through their stages without waiting for unrelated + jobs in the parent pipeline to finish. +- The configuration is split up into smaller child pipeline configurations, which are + easier to understand. This reduces the cognitive load to understand the overall configuration. +- Imports are done at the child pipeline level, reducing the likelihood of collisions. +- Each pipeline has only the steps relevant steps, making it easier to understand what's going on. + +Child pipelines work well with other GitLab CI features: + +- Use [`only: changes`](yaml/README.md#onlychangesexceptchanges) to trigger pipelines only when + certain files change. This is useful for monorepos, for example. +- Since the parent pipeline in `.gitlab-ci.yml` and the child pipeline run as normal + pipelines, they can have their own behaviors and sequencing in relation to triggers. + +All of this will work with [`include:`](yaml/README.md#include) feature so you can compose +the child pipeline configuration. + +## Examples + +The simplest case is [triggering a child pipeline](yaml/README.md#trigger-premium) using a +local YAML file to define the pipeline configuration. In this case, the parent pipeline will +trigger the child pipeline, and continue without waiting: + +```yaml +microservice_a: + trigger: + include: path/to/microservice_a.yml +``` + +You can include multiple files when composing a child pipeline: + +```yaml +microservice_a: + trigger: + include: + - local: path/to/microservice_a.yml + - template: SAST.gitlab-ci.yml +``` + +NOTE: **Note:** +The max number of entries that are accepted for `trigger:include:` is three. + +Similar to [multi-project pipelines](multi_project_pipelines.md#mirroring-status-from-triggered-pipeline), +we can set the parent pipeline to depend on the status of the child pipeline upon completion: + +```yaml +microservice_a: + trigger: + include: + - local: path/to/microservice_a.yml + - template: SAST.gitlab-ci.yml + strategy: depend +``` + +## Limitations + +A parent pipeline can trigger many child pipelines, but a child pipeline cannot trigger +further child pipelines. See the [related issue](https://gitlab.com/gitlab-org/gitlab/issues/29651) for discussion on possible future improvements. diff --git a/doc/ci/pipelines.md b/doc/ci/pipelines.md index 4d942ea3d54..71c4c9ca0ec 100644 --- a/doc/ci/pipelines.md +++ b/doc/ci/pipelines.md @@ -246,6 +246,13 @@ Pipelines for different projects can be combined and visualized together. For more information, see [Multi-project pipelines](multi_project_pipelines.md). +## Parent-child pipelines + +Complex pipelines can be broken down into one parent pipeline that can trigger +multiple child sub-pipelines, which all run in the same project and with the same SHA. + +For more information, see [Parent-Child pipelines](parent_child_pipelines.md). + ## Working with pipelines In general, pipelines are executed automatically and require no intervention once created. diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index e388fdde58a..aafbe4c9189 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -2600,14 +2600,17 @@ job split into three separate jobs. from `trigger` definition is started by GitLab, a downstream pipeline gets created. -Learn more about [multi-project pipelines](../multi_project_pipelines.md#creating-multi-project-pipelines-from-gitlab-ciyml). +This keyword allows the creation of two different types of downstream pipelines: + +- [Multi-project pipelines](../multi_project_pipelines.md#creating-multi-project-pipelines-from-gitlab-ciyml) +- [Child pipelines](../parent_child_pipelines.md) NOTE: **Note:** Using a `trigger` with `when:manual` together results in the error `jobs:#{job-name} when should be on_success, on_failure or always`, because `when:manual` prevents triggers being used. -#### Simple `trigger` syntax +#### Simple `trigger` syntax for multi-project pipelines The simplest way to configure a downstream trigger is to use `trigger` keyword with a full path to a downstream project: @@ -2622,7 +2625,7 @@ staging: trigger: my/deployment ``` -#### Complex `trigger` syntax +#### Complex `trigger` syntax for multi-project pipelines It is possible to configure a branch name that GitLab will use to create a downstream pipeline with: @@ -2657,6 +2660,28 @@ upstream_bridge: pipeline: other/project ``` +#### `trigger` syntax for child pipeline + +To create a [child pipeline](../parent_child_pipelines.md), specify the path to the +YAML file containing the CI config of the child pipeline: + +```yaml +trigger_job: + trigger: + include: path/to/child-pipeline.yml +``` + +Similar to [multi-project pipelines](../multi_project_pipelines.md#mirroring-status-from-triggered-pipeline), +it is possible to mirror the status from a triggered pipeline: + +```yaml +trigger_job: + trigger: + include: + - local: path/to/child-pipeline.yml + strategy: depend +``` + ### `interruptible` > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/23464) in GitLab 12.3. diff --git a/lib/gitlab/background_migration/activate_prometheus_services_for_shared_cluster_applications.rb b/lib/gitlab/background_migration/activate_prometheus_services_for_shared_cluster_applications.rb deleted file mode 100644 index 19f5821d449..00000000000 --- a/lib/gitlab/background_migration/activate_prometheus_services_for_shared_cluster_applications.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module BackgroundMigration - # Create missing PrometheusServices records or sets active attribute to true - # for all projects which belongs to cluster with Prometheus Application installed. - class ActivatePrometheusServicesForSharedClusterApplications - module Migratable - # Migration model namespace isolated from application code. - class PrometheusService < ActiveRecord::Base - self.inheritance_column = :_type_disabled - self.table_name = 'services' - - default_scope { where("services.type = 'PrometheusService'") } - - def self.for_project(project_id) - new( - project_id: project_id, - active: true, - properties: '{}', - type: 'PrometheusService', - template: false, - push_events: true, - issues_events: true, - merge_requests_events: true, - tag_push_events: true, - note_events: true, - category: 'monitoring', - default: false, - wiki_page_events: true, - pipeline_events: true, - confidential_issues_events: true, - commit_events: true, - job_events: true, - confidential_note_events: true, - deployment_events: false - ) - end - - def managed? - properties == '{}' - end - end - end - - def perform(project_id) - service = Migratable::PrometheusService.find_by(project_id: project_id) || Migratable::PrometheusService.for_project(project_id) - service.update!(active: true) if service.managed? - end - end - end -end diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 0f355906948..6a16e6df23d 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -22,12 +22,6 @@ module Gitlab end end - def uses_unsupported_legacy_trigger? - trigger_request.present? && - trigger_request.trigger.legacy? && - !trigger_request.trigger.supports_legacy_tokens? - end - def branch_exists? strong_memoize(:is_branch) do project.repository.branch_exists?(ref) diff --git a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb index f9ed9d91177..a30b6c6ef0e 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb @@ -14,16 +14,12 @@ module Gitlab return error('Pipelines are disabled!') end - if @command.uses_unsupported_legacy_trigger? - return error('Trigger token is invalid because is not owned by any user') + unless allowed_to_create_pipeline? + return error('Insufficient permissions to create a new pipeline') end - unless allowed_to_trigger_pipeline? - if can?(current_user, :create_pipeline, project) - return error("Insufficient permissions for protected ref '#{command.ref}'") - else - return error('Insufficient permissions to create a new pipeline') - end + unless allowed_to_write_ref? + return error("Insufficient permissions for protected ref '#{command.ref}'") end end @@ -31,17 +27,13 @@ module Gitlab @pipeline.errors.any? end - def allowed_to_trigger_pipeline? - if current_user - allowed_to_create? - else # legacy triggers don't have a corresponding user - !@command.protected_ref? - end - end + private - def allowed_to_create? - return unless can?(current_user, :create_pipeline, project) + def allowed_to_create_pipeline? + can?(current_user, :create_pipeline, project) + end + def allowed_to_write_ref? access = Gitlab::UserAccess.new(current_user, project: project) if @command.branch_exists? diff --git a/lib/gitlab/import_export/group_project_object_builder.rb b/lib/gitlab/import_export/group_project_object_builder.rb index b94839363df..2e7ab3d4b69 100644 --- a/lib/gitlab/import_export/group_project_object_builder.rb +++ b/lib/gitlab/import_export/group_project_object_builder.rb @@ -26,6 +26,8 @@ module Gitlab end def find + return if epic? && group.nil? + find_object || klass.create(project_attributes) end @@ -54,10 +56,10 @@ module Gitlab # or, if group is present: # `"{table_name}"."project_id" = {project.id} OR "{table_name}"."group_id" = {group.id}` def where_clause_base - clause = table[:project_id].eq(project.id) if project - clause = clause.or(table[:group_id].eq(group.id)) if group - - clause + [].tap do |clauses| + clauses << table[:project_id].eq(project.id) if project + clauses << table[:group_id].eq(group.id) if group + end.reduce(:or) end # Returns Arel clause `"{table_name}"."title" = '{attributes['title']}'` @@ -108,6 +110,10 @@ module Gitlab klass == MergeRequest end + def epic? + klass == Epic + end + # If an existing group milestone used the IID # claim the IID back and set the group milestone to use one available # This is necessary to fix situations like the following: diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 3208e6648a7..0f0397ec13b 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -322,6 +322,13 @@ excluded_attributes: - :board_id - :label_id - :milestone_id + epic: + - :start_date_sourcing_milestone_id + - :due_date_sourcing_milestone_id + - :parent_id + - :state_id + - :start_date_sourcing_epic_id + - :due_date_sourcing_epic_id methods: notes: - :type @@ -374,6 +381,7 @@ ee: - design_versions: - actions: - :design # Duplicate export of issues.designs in order to link the record to both Issue and Action + - :epic - protected_branches: - :unprotect_access_levels - protected_environments: diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index 9dd30803196..9a5e01462fb 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -40,7 +40,21 @@ module Gitlab IMPORTED_OBJECT_MAX_RETRIES = 5.freeze - EXISTING_OBJECT_CHECK = %i[milestone milestones label labels project_label project_labels group_label group_labels project_feature merge_request ProjectCiCdSetting container_expiration_policy].freeze + EXISTING_OBJECT_CHECK = %i[ + milestone + milestones + label + labels + project_label + project_labels + group_label + group_labels + project_feature + merge_request + epic + ProjectCiCdSetting + container_expiration_policy + ].freeze TOKEN_RESET_MODELS = %i[Project Namespace Ci::Trigger Ci::Build Ci::Runner ProjectHook].freeze @@ -86,9 +100,6 @@ module Gitlab def create return if unknown_service? - # Do not import legacy triggers - return if !Feature.enabled?(:use_legacy_pipeline_triggers, @project) && legacy_trigger? - setup_models object = generate_imported_object @@ -345,10 +356,6 @@ module Gitlab !Object.const_defined?(parsed_relation_hash['type']) end - def legacy_trigger? - @relation_name == :'Ci::Trigger' && @relation_hash['owner_id'].nil? - end - def find_or_create_object! if UNIQUE_RELATIONS.include?(@relation_name) unique_relation_object = relation_class.find_or_create_by(project_id: @project.id) diff --git a/lib/gitlab/import_export/relation_tree_restorer.rb b/lib/gitlab/import_export/relation_tree_restorer.rb index 886087bc73a..4f58406360e 100644 --- a/lib/gitlab/import_export/relation_tree_restorer.rb +++ b/lib/gitlab/import_export/relation_tree_restorer.rb @@ -4,7 +4,7 @@ module Gitlab module ImportExport class RelationTreeRestorer # Relations which cannot be saved at project level (and have a group assigned) - GROUP_MODELS = [GroupLabel, Milestone].freeze + GROUP_MODELS = [GroupLabel, Milestone, Epic].freeze attr_reader :user attr_reader :shared diff --git a/package.json b/package.json index 95bf5214b78..d658b8ff2ec 100644 --- a/package.json +++ b/package.json @@ -82,6 +82,7 @@ "fuzzaldrin-plus": "^0.5.0", "glob": "^7.1.2", "graphql": "^14.0.2", + "immer": "^5.2.1", "imports-loader": "^0.8.0", "jed": "^1.1.1", "jest-transform-graphql": "^2.1.0", diff --git a/qa/qa/page/project/web_ide/edit.rb b/qa/qa/page/project/web_ide/edit.rb index 4d26cadcdfe..b22828e554f 100644 --- a/qa/qa/page/project/web_ide/edit.rb +++ b/qa/qa/page/project/web_ide/edit.rb @@ -108,9 +108,9 @@ module QA # Click :commit_button and keep retrying just in case part of the # animation is still in process even when the buttons have the # expected visibility. - commit_success_msg_shown = retry_until do - click_element :commit_to_current_branch_radio - click_element :commit_button + commit_success_msg_shown = retry_until(sleep_interval: 5) do + click_element(:commit_to_current_branch_radio) if has_element?(:commit_to_current_branch_radio) + click_element(:commit_button) if has_element?(:commit_button) wait(reload: false) do has_text?('Your changes have been committed') diff --git a/spec/controllers/admin/sessions_controller_spec.rb b/spec/controllers/admin/sessions_controller_spec.rb index bd0bb0bd81f..be996aee1d2 100644 --- a/spec/controllers/admin/sessions_controller_spec.rb +++ b/spec/controllers/admin/sessions_controller_spec.rb @@ -122,7 +122,7 @@ describe Admin::SessionsController, :do_not_mock_admin_mode do describe '#destroy' do context 'for regular users' do it 'shows error page' do - get :destroy + post :destroy expect(response).to have_gitlab_http_status(404) expect(controller.current_user_mode.admin_mode?).to be(false) @@ -139,7 +139,7 @@ describe Admin::SessionsController, :do_not_mock_admin_mode do post :create, params: { password: user.password } expect(controller.current_user_mode.admin_mode?).to be(true) - get :destroy + post :destroy expect(response).to have_gitlab_http_status(:found) expect(response).to redirect_to(root_path) diff --git a/spec/features/merge_request/maintainer_edits_fork_spec.rb b/spec/features/merge_request/maintainer_edits_fork_spec.rb index 4e161d530d3..4f2c5fc73d8 100644 --- a/spec/features/merge_request/maintainer_edits_fork_spec.rb +++ b/spec/features/merge_request/maintainer_edits_fork_spec.rb @@ -32,8 +32,6 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js wait_for_requests end - it_behaves_like 'rendering a single diff version' - it 'mentions commits will go to the source branch' do expect(page).to have_content('Your changes can be committed to fix because a merge request is open.') end diff --git a/spec/features/merge_request/user_comments_on_diff_spec.rb b/spec/features/merge_request/user_comments_on_diff_spec.rb index 6a23b6cdf60..19b8a7f74b7 100644 --- a/spec/features/merge_request/user_comments_on_diff_spec.rb +++ b/spec/features/merge_request/user_comments_on_diff_spec.rb @@ -13,15 +13,12 @@ describe 'User comments on a diff', :js do let(:user) { create(:user) } before do - stub_feature_flags(single_mr_diff_view: false) project.add_maintainer(user) sign_in(user) visit(diffs_project_merge_request_path(project, merge_request)) end - it_behaves_like 'rendering a single diff version' - context 'when viewing comments' do context 'when toggling inline comments' do context 'in a single file' do diff --git a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb index e6634a8ff39..e0724a04ea3 100644 --- a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb +++ b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb @@ -9,7 +9,6 @@ describe 'Merge request > User creates image diff notes', :js do let(:user) { project.creator } before do - stub_feature_flags(single_mr_diff_view: false) sign_in(user) # Stub helper to return any blob file as image from public app folder. @@ -18,8 +17,6 @@ describe 'Merge request > User creates image diff notes', :js do allow_any_instance_of(DiffHelper).to receive(:diff_file_old_blob_raw_url).and_return('/favicon.png') end - it_behaves_like 'rendering a single diff version' - context 'create commit diff notes' do commit_id = '2f63565e7aac07bcdadb654e253078b727143ec4' diff --git a/spec/features/merge_request/user_expands_diff_spec.rb b/spec/features/merge_request/user_expands_diff_spec.rb index 9b040271468..9bce5264817 100644 --- a/spec/features/merge_request/user_expands_diff_spec.rb +++ b/spec/features/merge_request/user_expands_diff_spec.rb @@ -7,7 +7,6 @@ describe 'User expands diff', :js do let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) } before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes) @@ -18,8 +17,6 @@ describe 'User expands diff', :js do wait_for_requests end - it_behaves_like 'rendering a single diff version' - it 'allows user to expand diff' do page.within find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9"]') do click_link 'Click to expand it.' diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index 6328c0a5133..8b16760606c 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb @@ -14,15 +14,12 @@ describe 'Merge request > User posts diff notes', :js do let(:test_note_comment) { 'this is a test note!' } before do - stub_feature_flags(single_mr_diff_view: false) set_cookie('sidebar_collapsed', 'true') project.add_developer(user) sign_in(user) end - it_behaves_like 'rendering a single diff version' - context 'when hovering over a parallel view diff file' do before do visit diffs_project_merge_request_path(project, merge_request, view: 'parallel') diff --git a/spec/features/merge_request/user_posts_notes_spec.rb b/spec/features/merge_request/user_posts_notes_spec.rb index c0655581b18..f24e7090605 100644 --- a/spec/features/merge_request/user_posts_notes_spec.rb +++ b/spec/features/merge_request/user_posts_notes_spec.rb @@ -165,9 +165,9 @@ describe 'Merge request > User posts notes', :js do find('.js-note-edit').click page.within('.current-note-edit-form') do - expect(find('#note_note').value).to eq('This is the new content') + expect(find('#note_note').value).to include('This is the new content') first('.js-md').click - expect(find('#note_note').value).to eq('This is the new content****') + expect(find('#note_note').value).to include('This is the new content****') end end diff --git a/spec/features/merge_request/user_resolves_conflicts_spec.rb b/spec/features/merge_request/user_resolves_conflicts_spec.rb index f0949fefa3b..ce85e81868d 100644 --- a/spec/features/merge_request/user_resolves_conflicts_spec.rb +++ b/spec/features/merge_request/user_resolves_conflicts_spec.rb @@ -9,7 +9,6 @@ describe 'Merge request > User resolves conflicts', :js do before do # In order to have the diffs collapsed, we need to disable the increase feature stub_feature_flags(gitlab_git_diff_size_limit_increase: false) - stub_feature_flags(single_mr_diff_view: false) end def create_merge_request(source_branch) @@ -18,8 +17,6 @@ describe 'Merge request > User resolves conflicts', :js do end end - it_behaves_like 'rendering a single diff version' - shared_examples 'conflicts are resolved in Interactive mode' do it 'conflicts are resolved in Interactive mode' do within find('.files-wrapper .diff-file', text: 'files/ruby/popen.rb') do 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 9cbea8a8466..eb86b1e33af 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 @@ -20,12 +20,9 @@ describe 'Merge request > User resolves diff notes and threads', :js do end before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) end - it_behaves_like 'rendering a single diff version' - context 'no threads' do before do project.add_maintainer(user) diff --git a/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb b/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb index 70afe056c64..3e77b9e75d6 100644 --- a/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb +++ b/spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb @@ -21,7 +21,6 @@ describe 'Merge request > User sees avatars on diff notes', :js do let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) } before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) project.add_maintainer(user) sign_in user @@ -29,8 +28,6 @@ describe 'Merge request > User sees avatars on diff notes', :js do set_cookie('sidebar_collapsed', 'true') end - it_behaves_like 'rendering a single diff version' - context 'discussion tab' do before do visit project_merge_request_path(project, merge_request) diff --git a/spec/features/merge_request/user_sees_diff_spec.rb b/spec/features/merge_request/user_sees_diff_spec.rb index de142344c26..2d91d09a486 100644 --- a/spec/features/merge_request/user_sees_diff_spec.rb +++ b/spec/features/merge_request/user_sees_diff_spec.rb @@ -10,12 +10,9 @@ describe 'Merge request > User sees diff', :js do let(:merge_request) { create(:merge_request, source_project: project) } before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) end - it_behaves_like 'rendering a single diff version' - context 'when linking to note' do describe 'with unresolved note' do let(:note) { create :diff_note_on_merge_request, project: project, noteable: merge_request } diff --git a/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb b/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb index e28d2ca5536..59e5f5c847d 100644 --- a/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb +++ b/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb @@ -11,14 +11,11 @@ describe 'Merge request > User sees MR with deleted source branch', :js do let(:user) { project.creator } before do - stub_feature_flags(single_mr_diff_view: false) merge_request.update!(source_branch: 'this-branch-does-not-exist') sign_in(user) visit project_merge_request_path(project, merge_request) end - it_behaves_like 'rendering a single diff version' - it 'shows a message about missing source branch' do expect(page).to have_content('Source branch does not exist.') end diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb index b3aef601c7b..cab86f3fd94 100644 --- a/spec/features/merge_request/user_sees_versions_spec.rb +++ b/spec/features/merge_request/user_sees_versions_spec.rb @@ -16,7 +16,6 @@ describe 'Merge request > User sees versions', :js do let!(:params) { {} } before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) project.add_maintainer(user) @@ -24,8 +23,6 @@ describe 'Merge request > User sees versions', :js do visit diffs_project_merge_request_path(project, merge_request, params) end - it_behaves_like 'rendering a single diff version' - shared_examples 'allows commenting' do |file_id:, line_code:, comment:| it do diff_file_selector = ".diff-file[id='#{file_id}']" diff --git a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb index 085fe38b47a..95cb0a2dee3 100644 --- a/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb +++ b/spec/features/merge_request/user_suggests_changes_on_diff_spec.rb @@ -25,15 +25,12 @@ describe 'User comments on a diff', :js do let(:user) { create(:user) } before do - stub_feature_flags(single_mr_diff_view: false) project.add_maintainer(user) sign_in(user) visit(diffs_project_merge_request_path(project, merge_request)) end - it_behaves_like 'rendering a single diff version' - context 'single suggestion note' do it 'hides suggestion popover' do click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) diff --git a/spec/features/merge_request/user_toggles_whitespace_changes_spec.rb b/spec/features/merge_request/user_toggles_whitespace_changes_spec.rb index 5e59bc87e68..4db067a4e41 100644 --- a/spec/features/merge_request/user_toggles_whitespace_changes_spec.rb +++ b/spec/features/merge_request/user_toggles_whitespace_changes_spec.rb @@ -8,7 +8,6 @@ describe 'Merge request > User toggles whitespace changes', :js do let(:user) { project.creator } before do - stub_feature_flags(single_mr_diff_view: false) project.add_maintainer(user) sign_in(user) visit diffs_project_merge_request_path(project, merge_request) @@ -16,8 +15,6 @@ describe 'Merge request > User toggles whitespace changes', :js do find('.js-show-diff-settings').click end - it_behaves_like 'rendering a single diff version' - it 'has a button to toggle whitespace changes' do expect(page).to have_content 'Show whitespace changes' end diff --git a/spec/features/merge_request/user_views_diffs_spec.rb b/spec/features/merge_request/user_views_diffs_spec.rb index 313f438e23b..e0e4058dd47 100644 --- a/spec/features/merge_request/user_views_diffs_spec.rb +++ b/spec/features/merge_request/user_views_diffs_spec.rb @@ -9,7 +9,6 @@ describe 'User views diffs', :js do let(:project) { create(:project, :public, :repository) } before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) visit(diffs_project_merge_request_path(project, merge_request)) @@ -18,8 +17,6 @@ describe 'User views diffs', :js do find('.js-toggle-tree-list').click end - it_behaves_like 'rendering a single diff version' - shared_examples 'unfold diffs' do it 'unfolds diffs upwards' do first('.js-unfold').click diff --git a/spec/features/projects/blobs/edit_spec.rb b/spec/features/projects/blobs/edit_spec.rb index 0a5bc64b429..a1d6a8896c7 100644 --- a/spec/features/projects/blobs/edit_spec.rb +++ b/spec/features/projects/blobs/edit_spec.rb @@ -12,11 +12,9 @@ describe 'Editing file blob', :js do let(:readme_file_path) { 'README.md' } before do - stub_feature_flags(web_ide_default: false, single_mr_diff_view: false) + stub_feature_flags(web_ide_default: false) end - it_behaves_like 'rendering a single diff version' - context 'as a developer' do let(:user) { create(:user) } let(:role) { :developer } diff --git a/spec/features/projects/view_on_env_spec.rb b/spec/features/projects/view_on_env_spec.rb index c2d4cefad12..8b25565c08a 100644 --- a/spec/features/projects/view_on_env_spec.rb +++ b/spec/features/projects/view_on_env_spec.rb @@ -9,14 +9,11 @@ describe 'View on environment', :js do let(:user) { project.creator } before do - stub_feature_flags(single_mr_diff_view: false) stub_feature_flags(diffs_batch_load: false) project.add_maintainer(user) end - it_behaves_like 'rendering a single diff version' - context 'when the branch has a route map' do let(:route_map) do <<-MAP.strip_heredoc diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 19cd21e4161..af406961bbc 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -65,22 +65,6 @@ describe 'Triggers', :js do expect(page.find('.triggers-list')).to have_content new_trigger_title expect(page.find('.triggers-list .trigger-owner')).to have_content user.name end - - it 'edit "legacy" trigger and save' do - # Create new trigger without owner association, i.e. Legacy trigger - create(:ci_trigger, owner: user, project: @project).update_attribute(:owner, nil) - visit project_settings_ci_cd_path(@project) - - # See if the trigger can be edited and description is blank - find('a[title="Edit"]').send_keys(:return) - expect(page.find('#trigger_description').value).to have_content '' - - # See if trigger can be updated with description and saved successfully - fill_in 'trigger_description', with: new_trigger_title - click_button 'Save trigger' - expect(page.find('.flash-notice')).to have_content 'Trigger was successfully updated.' - expect(page.find('.triggers-list')).to have_content new_trigger_title - end end describe 'trigger "Revoke" workflow' do @@ -106,43 +90,18 @@ describe 'Triggers', :js do end describe 'show triggers workflow' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: false) - end - it 'contains trigger description placeholder' do expect(page.find('#trigger_description')['placeholder']).to eq 'Trigger description' end - it 'show "invalid" badge for legacy trigger' do - create(:ci_trigger, owner: user, project: @project).update_attribute(:owner, nil) - visit project_settings_ci_cd_path(@project) - - expect(page.find('.triggers-list')).to have_content 'invalid' - end - it 'show "invalid" badge for trigger with owner having insufficient permissions' do create(:ci_trigger, owner: guest_user, project: @project, description: trigger_title) visit project_settings_ci_cd_path(@project) - # See if trigger without owner (i.e. legacy) shows "legacy" badge and is non-editable expect(page.find('.triggers-list')).to have_content 'invalid' expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]') end - it 'do not show "Edit" or full token for legacy trigger' do - create(:ci_trigger, owner: user, project: @project, description: trigger_title) - .update_attribute(:owner, nil) - visit project_settings_ci_cd_path(@project) - - # See if trigger not owned shows only first few token chars and doesn't have copy-to-clipboard button - expect(page.find('.triggers-list')).to have_content(@project.triggers.first.token[0..3]) - expect(page.find('.triggers-list')).not_to have_selector('button.btn-clipboard') - - # See if trigger is non-editable - expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]') - end - it 'do not show "Edit" or full token for not owned trigger' do # Create trigger with user different from current_user create(:ci_trigger, owner: user2, project: @project, description: trigger_title) @@ -169,56 +128,5 @@ describe 'Triggers', :js do expect(page.find('.triggers-list .trigger-owner')).to have_content user.name expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]') end - - context 'when :use_legacy_pipeline_triggers feature flag is enabled' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: true) - end - - it 'show "legacy" badge for legacy trigger' do - create(:ci_trigger, owner: nil, project: @project) - visit project_settings_ci_cd_path(@project) - - # See if trigger without owner (i.e. legacy) shows "legacy" badge and is editable - expect(page.find('.triggers-list')).to have_content 'legacy' - expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]') - end - - it 'show "invalid" badge for trigger with owner having insufficient permissions' do - create(:ci_trigger, owner: guest_user, project: @project, description: trigger_title) - visit project_settings_ci_cd_path(@project) - - # See if trigger without owner (i.e. legacy) shows "legacy" badge and is non-editable - expect(page.find('.triggers-list')).to have_content 'invalid' - expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]') - end - - it 'do not show "Edit" or full token for not owned trigger' do - # Create trigger with user different from current_user - create(:ci_trigger, owner: user2, project: @project, description: trigger_title) - visit project_settings_ci_cd_path(@project) - - # See if trigger not owned by current_user shows only first few token chars and doesn't have copy-to-clipboard button - expect(page.find('.triggers-list')).to have_content(@project.triggers.first.token[0..3]) - expect(page.find('.triggers-list')).not_to have_selector('button.btn-clipboard') - - # See if trigger owner name doesn't match with current_user and trigger is non-editable - expect(page.find('.triggers-list .trigger-owner')).not_to have_content user.name - expect(page.find('.triggers-list')).not_to have_selector('a[title="Edit"]') - end - - it 'show "Edit" and full token for owned trigger' do - create(:ci_trigger, owner: user, project: @project, description: trigger_title) - visit project_settings_ci_cd_path(@project) - - # See if trigger shows full token and has copy-to-clipboard button - expect(page.find('.triggers-list')).to have_content @project.triggers.first.token - expect(page.find('.triggers-list')).to have_selector('button.btn-clipboard') - - # See if trigger owner name matches with current_user and is editable - expect(page.find('.triggers-list .trigger-owner')).to have_content user.name - expect(page.find('.triggers-list')).to have_selector('a[title="Edit"]') - end - end end end diff --git a/spec/fixtures/lib/gitlab/import_export/group/project.json b/spec/fixtures/lib/gitlab/import_export/group/project.json index 47faf271cca..ce4fa1981ff 100644 --- a/spec/fixtures/lib/gitlab/import_export/group/project.json +++ b/spec/fixtures/lib/gitlab/import_export/group/project.json @@ -175,6 +175,67 @@ } } ] + }, + { + "id": 3, + "title": "Issue with Epic", + "author_id": 1, + "project_id": 8, + "created_at": "2019-12-08T19:41:11.233Z", + "updated_at": "2019-12-08T19:41:53.194Z", + "position": 0, + "branch_name": null, + "description": "Donec at nulla vitae sem molestie rutrum ut at sem.", + "state": "opened", + "iid": 3, + "updated_by_id": null, + "confidential": false, + "due_date": null, + "moved_to_id": null, + "issue_assignees": [], + "notes": [], + "milestone": { + "id": 2, + "title": "A group milestone", + "description": "Group-level milestone", + "due_date": null, + "created_at": "2016-06-14T15:02:04.415Z", + "updated_at": "2016-06-14T15:02:04.415Z", + "state": "active", + "iid": 1, + "group_id": 100 + }, + "epic": { + "id": 1, + "group_id": 5, + "author_id": 1, + "assignee_id": null, + "iid": 1, + "updated_by_id": null, + "last_edited_by_id": null, + "lock_version": 0, + "start_date": null, + "end_date": null, + "last_edited_at": null, + "created_at": "2019-12-08T19:37:07.098Z", + "updated_at": "2019-12-08T19:43:11.568Z", + "title": "An epic", + "description": null, + "start_date_sourcing_milestone_id": null, + "due_date_sourcing_milestone_id": null, + "start_date_fixed": null, + "due_date_fixed": null, + "start_date_is_fixed": null, + "due_date_is_fixed": null, + "closed_by_id": null, + "closed_at": null, + "parent_id": null, + "relative_position": null, + "state_id": "opened", + "start_date_sourcing_epic_id": null, + "due_date_sourcing_epic_id": null, + "milestone_id": null + } } ], "snippets": [ diff --git a/spec/frontend/error_tracking/components/error_details_spec.js b/spec/frontend/error_tracking/components/error_details_spec.js index eefaff4aba7..b5ce9383eb9 100644 --- a/spec/frontend/error_tracking/components/error_details_spec.js +++ b/spec/frontend/error_tracking/components/error_details_spec.js @@ -13,6 +13,7 @@ describe('ErrorDetails', () => { let wrapper; let actions; let getters; + let mocks; const findInput = name => { const inputs = wrapper.findAll(GlFormInput).filter(c => c.attributes('name') === name); @@ -24,13 +25,27 @@ describe('ErrorDetails', () => { stubs: { LoadingButton }, localVue, store, + mocks, propsData: { + issueId: '123', + projectPath: '/root/gitlab-test', issueDetailsPath: '/123/details', issueStackTracePath: '/stacktrace', projectIssuesPath: '/test-project/issues/', csrfToken: 'fakeToken', }, }); + wrapper.setData({ + GQLerror: { + id: 129381, + title: 'Issue title', + externalUrl: 'http://sentry.gitlab.net/gitlab', + firstSeen: '2017-05-26T13:32:48Z', + lastSeen: '2018-05-26T13:32:48Z', + count: 12, + userCount: 2, + }, + }); } beforeEach(() => { @@ -61,6 +76,19 @@ describe('ErrorDetails', () => { }, }, }); + + const query = jest.fn(); + mocks = { + $apollo: { + query, + queries: { + GQLerror: { + loading: true, + stopPolling: jest.fn(), + }, + }, + }, + }; }); afterEach(() => { @@ -85,10 +113,11 @@ describe('ErrorDetails', () => { beforeEach(() => { store.state.details.loading = false; store.state.details.error.id = 1; + mocks.$apollo.queries.GQLerror.loading = false; + mountComponent(); }); it('should show Sentry error details without stacktrace', () => { - mountComponent(); expect(wrapper.find(GlLink).exists()).toBe(true); expect(wrapper.find(GlLoadingIcon).exists()).toBe(true); expect(wrapper.find(Stacktrace).exists()).toBe(false); @@ -99,13 +128,17 @@ describe('ErrorDetails', () => { it('should show language and error level badges', () => { store.state.details.error.tags = { level: 'error', logger: 'ruby' }; mountComponent(); - expect(wrapper.findAll(GlBadge).length).toBe(2); + return wrapper.vm.$nextTick().then(() => { + expect(wrapper.findAll(GlBadge).length).toBe(2); + }); }); it('should NOT show the badge if the tag is not present', () => { store.state.details.error.tags = { level: 'error' }; mountComponent(); - expect(wrapper.findAll(GlBadge).length).toBe(1); + return wrapper.vm.$nextTick().then(() => { + expect(wrapper.findAll(GlBadge).length).toBe(1); + }); }); }); @@ -113,8 +146,10 @@ describe('ErrorDetails', () => { it('should show stacktrace', () => { store.state.details.loadingStacktrace = false; mountComponent(); - expect(wrapper.find(GlLoadingIcon).exists()).toBe(false); - expect(wrapper.find(Stacktrace).exists()).toBe(true); + return wrapper.vm.$nextTick().then(() => { + expect(wrapper.find(GlLoadingIcon).exists()).toBe(false); + expect(wrapper.find(Stacktrace).exists()).toBe(true); + }); }); it('should NOT show stacktrace if no entries', () => { @@ -128,15 +163,6 @@ describe('ErrorDetails', () => { describe('When a user clicks the create issue button', () => { beforeEach(() => { - store.state.details.error = { - id: 129381, - title: 'Issue title', - external_url: 'http://sentry.gitlab.net/gitlab', - first_seen: '2017-05-26T13:32:48Z', - last_seen: '2018-05-26T13:32:48Z', - count: 12, - user_count: 2, - }; mountComponent(); }); diff --git a/spec/frontend/helpers/diffs_helper_spec.js b/spec/frontend/helpers/diffs_helper_spec.js new file mode 100644 index 00000000000..b223d48bf5c --- /dev/null +++ b/spec/frontend/helpers/diffs_helper_spec.js @@ -0,0 +1,113 @@ +import * as diffsHelper from '~/helpers/diffs_helper'; + +describe('diffs helper', () => { + function getDiffFile(withOverrides = {}) { + return { + parallel_diff_lines: ['line'], + highlighted_diff_lines: ['line'], + blob: { + readable_text: 'text', + }, + ...withOverrides, + }; + } + + describe('hasInlineLines', () => { + it('is false when the file does not exist', () => { + expect(diffsHelper.hasInlineLines()).toBeFalsy(); + }); + + it('is false when the file does not have the highlighted_diff_lines property', () => { + const missingInline = getDiffFile({ highlighted_diff_lines: undefined }); + + expect(diffsHelper.hasInlineLines(missingInline)).toBeFalsy(); + }); + + it('is false when the file has zero highlighted_diff_lines', () => { + const emptyInline = getDiffFile({ highlighted_diff_lines: [] }); + + expect(diffsHelper.hasInlineLines(emptyInline)).toBeFalsy(); + }); + + it('is true when the file has at least 1 highlighted_diff_lines', () => { + expect(diffsHelper.hasInlineLines(getDiffFile())).toBeTruthy(); + }); + }); + + describe('hasParallelLines', () => { + it('is false when the file does not exist', () => { + expect(diffsHelper.hasParallelLines()).toBeFalsy(); + }); + + it('is false when the file does not have the parallel_diff_lines property', () => { + const missingInline = getDiffFile({ parallel_diff_lines: undefined }); + + expect(diffsHelper.hasParallelLines(missingInline)).toBeFalsy(); + }); + + it('is false when the file has zero parallel_diff_lines', () => { + const emptyInline = getDiffFile({ parallel_diff_lines: [] }); + + expect(diffsHelper.hasParallelLines(emptyInline)).toBeFalsy(); + }); + + it('is true when the file has at least 1 parallel_diff_lines', () => { + expect(diffsHelper.hasParallelLines(getDiffFile())).toBeTruthy(); + }); + }); + + describe('isSingleViewStyle', () => { + it('is true when the file has at least 1 inline line but no parallel lines for any reason', () => { + const noParallelLines = getDiffFile({ parallel_diff_lines: undefined }); + const emptyParallelLines = getDiffFile({ parallel_diff_lines: [] }); + + expect(diffsHelper.isSingleViewStyle(noParallelLines)).toBeTruthy(); + expect(diffsHelper.isSingleViewStyle(emptyParallelLines)).toBeTruthy(); + }); + + it('is true when the file has at least 1 parallel line but no inline lines for any reason', () => { + const noInlineLines = getDiffFile({ highlighted_diff_lines: undefined }); + const emptyInlineLines = getDiffFile({ highlighted_diff_lines: [] }); + + expect(diffsHelper.isSingleViewStyle(noInlineLines)).toBeTruthy(); + expect(diffsHelper.isSingleViewStyle(emptyInlineLines)).toBeTruthy(); + }); + + it('is true when the file does not have any inline lines or parallel lines for any reason', () => { + const noLines = getDiffFile({ + highlighted_diff_lines: undefined, + parallel_diff_lines: undefined, + }); + const emptyLines = getDiffFile({ + highlighted_diff_lines: [], + parallel_diff_lines: [], + }); + + expect(diffsHelper.isSingleViewStyle(noLines)).toBeTruthy(); + expect(diffsHelper.isSingleViewStyle(emptyLines)).toBeTruthy(); + expect(diffsHelper.isSingleViewStyle()).toBeTruthy(); + }); + + it('is false when the file has both inline and parallel lines', () => { + expect(diffsHelper.isSingleViewStyle(getDiffFile())).toBeFalsy(); + }); + }); + + describe.each` + context | inline | parallel | blob | expected + ${'only has inline lines'} | ${['line']} | ${undefined} | ${undefined} | ${true} + ${'only has parallel lines'} | ${undefined} | ${['line']} | ${undefined} | ${true} + ${"doesn't have inline, parallel, or blob"} | ${undefined} | ${undefined} | ${undefined} | ${true} + ${'has blob readable text'} | ${undefined} | ${undefined} | ${{ readable_text: 'text' }} | ${false} + `('when hasDiff', ({ context, inline, parallel, blob, expected }) => { + it(`${context}`, () => { + const diffFile = getDiffFile({ + highlighted_diff_lines: inline, + parallel_diff_lines: parallel, + blob, + }); + + expect(diffsHelper.hasDiff(diffFile)).toEqual(expected); + }); + }); +}); diff --git a/spec/frontend/monitoring/components/dashboard_spec.js b/spec/frontend/monitoring/components/dashboard_spec.js index 4caa66cf600..efae8b941ee 100644 --- a/spec/frontend/monitoring/components/dashboard_spec.js +++ b/spec/frontend/monitoring/components/dashboard_spec.js @@ -345,7 +345,7 @@ describe('Dashboard', () => { it('metrics can be swapped', done => { const firstDraggable = findDraggables().at(0); - const mockMetrics = [...metricsGroupsAPIResponse[1].panels]; + const mockMetrics = [...metricsGroupsAPIResponse.panel_groups[1].panels]; const firstTitle = mockMetrics[0].title; const secondTitle = mockMetrics[1].title; diff --git a/spec/frontend/monitoring/mock_data.js b/spec/frontend/monitoring/mock_data.js index 6ded22b4a3f..77c92d0eca6 100644 --- a/spec/frontend/monitoring/mock_data.js +++ b/spec/frontend/monitoring/mock_data.js @@ -331,77 +331,80 @@ export const mockedQueryResultPayloadCoresTotal = { ], }; -export const metricsGroupsAPIResponse = [ - { - group: 'Response metrics (NGINX Ingress VTS)', - priority: 10, - panels: [ - { - metrics: [ - { - id: 'response_metrics_nginx_ingress_throughput_status_code', - label: 'Status Code', - metric_id: 1, - prometheus_endpoint_path: - '/root/autodevops-deploy/environments/32/prometheus/api/v1/query_range?query=sum%28rate%28nginx_upstream_responses_total%7Bupstream%3D~%22%25%7Bkube_namespace%7D-%25%7Bci_environment_slug%7D-.%2A%22%7D%5B2m%5D%29%29+by+%28status_code%29', - query_range: - 'sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) by (status_code)', - unit: 'req / sec', - }, - ], - title: 'Throughput', - type: 'area-chart', - weight: 1, - y_label: 'Requests / Sec', - }, - ], - }, - { - group: 'System metrics (Kubernetes)', - priority: 5, - panels: [ - { - title: 'Memory Usage (Pod average)', - type: 'area-chart', - y_label: 'Memory Used per Pod', - weight: 2, - metrics: [ - { - id: 'system_metrics_kubernetes_container_memory_average', - query_range: - 'avg(sum(container_memory_usage_bytes{container_name!="POD",pod_name=~"^%{ci_environment_slug}-([^c].*|c([^a]|a([^n]|n([^a]|a([^r]|r[^y])))).*|)-(.*)",namespace="%{kube_namespace}"}) by (job)) without (job) / count(avg(container_memory_usage_bytes{container_name!="POD",pod_name=~"^%{ci_environment_slug}-([^c].*|c([^a]|a([^n]|n([^a]|a([^r]|r[^y])))).*|)-(.*)",namespace="%{kube_namespace}"}) without (job)) /1024/1024', - label: 'Pod average', - unit: 'MB', - metric_id: 17, - prometheus_endpoint_path: - '/root/autodevops-deploy/environments/32/prometheus/api/v1/query_range?query=avg%28sum%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+by+%28job%29%29+without+%28job%29+%2F+count%28avg%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+without+%28job%29%29+%2F1024%2F1024', - appearance: { - line: { - width: 2, +export const metricsGroupsAPIResponse = { + dashboard: 'Environment metrics', + panel_groups: [ + { + group: 'Response metrics (NGINX Ingress VTS)', + priority: 10, + panels: [ + { + metrics: [ + { + id: 'response_metrics_nginx_ingress_throughput_status_code', + label: 'Status Code', + metric_id: 1, + prometheus_endpoint_path: + '/root/autodevops-deploy/environments/32/prometheus/api/v1/query_range?query=sum%28rate%28nginx_upstream_responses_total%7Bupstream%3D~%22%25%7Bkube_namespace%7D-%25%7Bci_environment_slug%7D-.%2A%22%7D%5B2m%5D%29%29+by+%28status_code%29', + query_range: + 'sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) by (status_code)', + unit: 'req / sec', + }, + ], + title: 'Throughput', + type: 'area-chart', + weight: 1, + y_label: 'Requests / Sec', + }, + ], + }, + { + group: 'System metrics (Kubernetes)', + priority: 5, + panels: [ + { + title: 'Memory Usage (Pod average)', + type: 'area-chart', + y_label: 'Memory Used per Pod', + weight: 2, + metrics: [ + { + id: 'system_metrics_kubernetes_container_memory_average', + query_range: + 'avg(sum(container_memory_usage_bytes{container_name!="POD",pod_name=~"^%{ci_environment_slug}-([^c].*|c([^a]|a([^n]|n([^a]|a([^r]|r[^y])))).*|)-(.*)",namespace="%{kube_namespace}"}) by (job)) without (job) / count(avg(container_memory_usage_bytes{container_name!="POD",pod_name=~"^%{ci_environment_slug}-([^c].*|c([^a]|a([^n]|n([^a]|a([^r]|r[^y])))).*|)-(.*)",namespace="%{kube_namespace}"}) without (job)) /1024/1024', + label: 'Pod average', + unit: 'MB', + metric_id: 17, + prometheus_endpoint_path: + '/root/autodevops-deploy/environments/32/prometheus/api/v1/query_range?query=avg%28sum%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+by+%28job%29%29+without+%28job%29+%2F+count%28avg%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+without+%28job%29%29+%2F1024%2F1024', + appearance: { + line: { + width: 2, + }, }, }, - }, - ], - }, - { - title: 'Core Usage (Total)', - type: 'area-chart', - y_label: 'Total Cores', - weight: 3, - metrics: [ - { - id: 'system_metrics_kubernetes_container_cores_total', - query_range: - 'avg(sum(rate(container_cpu_usage_seconds_total{container_name!="POD",pod_name=~"^%{ci_environment_slug}-(.*)",namespace="%{kube_namespace}"}[15m])) by (job)) without (job)', - label: 'Total', - unit: 'cores', - metric_id: 13, - }, - ], - }, - ], - }, -]; + ], + }, + { + title: 'Core Usage (Total)', + type: 'area-chart', + y_label: 'Total Cores', + weight: 3, + metrics: [ + { + id: 'system_metrics_kubernetes_container_cores_total', + query_range: + 'avg(sum(rate(container_cpu_usage_seconds_total{container_name!="POD",pod_name=~"^%{ci_environment_slug}-(.*)",namespace="%{kube_namespace}"}[15m])) by (job)) without (job)', + label: 'Total', + unit: 'cores', + metric_id: 13, + }, + ], + }, + ], + }, + ], +}; export const environmentData = [ { diff --git a/spec/frontend/monitoring/store/actions_spec.js b/spec/frontend/monitoring/store/actions_spec.js index f38bd4384e2..c1ad59ac95b 100644 --- a/spec/frontend/monitoring/store/actions_spec.js +++ b/spec/frontend/monitoring/store/actions_spec.js @@ -298,7 +298,7 @@ describe('Monitoring store actions', () => { ); expect(commit).toHaveBeenCalledWith( types.RECEIVE_METRICS_DATA_SUCCESS, - metricsDashboardResponse.dashboard.panel_groups, + metricsDashboardResponse.dashboard, ); expect(dispatch).toHaveBeenCalledWith('fetchPrometheusMetrics', params); }); @@ -441,7 +441,7 @@ describe('Monitoring store actions', () => { beforeEach(() => { state = storeState(); [metric] = metricsDashboardResponse.dashboard.panel_groups[0].panels[0].metrics; - [data] = metricsGroupsAPIResponse[0].panels[0].metrics; + [data] = metricsGroupsAPIResponse.panel_groups[0].panels[0].metrics; }); it('commits result', done => { diff --git a/spec/frontend/monitoring/store/mutations_spec.js b/spec/frontend/monitoring/store/mutations_spec.js index 60107a03674..e89c22268d4 100644 --- a/spec/frontend/monitoring/store/mutations_spec.js +++ b/spec/frontend/monitoring/store/mutations_spec.js @@ -100,12 +100,12 @@ describe('Monitoring mutations', () => { values: [[0, 1], [1, 1], [1, 3]], }, ]; - const dashboardGroups = metricsDashboardResponse.dashboard.panel_groups; + const { dashboard } = metricsDashboardResponse; const getMetric = () => stateCopy.dashboard.panel_groups[0].panels[0].metrics[0]; describe('REQUEST_METRIC_RESULT', () => { beforeEach(() => { - mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); + mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboard); }); it('stores a loading state on a metric', () => { expect(stateCopy.showEmptyState).toBe(true); @@ -128,7 +128,7 @@ describe('Monitoring mutations', () => { describe('RECEIVE_METRIC_RESULT_SUCCESS', () => { beforeEach(() => { - mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); + mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboard); }); it('clears empty state', () => { expect(stateCopy.showEmptyState).toBe(true); @@ -161,7 +161,7 @@ describe('Monitoring mutations', () => { describe('RECEIVE_METRIC_RESULT_FAILURE', () => { beforeEach(() => { - mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboardGroups); + mutations[types.RECEIVE_METRICS_DATA_SUCCESS](stateCopy, dashboard); }); it('maintains the loading state when a metric fails', () => { expect(stateCopy.showEmptyState).toBe(true); diff --git a/spec/helpers/projects/error_tracking_helper_spec.rb b/spec/helpers/projects/error_tracking_helper_spec.rb index 753144eef89..7c67448034b 100644 --- a/spec/helpers/projects/error_tracking_helper_spec.rb +++ b/spec/helpers/projects/error_tracking_helper_spec.rb @@ -80,11 +80,20 @@ describe Projects::ErrorTrackingHelper do let(:issue_id) { 1234 } let(:route_params) { [project.owner, project, issue_id, { format: :json }] } let(:details_path) { details_namespace_project_error_tracking_index_path(*route_params) } + let(:project_path) { project.full_path } let(:stack_trace_path) { stack_trace_namespace_project_error_tracking_index_path(*route_params) } let(:issues_path) { project_issues_path(project) } let(:result) { helper.error_details_data(project, issue_id) } + it 'returns the correct issue id' do + expect(result['issue-id']).to eq issue_id + end + + it 'returns the correct project path' do + expect(result['project-path']).to eq project_path + end + it 'returns the correct details path' do expect(result['issue-details-path']).to eq details_path end diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js index 266bbc1eadd..4b4a710df2d 100644 --- a/spec/javascripts/diffs/components/app_spec.js +++ b/spec/javascripts/diffs/components/app_spec.js @@ -10,6 +10,7 @@ import CompareVersions from '~/diffs/components/compare_versions.vue'; import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import TreeList from '~/diffs/components/tree_list.vue'; +import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '~/diffs/constants'; import createDiffsStore from '../create_diffs_store'; import diffsMockData from '../mock_data/merge_request_diffs'; @@ -41,7 +42,6 @@ describe('diffs/components/app', () => { changesEmptyStateIllustration: '', dismissEndpoint: '', showSuggestPopover: true, - useSingleDiffStyle: false, ...props, }, store, @@ -53,6 +53,12 @@ describe('diffs/components/app', () => { }); } + function getOppositeViewType(currentViewType) { + return currentViewType === INLINE_DIFF_VIEW_TYPE + ? PARALLEL_DIFF_VIEW_TYPE + : INLINE_DIFF_VIEW_TYPE; + } + beforeEach(() => { // setup globals (needed for component to mount :/) window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']); @@ -82,9 +88,146 @@ describe('diffs/components/app', () => { spyOn(wrapper.vm, 'startRenderDiffsQueue'); spyOn(wrapper.vm, 'unwatchDiscussions'); store.state.diffs.retrievingBatches = true; + store.state.diffs.diffFiles = []; wrapper.vm.$nextTick(done); }); + describe('when the diff view type changes and it should load a single diff view style', () => { + const noLinesDiff = { + highlighted_diff_lines: [], + parallel_diff_lines: [], + }; + const parallelLinesDiff = { + highlighted_diff_lines: [], + parallel_diff_lines: ['line'], + }; + const inlineLinesDiff = { + highlighted_diff_lines: ['line'], + parallel_diff_lines: [], + }; + const fullDiff = { + highlighted_diff_lines: ['line'], + parallel_diff_lines: ['line'], + }; + + function expectFetchToOccur({ + vueInstance, + done = () => {}, + batch = false, + existingFiles = 1, + } = {}) { + vueInstance.$nextTick(() => { + expect(vueInstance.diffFiles.length).toEqual(existingFiles); + + if (!batch) { + expect(vueInstance.fetchDiffFiles).toHaveBeenCalled(); + expect(vueInstance.fetchDiffFilesBatch).not.toHaveBeenCalled(); + } else { + expect(vueInstance.fetchDiffFiles).not.toHaveBeenCalled(); + expect(vueInstance.fetchDiffFilesBatch).toHaveBeenCalled(); + } + + done(); + }); + } + + beforeEach(() => { + wrapper.vm.glFeatures.singleMrDiffView = true; + }); + + it('fetches diffs if it has none', done => { + wrapper.vm.isLatestVersion = () => false; + + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, batch: false, existingFiles: 0, done }); + }); + + it('fetches diffs if it has both view styles, but no lines in either', done => { + wrapper.vm.isLatestVersion = () => false; + + store.state.diffs.diffFiles.push(noLinesDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, done }); + }); + + it('fetches diffs if it only has inline view style', done => { + wrapper.vm.isLatestVersion = () => false; + + store.state.diffs.diffFiles.push(inlineLinesDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, done }); + }); + + it('fetches diffs if it only has parallel view style', done => { + wrapper.vm.isLatestVersion = () => false; + + store.state.diffs.diffFiles.push(parallelLinesDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, done }); + }); + + it('fetches batch diffs if it has none', done => { + wrapper.vm.glFeatures.diffsBatchLoad = true; + + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, existingFiles: 0, done }); + }); + + it('fetches batch diffs if it has both view styles, but no lines in either', done => { + wrapper.vm.glFeatures.diffsBatchLoad = true; + + store.state.diffs.diffFiles.push(noLinesDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done }); + }); + + it('fetches batch diffs if it only has inline view style', done => { + wrapper.vm.glFeatures.diffsBatchLoad = true; + + store.state.diffs.diffFiles.push(inlineLinesDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done }); + }); + + it('fetches batch diffs if it only has parallel view style', done => { + wrapper.vm.glFeatures.diffsBatchLoad = true; + + store.state.diffs.diffFiles.push(parallelLinesDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done }); + }); + + it('does not fetch diffs if it has already fetched both styles of diff', () => { + wrapper.vm.glFeatures.diffsBatchLoad = false; + + store.state.diffs.diffFiles.push(fullDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expect(wrapper.vm.diffFiles.length).toEqual(1); + expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled(); + expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled(); + }); + + it('does not fetch batch diffs if it has already fetched both styles of diff', () => { + wrapper.vm.glFeatures.diffsBatchLoad = true; + + store.state.diffs.diffFiles.push(fullDiff); + store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType); + + expect(wrapper.vm.diffFiles.length).toEqual(1); + expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled(); + expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled(); + }); + }); + it('calls fetchDiffFiles if diffsBatchLoad is not enabled', done => { wrapper.vm.glFeatures.diffsBatchLoad = false; wrapper.vm.fetchData(false); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index 98a5348c3bc..436d7338361 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -120,7 +120,7 @@ describe('DiffsStoreActions', () => { describe('fetchDiffFiles', () => { it('should fetch diff files', done => { - const endpoint = '/fetch/diff/files?w=1'; + const endpoint = '/fetch/diff/files?view=inline&w=1'; const mock = new MockAdapter(axios); const res = { diff_files: 1, merge_request_diffs: [] }; mock.onGet(endpoint).reply(200, res); @@ -128,7 +128,7 @@ describe('DiffsStoreActions', () => { testAction( fetchDiffFiles, {}, - { endpoint }, + { endpoint, diffFiles: [], showWhitespace: false, diffViewType: 'inline' }, [ { type: types.SET_LOADING, payload: true }, { type: types.SET_LOADING, payload: false }, diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 93dbf03e1ed..24405dcc796 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -52,7 +52,14 @@ describe('DiffsStoreMutations', () => { describe('SET_DIFF_DATA', () => { it('should set diff data type properly', () => { - const state = {}; + const state = { + diffFiles: [ + { + content_sha: diffFileMockData.content_sha, + file_hash: diffFileMockData.file_hash, + }, + ], + }; const diffMock = { diff_files: [diffFileMockData], }; @@ -62,9 +69,41 @@ describe('DiffsStoreMutations', () => { const firstLine = state.diffFiles[0].parallel_diff_lines[0]; expect(firstLine.right.text).toBeUndefined(); + expect(state.diffFiles.length).toEqual(1); expect(state.diffFiles[0].renderIt).toEqual(true); expect(state.diffFiles[0].collapsed).toEqual(false); }); + + describe('given diffsBatchLoad feature flag is enabled', () => { + beforeEach(() => { + gon.features = { diffsBatchLoad: true }; + }); + + afterEach(() => { + delete gon.features; + }); + + it('should not modify the existing state', () => { + const state = { + diffFiles: [ + { + content_sha: diffFileMockData.content_sha, + file_hash: diffFileMockData.file_hash, + highlighted_diff_lines: [], + }, + ], + }; + const diffMock = { + diff_files: [diffFileMockData], + }; + + mutations[types.SET_DIFF_DATA](state, diffMock); + + // If the batch load is enabled, there shouldn't be any processing + // done on the existing state object, so we shouldn't have this. + expect(state.diffFiles[0].parallel_diff_lines).toBeUndefined(); + }); + }); }); describe('SET_DIFFSET_DIFF_DATA_BATCH_DATA', () => { @@ -168,11 +207,17 @@ describe('DiffsStoreMutations', () => { it('should update the state with the given data for the given file hash', () => { const fileHash = 123; const state = { - diffFiles: [{}, { file_hash: fileHash, existing_field: 0 }], + diffFiles: [{}, { content_sha: 'abc', file_hash: fileHash, existing_field: 0 }], }; const data = { diff_files: [ - { file_hash: fileHash, extra_field: 1, existing_field: 1, viewer: { name: 'text' } }, + { + content_sha: 'abc', + file_hash: fileHash, + extra_field: 1, + existing_field: 1, + viewer: { name: 'text' }, + }, ], }; @@ -208,7 +253,7 @@ describe('DiffsStoreMutations', () => { discussions: [], }, right: { - line_code: 'ABC_1', + line_code: 'ABC_2', discussions: [], }, }, @@ -274,7 +319,7 @@ describe('DiffsStoreMutations', () => { discussions: [], }, right: { - line_code: 'ABC_1', + line_code: 'ABC_2', discussions: [], }, }, @@ -352,7 +397,7 @@ describe('DiffsStoreMutations', () => { discussions: [], }, right: { - line_code: 'ABC_1', + line_code: 'ABC_2', discussions: [], }, }, @@ -448,6 +493,7 @@ describe('DiffsStoreMutations', () => { discussions: [], }, ], + parallel_diff_lines: [], }, ], }; diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 65eb4c9d2a3..638b4510221 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -314,11 +314,29 @@ describe('DiffsStoreUtils', () => { }); describe('prepareDiffData', () => { + let mock; let preparedDiff; + let splitInlineDiff; + let splitParallelDiff; + let completedDiff; beforeEach(() => { - preparedDiff = { diff_files: [getDiffFileMock()] }; + mock = getDiffFileMock(); + preparedDiff = { diff_files: [mock] }; + splitInlineDiff = { + diff_files: [Object.assign({}, mock, { parallel_diff_lines: undefined })], + }; + splitParallelDiff = { + diff_files: [Object.assign({}, mock, { highlighted_diff_lines: undefined })], + }; + completedDiff = { + diff_files: [Object.assign({}, mock, { highlighted_diff_lines: undefined })], + }; + utils.prepareDiffData(preparedDiff); + utils.prepareDiffData(splitInlineDiff); + utils.prepareDiffData(splitParallelDiff); + utils.prepareDiffData(completedDiff, [mock]); }); it('sets the renderIt and collapsed attribute on files', () => { @@ -359,6 +377,19 @@ describe('DiffsStoreUtils', () => { expect(firstLine.line_code).toEqual(firstLine.right.line_code); }); + + it('guarantees an empty array for both diff styles', () => { + expect(splitInlineDiff.diff_files[0].parallel_diff_lines.length).toEqual(0); + expect(splitInlineDiff.diff_files[0].highlighted_diff_lines.length).toBeGreaterThan(0); + expect(splitParallelDiff.diff_files[0].parallel_diff_lines.length).toBeGreaterThan(0); + expect(splitParallelDiff.diff_files[0].highlighted_diff_lines.length).toEqual(0); + }); + + it('merges existing diff files with newly loaded diff files to ensure split diffs are eventually completed', () => { + expect(completedDiff.diff_files.length).toEqual(1); + expect(completedDiff.diff_files[0].parallel_diff_lines.length).toBeGreaterThan(0); + expect(completedDiff.diff_files[0].highlighted_diff_lines.length).toBeGreaterThan(0); + }); }); describe('isDiscussionApplicableToLine', () => { diff --git a/spec/lib/gitlab/background_migration/activate_prometheus_services_for_shared_cluster_applications_spec.rb b/spec/lib/gitlab/background_migration/activate_prometheus_services_for_shared_cluster_applications_spec.rb deleted file mode 100644 index 9d9281f4e98..00000000000 --- a/spec/lib/gitlab/background_migration/activate_prometheus_services_for_shared_cluster_applications_spec.rb +++ /dev/null @@ -1,75 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::BackgroundMigration::ActivatePrometheusServicesForSharedClusterApplications, :migration, schema: 2019_12_20_102807 do - include MigrationHelpers::PrometheusServiceHelpers - - let(:namespaces) { table(:namespaces) } - let(:projects) { table(:projects) } - let(:services) { table(:services) } - let(:namespace) { namespaces.create(name: 'user', path: 'user') } - let(:project) { projects.create(namespace_id: namespace.id) } - - let(:columns) do - %w(project_id active properties type template push_events - issues_events merge_requests_events tag_push_events - note_events category default wiki_page_events pipeline_events - confidential_issues_events commit_events job_events - confidential_note_events deployment_events) - end - - describe '#perform' do - it 'is idempotent' do - expect { subject.perform(project.id) }.to change { services.order(:id).map { |row| row.attributes } } - - expect { subject.perform(project.id) }.not_to change { services.order(:id).map { |row| row.attributes } } - end - - context 'non prometheus services' do - it 'does not change them' do - other_type = 'SomeOtherService' - services.create(service_params_for(project.id, active: true, type: other_type)) - - expect { subject.perform(project.id) }.not_to change { services.where(type: other_type).order(:id).map { |row| row.attributes } } - end - end - - context 'prometheus services are configured manually ' do - it 'does not change them' do - properties = '{"api_url":"http://test.dev","manual_configuration":"1"}' - services.create(service_params_for(project.id, properties: properties, active: false)) - - expect { subject.perform(project.id) }.not_to change { services.order(:id).map { |row| row.attributes } } - end - end - - context 'prometheus integration services do not exist' do - it 'creates missing services entries' do - subject.perform(project.id) - - rows = services.order(:id).map { |row| row.attributes.slice(*columns).symbolize_keys } - - expect([service_params_for(project.id, active: true)]).to eq rows - end - end - - context 'prometheus integration services exist' do - context 'in active state' do - it 'does not change them' do - services.create(service_params_for(project.id, active: true)) - - expect { subject.perform(project.id) }.not_to change { services.order(:id).map { |row| row.attributes } } - end - end - - context 'not in active state' do - it 'sets active attribute to true' do - service = services.create(service_params_for(project.id)) - - expect { subject.perform(project.id) }.to change { service.reload.active? }.from(false).to(true) - end - end - end - end -end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb index ac370433955..24d3beb35b9 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb @@ -76,45 +76,8 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do end end - context 'when pipeline triggered by legacy trigger' do - let(:user) { nil } - let(:trigger_request) do - build_stubbed(:ci_trigger_request, trigger: build_stubbed(:ci_trigger, owner: nil)) - end - - context 'when :use_legacy_pipeline_triggers feature flag is enabled' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: true) - step.perform! - end - - it 'allows legacy triggers to create a pipeline' do - expect(pipeline).to be_valid - end - - it 'does not break the chain' do - expect(step.break?).to eq false - end - end - - context 'when :use_legacy_pipeline_triggers feature flag is disabled' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: false) - step.perform! - end - - it 'prevents legacy triggers from creating a pipeline' do - expect(pipeline.errors.to_a).to include /Trigger token is invalid/ - end - - it 'breaks the pipeline builder chain' do - expect(step.break?).to eq true - end - end - end - - describe '#allowed_to_create?' do - subject { step.allowed_to_create? } + describe '#allowed_to_write_ref?' do + subject { step.send(:allowed_to_write_ref?) } context 'when user is a developer' do before do diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 07439880beb..ee4db935b58 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -578,3 +578,30 @@ zoom_meetings: sentry_issue: - issue design_versions: *version +epic: +- subscriptions +- award_emoji +- description_versions +- author +- assignee +- issues +- epic_issues +- milestone +- notes +- label_links +- labels +- todos +- metrics +- group +- parent +- children +- updated_by +- last_edited_by +- closed_by +- start_date_sourcing_milestone +- due_date_sourcing_milestone +- start_date_sourcing_epic +- due_date_sourcing_epic +- events +- resource_label_events +- user_mentions
\ No newline at end of file diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 4d12d05211b..56426bb8110 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -36,10 +36,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end context 'JSON' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: false) - end - it 'restores models based on JSON' do expect(@restored_project_json).to be_truthy end @@ -502,7 +498,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end it_behaves_like 'restores project successfully', - issues: 2, + issues: 3, labels: 2, label_with_priorities: 'A project label', milestones: 2, @@ -515,7 +511,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it 'restores issue states' do expect(project.issues.with_state(:closed).count).to eq(1) - expect(project.issues.with_state(:opened).count).to eq(1) + expect(project.issues.with_state(:opened).count).to eq(2) end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index e5014eeca09..c921e7cadde 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -766,3 +766,33 @@ ContainerExpirationPolicy: - older_than - keep_n - enabled +Epic: + - id + - milestone_id + - group_id + - author_id + - assignee_id + - iid + - updated_by_id + - last_edited_by_id + - lock_version + - start_date + - end_date + - last_edited_at + - created_at + - updated_at + - title + - description + - start_date_sourcing_milestone_id + - due_date_sourcing_milestone_id + - start_date_fixed + - due_date_fixed + - start_date_is_fixed + - due_date_is_fixed + - closed_by_id + - closed_at + - parent_id + - relative_position + - state_id + - start_date_sourcing_epic_id + - due_date_sourcing_epic_id diff --git a/spec/migrations/20191204114127_delete_legacy_triggers_spec.rb b/spec/migrations/20191204114127_delete_legacy_triggers_spec.rb new file mode 100644 index 00000000000..c2660d699ca --- /dev/null +++ b/spec/migrations/20191204114127_delete_legacy_triggers_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20191204114127_delete_legacy_triggers.rb') + +describe DeleteLegacyTriggers, :migration, schema: 2019_11_25_140458 do + let(:ci_trigger_table) { table(:ci_triggers) } + let(:user) { table(:users).create!(name: 'test', email: 'test@example.com', projects_limit: 1) } + + before do + @trigger_with_user = ci_trigger_table.create!(owner_id: user.id) + ci_trigger_table.create!(owner_id: nil) + ci_trigger_table.create!(owner_id: nil) + end + + it 'removes legacy triggers which has null owner_id' do + expect do + migrate! + end.to change(ci_trigger_table, :count).by(-2) + + expect(ci_trigger_table.all).to eq([@trigger_with_user]) + end +end diff --git a/spec/migrations/patch_prometheus_services_for_shared_cluster_applications_spec.rb b/spec/migrations/patch_prometheus_services_for_shared_cluster_applications_spec.rb deleted file mode 100644 index 74f6d56a2eb..00000000000 --- a/spec/migrations/patch_prometheus_services_for_shared_cluster_applications_spec.rb +++ /dev/null @@ -1,134 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20191220102807_patch_prometheus_services_for_shared_cluster_applications.rb') - -describe PatchPrometheusServicesForSharedClusterApplications, :migration, :sidekiq do - include MigrationHelpers::PrometheusServiceHelpers - - let(:namespaces) { table(:namespaces) } - let(:projects) { table(:projects) } - let(:services) { table(:services) } - let(:clusters) { table(:clusters) } - let(:cluster_groups) { table(:cluster_groups) } - let(:clusters_applications_prometheus) { table(:clusters_applications_prometheus) } - let(:namespace) { namespaces.create!(name: 'gitlab', path: 'gitlab-org') } - - let(:application_statuses) do - { - errored: -1, - installed: 3, - updated: 5 - } - end - - let(:cluster_types) do - { - instance_type: 1, - group_type: 2 - } - end - - describe '#up' do - let!(:project_with_missing_service) { projects.create!(name: 'gitlab', path: 'gitlab-ce', namespace_id: namespace.id) } - let(:project_with_inactive_service) { projects.create!(name: 'gitlab', path: 'gitlab-ee', namespace_id: namespace.id) } - let(:project_with_active_service) { projects.create!(name: 'gitlab', path: 'gitlab-ee', namespace_id: namespace.id) } - let(:project_with_manual_active_service) { projects.create!(name: 'gitlab', path: 'gitlab-ee', namespace_id: namespace.id) } - let(:project_with_manual_inactive_service) { projects.create!(name: 'gitlab', path: 'gitlab-ee', namespace_id: namespace.id) } - let(:project_with_active_not_prometheus_service) { projects.create!(name: 'gitlab', path: 'gitlab-ee', namespace_id: namespace.id) } - let(:project_with_inactive_not_prometheus_service) { projects.create!(name: 'gitlab', path: 'gitlab-ee', namespace_id: namespace.id) } - - before do - services.create(service_params_for(project_with_inactive_service.id, active: false)) - services.create(service_params_for(project_with_active_service.id, active: true)) - services.create(service_params_for(project_with_active_not_prometheus_service.id, active: true, type: 'other')) - services.create(service_params_for(project_with_inactive_not_prometheus_service.id, active: false, type: 'other')) - services.create(service_params_for(project_with_manual_inactive_service.id, active: false, properties: { some: 'data' }.to_json)) - services.create(service_params_for(project_with_manual_active_service.id, active: true, properties: { some: 'data' }.to_json)) - end - - shared_examples 'patch prometheus services post migration' do - context 'prometheus application is installed on the cluster' do - it 'schedules a background migration' do - clusters_applications_prometheus.create(cluster_id: cluster.id, status: application_statuses[:installed], version: '123') - - Sidekiq::Testing.fake! do - Timecop.freeze do - background_migrations = [["ActivatePrometheusServicesForSharedClusterApplications", project_with_missing_service.id], - ["ActivatePrometheusServicesForSharedClusterApplications", project_with_inactive_service.id], - ["ActivatePrometheusServicesForSharedClusterApplications", project_with_active_not_prometheus_service.id], - ["ActivatePrometheusServicesForSharedClusterApplications", project_with_inactive_not_prometheus_service.id]] - - migrate! - - enqueued_migrations = BackgroundMigrationWorker.jobs.map { |job| job['args'] } - expect(enqueued_migrations).to match_array(background_migrations) - end - end - end - end - - context 'prometheus application was recently updated on the cluster' do - it 'schedules a background migration' do - clusters_applications_prometheus.create(cluster_id: cluster.id, status: application_statuses[:updated], version: '123') - - Sidekiq::Testing.fake! do - Timecop.freeze do - background_migrations = [["ActivatePrometheusServicesForSharedClusterApplications", project_with_missing_service.id], - ["ActivatePrometheusServicesForSharedClusterApplications", project_with_inactive_service.id], - ["ActivatePrometheusServicesForSharedClusterApplications", project_with_active_not_prometheus_service.id], - ["ActivatePrometheusServicesForSharedClusterApplications", project_with_inactive_not_prometheus_service.id]] - - migrate! - - enqueued_migrations = BackgroundMigrationWorker.jobs.map { |job| job['args'] } - expect(enqueued_migrations).to match_array(background_migrations) - end - end - end - end - - context 'prometheus application failed to install on the cluster' do - it 'does not schedule a background migration' do - clusters_applications_prometheus.create(cluster_id: cluster.id, status: application_statuses[:errored], version: '123') - - Sidekiq::Testing.fake! do - Timecop.freeze do - migrate! - - expect(BackgroundMigrationWorker.jobs.size).to eq 0 - end - end - end - end - - context 'prometheus application is NOT installed on the cluster' do - it 'does not schedule a background migration' do - Sidekiq::Testing.fake! do - Timecop.freeze do - migrate! - - expect(BackgroundMigrationWorker.jobs.size).to eq 0 - end - end - end - end - end - - context 'Cluster is group_type' do - let(:cluster) { clusters.create(name: 'cluster', cluster_type: cluster_types[:group_type]) } - - before do - cluster_groups.create(group_id: namespace.id, cluster_id: cluster.id) - end - - it_behaves_like 'patch prometheus services post migration' - end - - context 'Cluster is instance_type' do - let(:cluster) { clusters.create(name: 'cluster', cluster_type: cluster_types[:instance_type]) } - - it_behaves_like 'patch prometheus services post migration' - end - end -end diff --git a/spec/models/ci/trigger_spec.rb b/spec/models/ci/trigger_spec.rb index 5b5d6f51b33..5b0815f8156 100644 --- a/spec/models/ci/trigger_spec.rb +++ b/spec/models/ci/trigger_spec.rb @@ -11,6 +11,10 @@ describe Ci::Trigger do it { is_expected.to have_many(:trigger_requests) } end + describe 'validations' do + it { is_expected.to validate_presence_of(:owner) } + end + describe 'before_validation' do it 'sets an random token if none provided' do trigger = create(:ci_trigger_without_token, project: project) @@ -35,63 +39,22 @@ describe Ci::Trigger do end end - describe '#legacy?' do - let(:trigger) { create(:ci_trigger, owner: owner, project: project) } - - subject { trigger } - - context 'when owner is blank' do - let(:owner) { nil } - - it { is_expected.to be_legacy } - end - - context 'when owner is set' do - let(:owner) { create(:user) } - - it { is_expected.not_to be_legacy } - end - end - describe '#can_access_project?' do let(:owner) { create(:user) } let(:trigger) { create(:ci_trigger, owner: owner, project: project) } - context 'when owner is blank' do + subject { trigger.can_access_project? } + + context 'and is member of the project' do before do - stub_feature_flags(use_legacy_pipeline_triggers: false) - trigger.update_attribute(:owner, nil) + project.add_developer(owner) end - subject { trigger.can_access_project? } - - it { is_expected.to eq(false) } - - context 'when :use_legacy_pipeline_triggers feature flag is enabled' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: true) - end - - subject { trigger.can_access_project? } - - it { is_expected.to eq(true) } - end + it { is_expected.to eq(true) } end - context 'when owner is set' do - subject { trigger.can_access_project? } - - context 'and is member of the project' do - before do - project.add_developer(owner) - end - - it { is_expected.to eq(true) } - end - - context 'and is not member of the project' do - it { is_expected.to eq(false) } - end + context 'and is not member of the project' do + it { is_expected.to eq(false) } end end end diff --git a/spec/policies/ci/trigger_policy_spec.rb b/spec/policies/ci/trigger_policy_spec.rb index e936277a391..28e5a2b2cd6 100644 --- a/spec/policies/ci/trigger_policy_spec.rb +++ b/spec/policies/ci/trigger_policy_spec.rb @@ -10,60 +10,6 @@ describe Ci::TriggerPolicy do subject { described_class.new(user, trigger) } describe '#rules' do - context 'when owner is undefined' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: false) - trigger.update_attribute(:owner, nil) - end - - context 'when user is maintainer of the project' do - before do - project.add_maintainer(user) - end - - it { is_expected.to be_allowed(:manage_trigger) } - it { is_expected.not_to be_allowed(:admin_trigger) } - end - - context 'when user is developer of the project' do - before do - project.add_developer(user) - end - - it { is_expected.not_to be_allowed(:manage_trigger) } - it { is_expected.not_to be_allowed(:admin_trigger) } - end - - context 'when :use_legacy_pipeline_triggers feature flag is enabled' do - before do - stub_feature_flags(use_legacy_pipeline_triggers: true) - end - - context 'when user is maintainer of the project' do - before do - project.add_maintainer(user) - end - - it { is_expected.to be_allowed(:manage_trigger) } - it { is_expected.to be_allowed(:admin_trigger) } - end - - context 'when user is developer of the project' do - before do - project.add_developer(user) - end - - it { is_expected.not_to be_allowed(:manage_trigger) } - it { is_expected.not_to be_allowed(:admin_trigger) } - end - - context 'when user is not member of the project' do - it { is_expected.not_to be_allowed(:manage_trigger) } - it { is_expected.not_to be_allowed(:admin_trigger) } - end - end - end - context 'when owner is an user' do before do trigger.update!(owner: user) diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index fd1104fa978..d54d112cd9f 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -87,22 +87,6 @@ describe API::Triggers do expect(pipeline.variables.map { |v| { v.key => v.value } }.last).to eq(variables) end end - - context 'when legacy trigger' do - before do - trigger.update(owner: nil) - end - - it 'creates pipeline' do - post api("/projects/#{project.id}/trigger/pipeline"), params: options.merge(ref: 'master') - - expect(response).to have_gitlab_http_status(201) - expect(json_response).to include('id' => pipeline.id) - pipeline.builds.reload - expect(pipeline.builds.pending.size).to eq(2) - expect(pipeline.builds.size).to eq(5) - end - end end context 'when triggering a pipeline from a trigger token' do diff --git a/spec/routing/admin_routing_spec.rb b/spec/routing/admin_routing_spec.rb index a82bdfe3ce8..93b2c19c74a 100644 --- a/spec/routing/admin_routing_spec.rb +++ b/spec/routing/admin_routing_spec.rb @@ -161,3 +161,17 @@ describe Admin::GroupsController, "routing" do expect(get("/admin/groups/#{name}/edit")).to route_to('admin/groups#edit', id: name) end end + +describe Admin::SessionsController, "routing" do + it "to #new" do + expect(get("/admin/session/new")).to route_to('admin/sessions#new') + end + + it "to #create" do + expect(post("/admin/session")).to route_to('admin/sessions#create') + end + + it "to #destroy" do + expect(post("/admin/session/destroy")).to route_to('admin/sessions#destroy') + end +end diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 6f67cdb1222..ff002469e3c 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -256,10 +256,8 @@ describe "Authentication", "routing" do expect(post("/users/sign_in")).to route_to('sessions#create') end - # sign_out with GET instead of DELETE facilitates ad-hoc single-sign-out processes - # (https://gitlab.com/gitlab-org/gitlab-foss/issues/39708) - it "GET /users/sign_out" do - expect(get("/users/sign_out")).to route_to('sessions#destroy') + it "POST /users/sign_out" do + expect(post("/users/sign_out")).to route_to('sessions#destroy') end it "POST /users/password" do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 2876f7b5f59..dc67efe0fbe 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1123,21 +1123,6 @@ describe Ci::CreatePipelineService do it_behaves_like 'when ref is protected' end - context 'when ref is not protected' do - context 'when trigger belongs to no one' do - let(:user) {} - let(:trigger) { create(:ci_trigger, owner: nil) } - let(:trigger_request) { create(:ci_trigger_request, trigger: trigger) } - let(:pipeline) { execute_service(trigger_request: trigger_request) } - - it 'creates an unprotected pipeline' do - expect(pipeline).to be_persisted - expect(pipeline).not_to be_protected - expect(Ci::Pipeline.count).to eq(1) - end - end - end - context 'when pipeline is running for a tag' do before do config = YAML.dump(test: { script: 'test', only: ['branches'] }, diff --git a/spec/support/migrations_helpers/prometheus_service_helpers.rb b/spec/support/migrations_helpers/prometheus_service_helpers.rb deleted file mode 100644 index 88f2f71ee1e..00000000000 --- a/spec/support/migrations_helpers/prometheus_service_helpers.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -module MigrationHelpers - module PrometheusServiceHelpers - def service_params_for(project_id, params = {}) - { - project_id: project_id, - active: false, - properties: '{}', - type: 'PrometheusService', - template: false, - push_events: true, - issues_events: true, - merge_requests_events: true, - tag_push_events: true, - note_events: true, - category: 'monitoring', - default: false, - wiki_page_events: true, - pipeline_events: true, - confidential_issues_events: true, - commit_events: true, - job_events: true, - confidential_note_events: true, - deployment_events: false - }.merge(params) - end - - def row_attributes(entity) - entity.attributes.with_indifferent_access.tap do |hash| - hash.merge!(hash.slice(:created_at, :updated_at).transform_values { |v| v.to_s(:db) }) - end - end - end -end diff --git a/spec/support/shared_examples/merge_requests_rendering_a_single_diff_version.rb b/spec/support/shared_examples/merge_requests_rendering_a_single_diff_version.rb deleted file mode 100644 index 18d025a4b07..00000000000 --- a/spec/support/shared_examples/merge_requests_rendering_a_single_diff_version.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -# This pending test can be removed when `single_mr_diff_view` is enabled by default -# disabling the feature flag above is then not needed anymore. -RSpec.shared_examples 'rendering a single diff version' do |attribute| - before do - stub_feature_flags(diffs_batch_load: false) - end - - pending 'allows editing diff settings single_mr_diff_view is enabled' do - project = create(:project, :repository) - user = project.creator - merge_request = create(:merge_request, source_project: project) - stub_feature_flags(single_mr_diff_view: true) - sign_in(user) - - visit(diffs_project_merge_request_path(project, merge_request)) - - expect(page).to have_selector('.js-show-diff-settings') - end -end diff --git a/yarn.lock b/yarn.lock index 5289e563189..aa18e5d0096 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5640,6 +5640,11 @@ immediate@~3.0.5: resolved "https://registry.yarnpkg.com/immediate/-/immediate-3.0.6.tgz#9db1dbd0faf8de6fbe0f5dd5e56bb606280de69b" integrity sha1-nbHb0Pr43m++D13V5Wu2BigN5ps= +immer@^5.2.1: + version "5.2.1" + resolved "https://registry.yarnpkg.com/immer/-/immer-5.2.1.tgz#7d4f74c242178e87151d595f48db1b5c51580485" + integrity sha512-9U1GEbJuH6nVoyuFRgTQDGMzcBuNBPfXM3M7Pp/sdmYKTKYOBUZGgeUb9H57GfLK/xC1DMLarWX2FrhMBfUJ8g== + import-fresh@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/import-fresh/-/import-fresh-2.0.0.tgz#d81355c15612d386c61f9ddd3922d4304822a546" |
