diff options
100 files changed, 1040 insertions, 111 deletions
diff --git a/app/assets/images/experienced.svg b/app/assets/images/experienced.svg new file mode 100644 index 00000000000..1c93cfcf1ee --- /dev/null +++ b/app/assets/images/experienced.svg @@ -0,0 +1 @@ +<svg height="82" viewBox="0 0 78 82" width="78" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><g fill-rule="nonzero"><path d="m2.12 42c-.08 1-.12 2-.12 3 0 20.43 16.57 37 37 37s37-16.57 37-37c0-1-.04-2-.12-3-1.53 19.03-17.46 34-36.88 34s-35.35-14.97-36.88-34z" fill="#000" fill-opacity=".03"/><path d="m39 78c-21.54 0-39-17.46-39-39s17.46-39 39-39 39 17.46 39 39-17.46 39-39 39zm0-4c19.33 0 35-15.67 35-35s-15.67-35-35-35-35 15.67-35 35 15.67 35 35 35z" fill="#eee"/><path d="m44 31-2.5-3-2.5 3-2.5-3-2.5 3-2.5-3-2.5 3h-2.72c2.65-4.2 7.36-7 12.72-7s10.07 2.8 12.72 7h-2.72l-2.5-3z" fill="#e1dbf2"/><path d="m39 57c-9.4 0-17-7.6-17-17s7.6-17 17-17 17 7.6 17 17-7.6 17-17 17zm0-4c7.18 0 13-5.82 13-13s-5.82-13-13-13-13 5.82-13 13 5.82 13 13 13z" fill="#e1dbf2"/></g><g fill="#e2ba3e" stroke="#fff" transform="translate(14 18)"><path d="m8.5 12.75-4.99617464 2.6266445.95418445-5.56332227-4.0419902-3.93996668 5.58589307-.81167778 2.49808732-5.06167777 2.4980873 5.06167777 5.5858931.81167778-4.0419902 3.93996668.9541844 5.56332227z"/><path d="m24.5 12.75-4.9961746 2.6266445.9541844-5.56332227-4.0419902-3.93996668 5.5858931-.81167778 2.4980873-5.06167777 2.4980873 5.06167777 5.5858931.81167778-4.0419902 3.93996668.9541844 5.56332227z"/><path d="m40.5 12.75-4.9961746 2.6266445.9541844-5.56332227-4.0419902-3.93996668 5.5858931-.81167778 2.4980873-5.06167777 2.4980873 5.06167777 5.5858931.81167778-4.0419902 3.93996668.9541844 5.56332227z"/></g><path d="m35 45h8c0 2.2-1.8 4-4 4s-4-1.8-4-4zm-1.5-2c-.83 0-1.5-.67-1.5-1.5s.67-1.5 1.5-1.5 1.5.67 1.5 1.5-.67 1.5-1.5 1.5zm11 0c-.83 0-1.5-.67-1.5-1.5s.67-1.5 1.5-1.5 1.5.67 1.5 1.5-.67 1.5-1.5 1.5z" fill="#6b4fbb" fill-rule="nonzero"/></g></svg>
\ No newline at end of file diff --git a/app/assets/images/learn-gitlab-avatar.jpg b/app/assets/images/learn-gitlab-avatar.jpg Binary files differnew file mode 100644 index 00000000000..65ec29444cb --- /dev/null +++ b/app/assets/images/learn-gitlab-avatar.jpg diff --git a/app/assets/images/novice.svg b/app/assets/images/novice.svg new file mode 100644 index 00000000000..c6744fa4550 --- /dev/null +++ b/app/assets/images/novice.svg @@ -0,0 +1 @@ +<svg width="78" height="82" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path d="M2.12 42C2.04 43 2 44 2 45c0 20.43 16.57 37 37 37s37-16.57 37-37c0-1-.04-2-.12-3C74.35 61.03 58.42 76 39 76S3.65 61.03 2.12 42z" fill-opacity=".03" fill="#000" fill-rule="nonzero"/><path d="M39 78C17.46 78 0 60.54 0 39S17.46 0 39 0s39 17.46 39 39-17.46 39-39 39zm0-4c19.33 0 35-15.67 35-35S58.33 4 39 4 4 19.67 4 39s15.67 35 35 35z" fill="#EEE" fill-rule="nonzero"/><path d="M44 31l-2.5-3-2.5 3-2.5-3-2.5 3-2.5-3-2.5 3h-2.72c2.65-4.2 7.36-7 12.72-7s10.07 2.8 12.72 7H49l-2.5-3-2.5 3z" fill="#E1DBF2" fill-rule="nonzero"/><path d="M39 57c-9.4 0-17-7.6-17-17s7.6-17 17-17 17 7.6 17 17-7.6 17-17 17zm0-4c7.18 0 13-5.82 13-13s-5.82-13-13-13-13 5.82-13 13 5.82 13 13 13z" fill="#E1DBF2" fill-rule="nonzero"/><g transform="translate(20 13)"><path d="M5.731 15.24V8.736H33.75v6.504c0 4.851-6.272 8.784-14.01 8.784-7.737 0-14.009-3.933-14.009-8.784z" fill="#0B2630"/><path d="M.75 7.662L18.824.182a2.4 2.4 0 0 1 1.835 0l18.072 7.48a1.2 1.2 0 0 1 0 2.218l-18.072 7.48a2.4 2.4 0 0 1-1.835 0L.75 9.88a1.2 1.2 0 0 1 0-2.218z" fill="#29424E"/><path d="M19.295 9.771a1.194 1.194 0 0 1-.66-1.557c.248-.612.948-.907 1.562-.659l11.516 4.657v11.766c0 .66-.538 1.195-1.2 1.195-.663 0-1.2-.535-1.2-1.195V13.822l-10.018-4.05z" fill="#FFCF00" fill-rule="nonzero"/><path d="M32.613 23.373v3.807h-4.2v-3.807c0-.711.353-1.34.894-1.72h2.411c.541.38.895 1.009.895 1.72z" fill="#FCA326"/><path fill="#FFCF00" d="M28.413 23.592H32.613V27.18H28.413z"/></g><path d="M35 45h8c0 2.2-1.8 4-4 4s-4-1.8-4-4zm-1.5-2c-.83 0-1.5-.67-1.5-1.5s.67-1.5 1.5-1.5 1.5.67 1.5 1.5-.67 1.5-1.5 1.5zm11 0c-.83 0-1.5-.67-1.5-1.5s.67-1.5 1.5-1.5 1.5.67 1.5 1.5-.67 1.5-1.5 1.5z" fill="#6B4FBB" fill-rule="nonzero"/></g></svg>
\ No newline at end of file diff --git a/app/assets/javascripts/alert_management/components/alert_details.vue b/app/assets/javascripts/alert_management/components/alert_details.vue index 6abdbc6be98..50410380332 100644 --- a/app/assets/javascripts/alert_management/components/alert_details.vue +++ b/app/assets/javascripts/alert_management/components/alert_details.vue @@ -15,11 +15,13 @@ import { s__ } from '~/locale'; import query from '../graphql/queries/details.query.graphql'; import { fetchPolicies } from '~/lib/graphql'; import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue'; +import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { ALERTS_SEVERITY_LABELS, trackAlertsDetailsViewsOptions } from '../constants'; import createIssueQuery from '../graphql/mutations/create_issue_from_alert.graphql'; import { visitUrl, joinPaths } from '~/lib/utils/url_utility'; import Tracking from '~/tracking'; import { toggleContainerClasses } from '~/lib/utils/dom_utils'; +import SystemNote from './system_notes/system_note.vue'; import AlertSidebar from './alert_sidebar.vue'; const containerEl = document.querySelector('.page-with-contextual-sidebar'); @@ -47,7 +49,9 @@ export default { GlTable, TimeAgoTooltip, AlertSidebar, + SystemNote, }, + mixins: [glFeatureFlagsMixin()], props: { alertId: { type: String, @@ -159,6 +163,9 @@ export default { const { category, action } = trackAlertsDetailsViewsOptions; Tracking.event(category, action); }, + alertRefresh() { + this.$apollo.queries.alert.refetch(); + }, }, }; </script> @@ -287,6 +294,13 @@ export default { </div> <div class="gl-pl-2" data-testid="service">{{ alert.service }}</div> </div> + <template v-if="glFeatures.alertAssignee"> + <div v-if="alert.notes" class="issuable-discussion"> + <ul class="notes main-notes-list timeline"> + <system-note v-for="note in alert.notes.nodes" :key="note.id" :note="note" /> + </ul> + </div> + </template> </gl-tab> <gl-tab data-testid="fullDetailsTab" :title="$options.i18n.fullAlertDetailsTitle"> <gl-table @@ -309,6 +323,7 @@ export default { :project-path="projectPath" :alert="alert" :sidebar-collapsed="sidebarCollapsed" + @alert-refresh="alertRefresh" @toggle-sidebar="toggleSidebar" @alert-sidebar-error="handleAlertSidebarError" /> diff --git a/app/assets/javascripts/alert_management/components/alert_sidebar.vue b/app/assets/javascripts/alert_management/components/alert_sidebar.vue index e1fd3e7fd24..38a250abc30 100644 --- a/app/assets/javascripts/alert_management/components/alert_sidebar.vue +++ b/app/assets/javascripts/alert_management/components/alert_sidebar.vue @@ -53,6 +53,7 @@ export default { v-if="glFeatures.alertAssignee" :project-path="projectPath" :alert="alert" + @alert-refresh="$emit('alert-refresh')" @toggle-sidebar="$emit('toggle-sidebar')" @alert-sidebar-error="$emit('alert-sidebar-error', $event)" /> diff --git a/app/assets/javascripts/alert_management/components/sidebar/sidebar_assignees.vue b/app/assets/javascripts/alert_management/components/sidebar/sidebar_assignees.vue index 358e0974026..bd9bf32df12 100644 --- a/app/assets/javascripts/alert_management/components/sidebar/sidebar_assignees.vue +++ b/app/assets/javascripts/alert_management/components/sidebar/sidebar_assignees.vue @@ -141,6 +141,7 @@ export default { }) .then(() => { this.hideDropdown(); + this.$emit('alert-refresh'); }) .catch(() => { this.$emit('alert-sidebar-error', this.$options.UPDATE_ALERT_ASSIGNEES_ERROR); diff --git a/app/assets/javascripts/alert_management/components/system_notes/system_note.vue b/app/assets/javascripts/alert_management/components/system_notes/system_note.vue new file mode 100644 index 00000000000..e8d07daea46 --- /dev/null +++ b/app/assets/javascripts/alert_management/components/system_notes/system_note.vue @@ -0,0 +1,39 @@ +<script> +import NoteHeader from '~/notes/components/note_header.vue'; +import { spriteIcon } from '~/lib/utils/common_utils'; + +export default { + components: { + NoteHeader, + }, + props: { + note: { + type: Object, + required: true, + }, + }, + computed: { + noteAnchorId() { + return `note_${this.note?.id?.split('/').pop()}`; + }, + iconHtml() { + return spriteIcon('user'); + }, + }, +}; +</script> + +<template> + <li :id="noteAnchorId" class="timeline-entry note system-note note-wrapper"> + <div class="timeline-entry-inner"> + <div class="timeline-icon" v-html="iconHtml"></div> + <div class="timeline-content"> + <div class="note-header"> + <note-header :author="note.author" :created-at="note.createdAt" :note-id="note.id"> + <span v-html="note.bodyHtml"></span> + </note-header> + </div> + </div> + </div> + </li> +</template> diff --git a/app/assets/javascripts/alert_management/graphql/fragments/alert_note.fragment.graphql b/app/assets/javascripts/alert_management/graphql/fragments/alert_note.fragment.graphql new file mode 100644 index 00000000000..c72300e9757 --- /dev/null +++ b/app/assets/javascripts/alert_management/graphql/fragments/alert_note.fragment.graphql @@ -0,0 +1,16 @@ +#import "~/graphql_shared/fragments/author.fragment.graphql" + +fragment AlertNote on Note { + id + author { + id + state + ...Author + } + body + bodyHtml + createdAt + discussion { + id + } +} diff --git a/app/assets/javascripts/alert_management/graphql/fragments/detailItem.fragment.graphql b/app/assets/javascripts/alert_management/graphql/fragments/detail_item.fragment.graphql index 95bc4d97516..cbe7e169be3 100644 --- a/app/assets/javascripts/alert_management/graphql/fragments/detailItem.fragment.graphql +++ b/app/assets/javascripts/alert_management/graphql/fragments/detail_item.fragment.graphql @@ -1,4 +1,5 @@ #import "./list_item.fragment.graphql" +#import "./alert_note.fragment.graphql" fragment AlertDetailItem on AlertManagementAlert { ...AlertListItem @@ -8,4 +9,9 @@ fragment AlertDetailItem on AlertManagementAlert { description updatedAt details + notes { + nodes { + ...AlertNote + } + } } diff --git a/app/assets/javascripts/alert_management/graphql/queries/details.query.graphql b/app/assets/javascripts/alert_management/graphql/queries/details.query.graphql index 7c77715fad2..c02b8accdd1 100644 --- a/app/assets/javascripts/alert_management/graphql/queries/details.query.graphql +++ b/app/assets/javascripts/alert_management/graphql/queries/details.query.graphql @@ -1,4 +1,4 @@ -#import "../fragments/detailItem.fragment.graphql" +#import "../fragments/detail_item.fragment.graphql" query alertDetails($fullPath: ID!, $alertId: String) { project(fullPath: $fullPath) { diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 8c9fe6349d6..d93fdcf0d59 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -135,7 +135,7 @@ hr { text-overflow: ellipsis; white-space: nowrap; - > div:not(.block), + > div:not(.block):not(.select2-display-none), .str-truncated { display: inline; } diff --git a/app/assets/stylesheets/framework/selects.scss b/app/assets/stylesheets/framework/selects.scss index 9adc149a8c1..c8c7d275339 100644 --- a/app/assets/stylesheets/framework/selects.scss +++ b/app/assets/stylesheets/framework/selects.scss @@ -58,6 +58,42 @@ } } + // Essentially we’re doing @include form-control-focus here (from + // bootstrap/scss/mixins/_forms.scss), except that the bootstrap mixin adds a + // `&:focus` selector and we’re never actually focusing the .select2-choice + // link nor the .select2-container, the Select2 library focuses an off-screen + // .select2-focusser element instead. + &.select2-container-active:not(.select2-dropdown-open) { + .select2-choice { + color: $input-focus-color; + background-color: $input-focus-bg; + border-color: $input-focus-border-color; + outline: 0; + } + + // Reusable focus “glow” box-shadow + @mixin form-control-focus-glow { + @if $enable-shadows { + box-shadow: $input-box-shadow, $input-focus-box-shadow; + } @else { + box-shadow: $input-focus-box-shadow; + } + } + + // Apply the focus “glow” shadow to the .select2-container if it also has + // the .block-truncated class as that applies an overflow: hidden, thereby + // hiding the glow of the nested .select2-choice element. + &.block-truncated { + @include form-control-focus-glow; + } + + // Apply the glow directly to the .select2-choice link if we’re not + // block-truncating the container. + &:not(.block-truncated) .select2-choice { + @include form-control-focus-glow; + } + } + &.is-invalid { ~ .invalid-feedback { display: block; diff --git a/app/assets/stylesheets/pages/experience_level.scss b/app/assets/stylesheets/pages/experience_level.scss new file mode 100644 index 00000000000..e57ad6321a5 --- /dev/null +++ b/app/assets/stylesheets/pages/experience_level.scss @@ -0,0 +1,29 @@ +.signup-page[data-page^='registrations:experience_levels'] { + $card-shadow-color: rgba($black, 0.2); + + .page-wrap { + background-color: $white; + } + + .card-deck { + max-width: 828px; + } + + .card { + transition: box-shadow 0.3s ease-in-out; + } + + .card:hover { + box-shadow: 0 $gl-spacing-scale-3 $gl-spacing-scale-5 $card-shadow-color; + } + + @media (min-width: $breakpoint-sm) { + .card-deck .card { + margin: 0 $gl-spacing-scale-3; + } + } + + .stretched-link:hover { + text-decoration: none; + } +} diff --git a/app/assets/stylesheets/utilities.scss b/app/assets/stylesheets/utilities.scss index 803eac52317..176d64272c2 100644 --- a/app/assets/stylesheets/utilities.scss +++ b/app/assets/stylesheets/utilities.scss @@ -89,3 +89,14 @@ .gl-text-purple { color: $purple; } .gl-bg-purple-light { background-color: $purple-light; } + +// move this to GitLab UI once onboarding experiment is considered a success +.gl-py-8 { + padding-top: $gl-spacing-scale-8; + padding-bottom: $gl-spacing-scale-8; +} + +// move this to GitLab UI once onboarding experiment is considered a success +.gl-pl-7 { + padding-left: $gl-spacing-scale-7; +} diff --git a/app/graphql/types/alert_management/alert_type.rb b/app/graphql/types/alert_management/alert_type.rb index 48c604f953e..cb55a9323b1 100644 --- a/app/graphql/types/alert_management/alert_type.rb +++ b/app/graphql/types/alert_management/alert_type.rb @@ -6,6 +6,8 @@ module Types graphql_name 'AlertManagementAlert' description "Describes an alert from the project's Alert Management" + implements(Types::Notes::NoteableType) + authorize :read_alert_management_alert field :iid, diff --git a/app/graphql/types/notes/noteable_type.rb b/app/graphql/types/notes/noteable_type.rb index 187c9109f8c..3a16d54f9cd 100644 --- a/app/graphql/types/notes/noteable_type.rb +++ b/app/graphql/types/notes/noteable_type.rb @@ -19,6 +19,8 @@ module Types Types::SnippetType when ::DesignManagement::Design Types::DesignManagement::DesignType + when ::AlertManagement::Alert + Types::AlertManagement::AlertType else raise "Unknown GraphQL type for #{object}" end diff --git a/app/models/alert_management/alert.rb b/app/models/alert_management/alert.rb index 564ef024c21..8dade750385 100644 --- a/app/models/alert_management/alert.rb +++ b/app/models/alert_management/alert.rb @@ -8,6 +8,7 @@ module AlertManagement include AtomicInternalId include ShaAttribute include Sortable + include Noteable include Gitlab::SQL::Pattern STATUSES = { @@ -30,6 +31,9 @@ module AlertManagement has_many :alert_assignees, inverse_of: :alert has_many :assignees, through: :alert_assignees + has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + has_many :user_mentions, class_name: 'AlertManagement::AlertUserMention', foreign_key: :alert_management_alert_id + has_internal_id :iid, scope: :project, init: ->(s) { s.project.alert_management_alerts.maximum(:iid) } sha_attribute :fingerprint diff --git a/app/models/alert_management/alert_user_mention.rb b/app/models/alert_management/alert_user_mention.rb new file mode 100644 index 00000000000..d36aa80ee05 --- /dev/null +++ b/app/models/alert_management/alert_user_mention.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module AlertManagement + class AlertUserMention < UserMention + belongs_to :alert_management_alert, class_name: '::AlertManagement::Alert' + belongs_to :note + end +end diff --git a/app/models/note.rb b/app/models/note.rb index 122f9695d63..6b6a7c50b00 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -274,6 +274,10 @@ class Note < ApplicationRecord noteable_type == "Snippet" end + def for_alert_mangement_alert? + noteable_type == 'AlertManagement::Alert' + end + def for_personal_snippet? noteable.is_a?(PersonalSnippet) end @@ -396,7 +400,13 @@ class Note < ApplicationRecord end def noteable_ability_name - for_snippet? ? 'snippet' : noteable_type.demodulize.underscore + if for_snippet? + 'snippet' + elsif for_alert_mangement_alert? + 'alert_management_alert' + else + noteable_type.demodulize.underscore + end end def can_be_discussion_note? diff --git a/app/services/alert_management/alerts/update_service.rb b/app/services/alert_management/alerts/update_service.rb index 48b3868bb6a..dc2fb01f8b9 100644 --- a/app/services/alert_management/alerts/update_service.rb +++ b/app/services/alert_management/alerts/update_service.rb @@ -19,9 +19,11 @@ module AlertManagement return error_no_updates if params.empty? filter_assignees + old_assignees = alert.assignees.to_a if alert.update(params) - assign_todo + process_assignement(old_assignees) + success else error(alert.errors.full_messages.to_sentence) @@ -32,29 +34,10 @@ module AlertManagement attr_reader :alert, :current_user, :params - def assign_todo - return unless assignee - - todo_service.assign_alert(alert, assignee) - end - def allowed? current_user.can?(:update_alert_management_alert, alert) end - def filter_assignees - return if params[:assignees].nil? - - params[:assignees] = Array(assignee) - end - - def assignee - strong_memoize(:assignee) do - # Take first assignee while multiple are not currently supported - params[:assignees]&.first - end - end - def todo_service strong_memoize(:todo_service) do TodoService.new @@ -76,6 +59,35 @@ module AlertManagement def error_no_updates error(_('Please provide attributes to update')) end + + # ----- Assignee-related behavior ------ + def filter_assignees + return if params[:assignees].nil? + + params[:assignees] = Array(assignee) + end + + def assignee + strong_memoize(:assignee) do + # Take first assignee while multiple are not currently supported + params[:assignees]&.first + end + end + + def process_assignement(old_assignees) + assign_todo + add_assignee_system_note(old_assignees) + end + + def assign_todo + return unless assignee + + todo_service.assign_alert(alert, assignee) + end + + def add_assignee_system_note(old_assignees) + SystemNoteService.change_issuable_assignees(alert, alert.project, current_user, old_assignees) + end end end end diff --git a/app/views/projects/_new_project_fields.html.haml b/app/views/projects/_new_project_fields.html.haml index 5d88be0925e..e0a426607d4 100644 --- a/app/views/projects/_new_project_fields.html.haml +++ b/app/views/projects/_new_project_fields.html.haml @@ -23,7 +23,7 @@ display_path: true, extra_group: namespace_id), {}, - { class: 'select2 js-select-namespace qa-project-namespace-select block-truncated', tabindex: 1, data: { track_label: "#{track_label}", track_event: "activate_form_input", track_property: "project_path", track_value: "" }}) + { class: 'select2 js-select-namespace qa-project-namespace-select block-truncated', data: { track_label: "#{track_label}", track_event: "activate_form_input", track_property: "project_path", track_value: "" }}) - else .input-group-prepend.static-namespace.flex-shrink-0.has-tooltip{ title: user_url(current_user.username) + '/' } diff --git a/app/views/registrations/experience_levels/show.html.haml b/app/views/registrations/experience_levels/show.html.haml index f319bf513e4..24b87790e18 100644 --- a/app/views/registrations/experience_levels/show.html.haml +++ b/app/views/registrations/experience_levels/show.html.haml @@ -1,26 +1,28 @@ - page_title _('What’s your experience level?') -%h3= _('Hello there') -%p= _('Welcome to the guided GitLab tour') +.gl-display-flex.gl-flex-direction-column.gl-align-items-center + = image_tag 'learn-gitlab-avatar.jpg', width: '90' -%br + %h2.gl-text-center.gl-mt-3.gl-mb-3= _('Hello there') + %p.gl-text-center.gl-font-lg.gl-mb-6= _('Welcome to the guided GitLab tour') -%h5= _('What describes you best?') + %h3.gl-text-center.gl-font-lg.gl-mt-6.gl-mb-0= _('What describes you best?') -%hr + .card-deck.gl-mt-6 + .card + .card-body.gl-display-flex.gl-py-8.gl-pr-5.gl-pl-7 + .gl-align-self-center.gl-pr-6 + = image_tag 'novice.svg', width: '78', height: '78', alt: '' + %div + %p.gl-font-lg.gl-font-weight-bold.gl-mb-2= _('Novice') + %p= _('I’m not very familiar with the basics of project management and DevOps.') + = link_to _('Show me everything'), users_sign_up_experience_level_path(experience_level: :novice, namespace_path: params[:namespace_path]), method: :patch, class: 'stretched-link' -%div - %p - %b= _('Novice') - %p= _('I’m not very familiar with the basics of project management and DevOps.') - %p - = link_to _('Show me everything'), users_sign_up_experience_level_path(experience_level: :novice, namespace_path: params[:namespace_path]), method: :patch - -%hr - -%div - %p - %b= _('Experienced') - %p= _('I’m familiar with the basics of project management and DevOps.') - %p - = link_to _('Show me more advanced stuff'), users_sign_up_experience_level_path(experience_level: :experienced, namespace_path: params[:namespace_path]), method: :patch + .card + .card-body.gl-display-flex.gl-py-8.gl-pr-5.gl-pl-7 + .gl-align-self-center.gl-pr-6 + = image_tag 'experienced.svg', width: '78', height: '78', alt: '' + %div + %p.gl-font-lg.gl-font-weight-bold.gl-mb-2= _('Experienced') + %p= _('I’m familiar with the basics of project management and DevOps.') + = link_to _('Show me more advanced stuff'), users_sign_up_experience_level_path(experience_level: :experienced, namespace_path: params[:namespace_path]), method: :patch, class: 'stretched-link' diff --git a/app/workers/authorized_keys_worker.rb b/app/workers/authorized_keys_worker.rb index eabfd89ba60..ab0e7fc4921 100644 --- a/app/workers/authorized_keys_worker.rb +++ b/app/workers/authorized_keys_worker.rb @@ -9,6 +9,7 @@ class AuthorizedKeysWorker urgency :high weight 2 idempotent! + loggable_arguments 0 def perform(action, *args) return unless Gitlab::CurrentSettings.authorized_keys_enabled? diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index 6a64afe47de..bff864ba420 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -5,6 +5,7 @@ class BackgroundMigrationWorker # rubocop:disable Scalability/IdempotentWorker feature_category :database urgency :throttled + loggable_arguments 0, 1 # The minimum amount of time between processing two jobs of the same migration # class. diff --git a/app/workers/cleanup_container_repository_worker.rb b/app/workers/cleanup_container_repository_worker.rb index c3fac453e73..4469ea8cff9 100644 --- a/app/workers/cleanup_container_repository_worker.rb +++ b/app/workers/cleanup_container_repository_worker.rb @@ -5,6 +5,7 @@ class CleanupContainerRepositoryWorker # rubocop:disable Scalability/IdempotentW queue_namespace :container_repository feature_category :container_registry + loggable_arguments 2 attr_reader :container_repository, :current_user diff --git a/app/workers/cluster_install_app_worker.rb b/app/workers/cluster_install_app_worker.rb index 002932a0fa5..f3da4d5c4bb 100644 --- a/app/workers/cluster_install_app_worker.rb +++ b/app/workers/cluster_install_app_worker.rb @@ -6,6 +6,7 @@ class ClusterInstallAppWorker # rubocop:disable Scalability/IdempotentWorker include ClusterApplications worker_has_external_dependencies! + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/cluster_patch_app_worker.rb b/app/workers/cluster_patch_app_worker.rb index f75004aa3e5..b0393809802 100644 --- a/app/workers/cluster_patch_app_worker.rb +++ b/app/workers/cluster_patch_app_worker.rb @@ -6,6 +6,7 @@ class ClusterPatchAppWorker # rubocop:disable Scalability/IdempotentWorker include ClusterApplications worker_has_external_dependencies! + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/cluster_update_app_worker.rb b/app/workers/cluster_update_app_worker.rb index 7ceeb167b33..29feb813043 100644 --- a/app/workers/cluster_update_app_worker.rb +++ b/app/workers/cluster_update_app_worker.rb @@ -9,6 +9,7 @@ class ClusterUpdateAppWorker # rubocop:disable Scalability/IdempotentWorker include ExclusiveLeaseGuard sidekiq_options retry: 3, dead: false + loggable_arguments 0, 3 LEASE_TIMEOUT = 10.minutes.to_i diff --git a/app/workers/cluster_upgrade_app_worker.rb b/app/workers/cluster_upgrade_app_worker.rb index 99f48415f08..d4650ab3a85 100644 --- a/app/workers/cluster_upgrade_app_worker.rb +++ b/app/workers/cluster_upgrade_app_worker.rb @@ -6,6 +6,7 @@ class ClusterUpgradeAppWorker # rubocop:disable Scalability/IdempotentWorker include ClusterApplications worker_has_external_dependencies! + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/cluster_wait_for_app_installation_worker.rb b/app/workers/cluster_wait_for_app_installation_worker.rb index e098c3b86b5..4bc29807ea4 100644 --- a/app/workers/cluster_wait_for_app_installation_worker.rb +++ b/app/workers/cluster_wait_for_app_installation_worker.rb @@ -10,6 +10,7 @@ class ClusterWaitForAppInstallationWorker # rubocop:disable Scalability/Idempote worker_has_external_dependencies! worker_resource_boundary :cpu + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/cluster_wait_for_app_update_worker.rb b/app/workers/cluster_wait_for_app_update_worker.rb index 9f1d83c2c7b..c0a11eb93a7 100644 --- a/app/workers/cluster_wait_for_app_update_worker.rb +++ b/app/workers/cluster_wait_for_app_update_worker.rb @@ -8,6 +8,8 @@ class ClusterWaitForAppUpdateWorker # rubocop:disable Scalability/IdempotentWork INTERVAL = 10.seconds TIMEOUT = 20.minutes + loggable_arguments 0 + def perform(app_name, app_id) find_application(app_name, app_id) do |app| ::Clusters::Applications::CheckUpgradeProgressService.new(app).execute diff --git a/app/workers/cluster_wait_for_ingress_ip_address_worker.rb b/app/workers/cluster_wait_for_ingress_ip_address_worker.rb index c7336ee515d..fa46135d279 100644 --- a/app/workers/cluster_wait_for_ingress_ip_address_worker.rb +++ b/app/workers/cluster_wait_for_ingress_ip_address_worker.rb @@ -6,6 +6,7 @@ class ClusterWaitForIngressIpAddressWorker # rubocop:disable Scalability/Idempot include ClusterApplications worker_has_external_dependencies! + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/clusters/applications/activate_service_worker.rb b/app/workers/clusters/applications/activate_service_worker.rb index abd7f8eaea4..c92f978a7d2 100644 --- a/app/workers/clusters/applications/activate_service_worker.rb +++ b/app/workers/clusters/applications/activate_service_worker.rb @@ -6,6 +6,8 @@ module Clusters include ApplicationWorker include ClusterQueue + loggable_arguments 1 + def perform(cluster_id, service_name) cluster = Clusters::Cluster.find_by_id(cluster_id) return unless cluster diff --git a/app/workers/clusters/applications/deactivate_service_worker.rb b/app/workers/clusters/applications/deactivate_service_worker.rb index fecbb6dde45..4d103bb0edc 100644 --- a/app/workers/clusters/applications/deactivate_service_worker.rb +++ b/app/workers/clusters/applications/deactivate_service_worker.rb @@ -6,6 +6,8 @@ module Clusters include ApplicationWorker include ClusterQueue + loggable_arguments 1 + def perform(cluster_id, service_name) cluster = Clusters::Cluster.find_by_id(cluster_id) raise cluster_missing_error(service_name) unless cluster diff --git a/app/workers/clusters/applications/uninstall_worker.rb b/app/workers/clusters/applications/uninstall_worker.rb index 977a25e8442..a9307931b59 100644 --- a/app/workers/clusters/applications/uninstall_worker.rb +++ b/app/workers/clusters/applications/uninstall_worker.rb @@ -8,6 +8,7 @@ module Clusters include ClusterApplications worker_has_external_dependencies! + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/clusters/applications/wait_for_uninstall_app_worker.rb b/app/workers/clusters/applications/wait_for_uninstall_app_worker.rb index a486cfa90b7..dc842788374 100644 --- a/app/workers/clusters/applications/wait_for_uninstall_app_worker.rb +++ b/app/workers/clusters/applications/wait_for_uninstall_app_worker.rb @@ -12,6 +12,7 @@ module Clusters worker_has_external_dependencies! worker_resource_boundary :cpu + loggable_arguments 0 def perform(app_name, app_id) find_application(app_name, app_id) do |app| diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb index 6f77183a48a..9c942228111 100644 --- a/app/workers/concerns/application_worker.rb +++ b/app/workers/concerns/application_worker.rb @@ -76,6 +76,22 @@ module ApplicationWorker get_sidekiq_options['queue'].to_s end + # Set/get which arguments can be logged and sent to Sentry. + # + # Numeric arguments are logged by default, so there is no need to + # list those. + # + # Non-numeric arguments must be listed by position, as Sidekiq + # cannot see argument names. + # + def loggable_arguments(*args) + if args.any? + @loggable_arguments = args + else + @loggable_arguments || [] + end + end + def queue_size Sidekiq::Queue.new(queue).size end diff --git a/app/workers/concerns/reactive_cacheable_worker.rb b/app/workers/concerns/reactive_cacheable_worker.rb index e73707c2b43..189b0607605 100644 --- a/app/workers/concerns/reactive_cacheable_worker.rb +++ b/app/workers/concerns/reactive_cacheable_worker.rb @@ -7,6 +7,7 @@ module ReactiveCacheableWorker include ApplicationWorker feature_category_not_owned! + loggable_arguments 0 def self.context_for_arguments(arguments) class_name, *_other_args = arguments diff --git a/app/workers/create_commit_signature_worker.rb b/app/workers/create_commit_signature_worker.rb index aeb6104a35c..f81baf20d19 100644 --- a/app/workers/create_commit_signature_worker.rb +++ b/app/workers/create_commit_signature_worker.rb @@ -5,8 +5,8 @@ class CreateCommitSignatureWorker feature_category :source_code_management weight 2 - idempotent! + loggable_arguments 0 # rubocop: disable CodeReuse/ActiveRecord def perform(commit_shas, project_id) diff --git a/app/workers/create_pipeline_worker.rb b/app/workers/create_pipeline_worker.rb index 54698518e4f..68fe44d01ce 100644 --- a/app/workers/create_pipeline_worker.rb +++ b/app/workers/create_pipeline_worker.rb @@ -8,6 +8,7 @@ class CreatePipelineWorker # rubocop:disable Scalability/IdempotentWorker feature_category :continuous_integration urgency :high worker_resource_boundary :cpu + loggable_arguments 2, 3, 4 def perform(project_id, user_id, ref, source, params = {}) project = Project.find(project_id) diff --git a/app/workers/delete_stored_files_worker.rb b/app/workers/delete_stored_files_worker.rb index 463f26fdd5a..9cf5631b7d8 100644 --- a/app/workers/delete_stored_files_worker.rb +++ b/app/workers/delete_stored_files_worker.rb @@ -4,6 +4,7 @@ class DeleteStoredFilesWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker feature_category_not_owned! + loggable_arguments 0 def perform(class_name, keys) klass = begin diff --git a/app/workers/delete_user_worker.rb b/app/workers/delete_user_worker.rb index 2f2bf500730..ed2e00f1241 100644 --- a/app/workers/delete_user_worker.rb +++ b/app/workers/delete_user_worker.rb @@ -4,6 +4,7 @@ class DeleteUserWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker feature_category :authentication_and_authorization + loggable_arguments 2 def perform(current_user_id, delete_user_id, options = {}) delete_user = User.find(delete_user_id) diff --git a/app/workers/export_csv_worker.rb b/app/workers/export_csv_worker.rb index 9e2b3ad9bb4..e7baaf40a41 100644 --- a/app/workers/export_csv_worker.rb +++ b/app/workers/export_csv_worker.rb @@ -5,6 +5,7 @@ class ExportCsvWorker # rubocop:disable Scalability/IdempotentWorker feature_category :issue_tracking worker_resource_boundary :cpu + loggable_arguments 2 def perform(current_user_id, project_id, params) @current_user = User.find(current_user_id) diff --git a/app/workers/file_hook_worker.rb b/app/workers/file_hook_worker.rb index f8cdea54a17..b1422cd8795 100644 --- a/app/workers/file_hook_worker.rb +++ b/app/workers/file_hook_worker.rb @@ -5,6 +5,7 @@ class FileHookWorker # rubocop:disable Scalability/IdempotentWorker sidekiq_options retry: false feature_category :integrations + loggable_arguments 0 def perform(file_name, data) success, message = Gitlab::FileHook.execute(file_name, data) diff --git a/app/workers/git_garbage_collect_worker.rb b/app/workers/git_garbage_collect_worker.rb index 37ca3af517f..f2222c7be5e 100644 --- a/app/workers/git_garbage_collect_worker.rb +++ b/app/workers/git_garbage_collect_worker.rb @@ -5,6 +5,7 @@ class GitGarbageCollectWorker # rubocop:disable Scalability/IdempotentWorker sidekiq_options retry: false feature_category :gitaly + loggable_arguments 1, 2, 3 # Timeout set to 24h LEASE_TIMEOUT = 86400 diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 21e478f935b..834c2f7791c 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -12,6 +12,7 @@ module Gitlab sidekiq_options dead: false feature_category :importers + loggable_arguments 1, 2 # The known importer stages and their corresponding Sidekiq workers. STAGES = { diff --git a/app/workers/gitlab/jira_import/import_issue_worker.rb b/app/workers/gitlab/jira_import/import_issue_worker.rb index 89f5fe8d462..7709d2ec31b 100644 --- a/app/workers/gitlab/jira_import/import_issue_worker.rb +++ b/app/workers/gitlab/jira_import/import_issue_worker.rb @@ -8,6 +8,8 @@ module Gitlab include Gitlab::JiraImport::QueueOptions include Gitlab::Import::DatabaseHelpers + loggable_arguments 3 + def perform(project_id, jira_issue_id, issue_attributes, waiter_key) issue_id = create_issue(issue_attributes, project_id) JiraImport.cache_issue_mapping(issue_id, jira_issue_id, project_id) diff --git a/app/workers/gitlab_shell_worker.rb b/app/workers/gitlab_shell_worker.rb index b104d3c681e..b8e1e3d8fc4 100644 --- a/app/workers/gitlab_shell_worker.rb +++ b/app/workers/gitlab_shell_worker.rb @@ -7,6 +7,7 @@ class GitlabShellWorker # rubocop:disable Scalability/IdempotentWorker feature_category :source_code_management urgency :high weight 2 + loggable_arguments 0 def perform(action, *arg) # Gitlab::Shell is being removed but we need to continue to process jobs diff --git a/app/workers/group_export_worker.rb b/app/workers/group_export_worker.rb index 3e0390429d6..6fd977e43d8 100644 --- a/app/workers/group_export_worker.rb +++ b/app/workers/group_export_worker.rb @@ -5,6 +5,7 @@ class GroupExportWorker # rubocop:disable Scalability/IdempotentWorker include ExceptionBacktrace feature_category :importers + loggable_arguments 2 def perform(current_user_id, group_id, params = {}) current_user = User.find(current_user_id) diff --git a/app/workers/hashed_storage/project_migrate_worker.rb b/app/workers/hashed_storage/project_migrate_worker.rb index 3ce60ce7eb6..03e53058dbb 100644 --- a/app/workers/hashed_storage/project_migrate_worker.rb +++ b/app/workers/hashed_storage/project_migrate_worker.rb @@ -5,6 +5,7 @@ module HashedStorage include ApplicationWorker queue_namespace :hashed_storage + loggable_arguments 1 attr_reader :project_id diff --git a/app/workers/hashed_storage/project_rollback_worker.rb b/app/workers/hashed_storage/project_rollback_worker.rb index 17b3cca83e1..d4a5e474323 100644 --- a/app/workers/hashed_storage/project_rollback_worker.rb +++ b/app/workers/hashed_storage/project_rollback_worker.rb @@ -5,6 +5,7 @@ module HashedStorage include ApplicationWorker queue_namespace :hashed_storage + loggable_arguments 1 attr_reader :project_id diff --git a/app/workers/mail_scheduler/notification_service_worker.rb b/app/workers/mail_scheduler/notification_service_worker.rb index 691af2a724d..309f23c8708 100644 --- a/app/workers/mail_scheduler/notification_service_worker.rb +++ b/app/workers/mail_scheduler/notification_service_worker.rb @@ -9,6 +9,7 @@ module MailScheduler feature_category :issue_tracking worker_resource_boundary :cpu + loggable_arguments 0 def perform(meth, *args) check_arguments!(args) diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb index cc5fe884aec..270bd831f96 100644 --- a/app/workers/merge_worker.rb +++ b/app/workers/merge_worker.rb @@ -6,6 +6,7 @@ class MergeWorker # rubocop:disable Scalability/IdempotentWorker feature_category :source_code_management urgency :high weight 5 + loggable_arguments 2 def perform(merge_request_id, current_user_id, params) params = params.with_indifferent_access diff --git a/app/workers/object_storage/background_move_worker.rb b/app/workers/object_storage/background_move_worker.rb index 7b0a7c7ec58..fba91e49e43 100644 --- a/app/workers/object_storage/background_move_worker.rb +++ b/app/workers/object_storage/background_move_worker.rb @@ -7,6 +7,7 @@ module ObjectStorage sidekiq_options retry: 5 feature_category_not_owned! + loggable_arguments 0, 1, 2, 3 def perform(uploader_class_name, subject_class_name, file_field, subject_id) uploader_class = uploader_class_name.constantize diff --git a/app/workers/object_storage/migrate_uploads_worker.rb b/app/workers/object_storage/migrate_uploads_worker.rb index d9d21f2cb7e..33a0e0f5f88 100644 --- a/app/workers/object_storage/migrate_uploads_worker.rb +++ b/app/workers/object_storage/migrate_uploads_worker.rb @@ -7,6 +7,7 @@ module ObjectStorage include ObjectStorageQueue feature_category_not_owned! + loggable_arguments 0, 1, 2, 3 SanityCheckError = Class.new(StandardError) diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index 875f17282f9..d699e32c1a0 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -5,6 +5,7 @@ class PagesWorker # rubocop:disable Scalability/IdempotentWorker sidekiq_options retry: 3 feature_category :pages + loggable_arguments 0, 1 def perform(action, *arg) send(action, *arg) # rubocop:disable GitlabSecurity/PublicSend diff --git a/app/workers/pipeline_process_worker.rb b/app/workers/pipeline_process_worker.rb index 66a661dde71..cd7c82d3117 100644 --- a/app/workers/pipeline_process_worker.rb +++ b/app/workers/pipeline_process_worker.rb @@ -7,6 +7,7 @@ class PipelineProcessWorker # rubocop:disable Scalability/IdempotentWorker queue_namespace :pipeline_processing feature_category :continuous_integration urgency :high + loggable_arguments 1 # rubocop: disable CodeReuse/ActiveRecord def perform(pipeline_id, build_ids = nil) diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index ddf112e648c..62d76294bc0 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -7,6 +7,7 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker urgency :high worker_resource_boundary :cpu weight 5 + loggable_arguments 0, 1, 2, 3 def perform(gl_repository, identifier, changes, push_options = {}) container, project, repo_type = Gitlab::GlRepository.parse(gl_repository) diff --git a/app/workers/process_commit_worker.rb b/app/workers/process_commit_worker.rb index bdfabea8938..5756ebb8358 100644 --- a/app/workers/process_commit_worker.rb +++ b/app/workers/process_commit_worker.rb @@ -13,8 +13,8 @@ class ProcessCommitWorker feature_category :source_code_management urgency :high weight 3 - idempotent! + loggable_arguments 2, 3 # project_id - The ID of the project this commit belongs to. # user_id - The ID of the user that pushed the commit. diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 573f903f4e0..b114c67de47 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -4,11 +4,11 @@ class ProjectCacheWorker # rubocop:disable Scalability/IdempotentWorker include ApplicationWorker - urgency :high - LEASE_TIMEOUT = 15.minutes.to_i feature_category :source_code_management + urgency :high + loggable_arguments 1, 2, 3 # project_id - The ID of the project for which to flush the cache. # files - An Array containing extra types of files to refresh such as diff --git a/app/workers/project_export_worker.rb b/app/workers/project_export_worker.rb index a287c511a65..d29348e85bc 100644 --- a/app/workers/project_export_worker.rb +++ b/app/workers/project_export_worker.rb @@ -8,6 +8,7 @@ class ProjectExportWorker # rubocop:disable Scalability/IdempotentWorker feature_category :importers worker_resource_boundary :memory urgency :throttled + loggable_arguments 2, 3 def perform(current_user_id, project_id, after_export_strategy = {}, params = {}) current_user = User.find(current_user_id) diff --git a/app/workers/propagate_integration_worker.rb b/app/workers/propagate_integration_worker.rb index cbab38465bc..15c0e761a0a 100644 --- a/app/workers/propagate_integration_worker.rb +++ b/app/workers/propagate_integration_worker.rb @@ -4,8 +4,8 @@ class PropagateIntegrationWorker include ApplicationWorker feature_category :integrations - idempotent! + loggable_arguments 1 def perform(integration_id, overwrite) Admin::PropagateIntegrationService.propagate( diff --git a/app/workers/rebase_worker.rb b/app/workers/rebase_worker.rb index 2e13af5e0aa..ee9ae827bb6 100644 --- a/app/workers/rebase_worker.rb +++ b/app/workers/rebase_worker.rb @@ -7,6 +7,7 @@ class RebaseWorker # rubocop:disable Scalability/IdempotentWorker feature_category :source_code_management weight 2 + loggable_arguments 2 def perform(merge_request_id, current_user_id, skip_ci = false) current_user = User.find(current_user_id) diff --git a/app/workers/repository_check/batch_worker.rb b/app/workers/repository_check/batch_worker.rb index 9e762860ec6..1e2cb912598 100644 --- a/app/workers/repository_check/batch_worker.rb +++ b/app/workers/repository_check/batch_worker.rb @@ -12,6 +12,8 @@ module RepositoryCheck attr_reader :shard_name + loggable_arguments 0 + def perform(shard_name) @shard_name = shard_name diff --git a/app/workers/repository_remove_remote_worker.rb b/app/workers/repository_remove_remote_worker.rb index 23a9ec1e202..5e632b1b1ca 100644 --- a/app/workers/repository_remove_remote_worker.rb +++ b/app/workers/repository_remove_remote_worker.rb @@ -5,6 +5,7 @@ class RepositoryRemoveRemoteWorker # rubocop:disable Scalability/IdempotentWorke include ExclusiveLeaseGuard feature_category :source_code_management + loggable_arguments 1 LEASE_TIMEOUT = 1.hour diff --git a/app/workers/repository_update_remote_mirror_worker.rb b/app/workers/repository_update_remote_mirror_worker.rb index cfff2382f04..21b5916f459 100644 --- a/app/workers/repository_update_remote_mirror_worker.rb +++ b/app/workers/repository_update_remote_mirror_worker.rb @@ -10,6 +10,7 @@ class RepositoryUpdateRemoteMirrorWorker # rubocop:disable Scalability/Idempoten sidekiq_options retry: 3, dead: false feature_category :source_code_management + loggable_arguments 1 LOCK_WAIT_TIME = 30.seconds MAX_TRIES = 3 diff --git a/app/workers/todos_destroyer/entity_leave_worker.rb b/app/workers/todos_destroyer/entity_leave_worker.rb index 558cc32d158..4996456dc91 100644 --- a/app/workers/todos_destroyer/entity_leave_worker.rb +++ b/app/workers/todos_destroyer/entity_leave_worker.rb @@ -5,6 +5,8 @@ module TodosDestroyer include ApplicationWorker include TodosDestroyerQueue + loggable_arguments 2 + def perform(user_id, entity_id, entity_type) ::Todos::Destroy::EntityLeaveService.new(user_id, entity_id, entity_type).execute end diff --git a/app/workers/update_external_pull_requests_worker.rb b/app/workers/update_external_pull_requests_worker.rb index 0d48877e1b0..e916331ae82 100644 --- a/app/workers/update_external_pull_requests_worker.rb +++ b/app/workers/update_external_pull_requests_worker.rb @@ -5,6 +5,7 @@ class UpdateExternalPullRequestsWorker # rubocop:disable Scalability/IdempotentW feature_category :source_code_management weight 3 + loggable_arguments 2 def perform(project_id, user_id, ref) project = Project.find_by_id(project_id) diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb index 63bb6171b9c..98534b258a7 100644 --- a/app/workers/update_merge_requests_worker.rb +++ b/app/workers/update_merge_requests_worker.rb @@ -7,6 +7,7 @@ class UpdateMergeRequestsWorker # rubocop:disable Scalability/IdempotentWorker urgency :high worker_resource_boundary :cpu weight 3 + loggable_arguments 2, 3, 4 LOG_TIME_THRESHOLD = 90 # seconds diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb index 6e1e7e7d62e..5230f3bfa1f 100644 --- a/app/workers/web_hook_worker.rb +++ b/app/workers/web_hook_worker.rb @@ -5,6 +5,7 @@ class WebHookWorker # rubocop:disable Scalability/IdempotentWorker feature_category :integrations worker_has_external_dependencies! + loggable_arguments 2 sidekiq_options retry: 4, dead: false diff --git a/changelogs/unreleased/214921-improve-tabbing-behavior-when-creating-new-projects.yml b/changelogs/unreleased/214921-improve-tabbing-behavior-when-creating-new-projects.yml new file mode 100644 index 00000000000..e7d56cf3189 --- /dev/null +++ b/changelogs/unreleased/214921-improve-tabbing-behavior-when-creating-new-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix tabbing through form fields in projects/new flow +merge_request: 33209 +author: +type: fixed diff --git a/changelogs/unreleased/alert-system-notes.yml b/changelogs/unreleased/alert-system-notes.yml new file mode 100644 index 00000000000..2cff9e8b2a4 --- /dev/null +++ b/changelogs/unreleased/alert-system-notes.yml @@ -0,0 +1,5 @@ +--- +title: Add system note when assigning user to alert +merge_request: 33217 +author: +type: added diff --git a/changelogs/unreleased/sidekiq-arguments-logging-tokens.yml b/changelogs/unreleased/sidekiq-arguments-logging-tokens.yml new file mode 100644 index 00000000000..143a85b2986 --- /dev/null +++ b/changelogs/unreleased/sidekiq-arguments-logging-tokens.yml @@ -0,0 +1,5 @@ +--- +title: Filter potentially-sensitive Sidekiq arguments from logs and Sentry +merge_request: 33967 +author: +type: changed diff --git a/db/migrate/20200527170649_create_alert_management_alert_user_mentions.rb b/db/migrate/20200527170649_create_alert_management_alert_user_mentions.rb new file mode 100644 index 00000000000..15e4ab2a357 --- /dev/null +++ b/db/migrate/20200527170649_create_alert_management_alert_user_mentions.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class CreateAlertManagementAlertUserMentions < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :alert_management_alert_user_mentions do |t| + t.references :alert_management_alert, type: :bigint, index: false, null: false, foreign_key: { on_delete: :cascade } + t.bigint :note_id, null: true + + t.integer :mentioned_users_ids, array: true + t.integer :mentioned_projects_ids, array: true + t.integer :mentioned_groups_ids, array: true + end + + add_index :alert_management_alert_user_mentions, [:note_id], where: 'note_id IS NOT NULL', unique: true, name: 'index_alert_user_mentions_on_note_id' + add_index :alert_management_alert_user_mentions, [:alert_management_alert_id], where: 'note_id IS NULL', unique: true, name: 'index_alert_user_mentions_on_alert_id' + add_index :alert_management_alert_user_mentions, [:alert_management_alert_id, :note_id], unique: true, name: 'index_alert_user_mentions_on_alert_id_and_note_id' + end +end diff --git a/db/migrate/20200605003204_add_foreign_key_to_alert_management_alert_user_mentions.rb b/db/migrate/20200605003204_add_foreign_key_to_alert_management_alert_user_mentions.rb new file mode 100644 index 00000000000..35a250521a6 --- /dev/null +++ b/db/migrate/20200605003204_add_foreign_key_to_alert_management_alert_user_mentions.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddForeignKeyToAlertManagementAlertUserMentions < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + add_foreign_key :alert_management_alert_user_mentions, :notes, column: :note_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey + end + end + + def down + with_lock_retries do + remove_foreign_key :alert_management_alert_user_mentions, column: :note_id + end + end +end diff --git a/db/structure.sql b/db/structure.sql index bb90a6c09e3..98d397c801c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -37,6 +37,24 @@ CREATE SEQUENCE public.alert_management_alert_assignees_id_seq ALTER SEQUENCE public.alert_management_alert_assignees_id_seq OWNED BY public.alert_management_alert_assignees.id; +CREATE TABLE public.alert_management_alert_user_mentions ( + id bigint NOT NULL, + alert_management_alert_id bigint NOT NULL, + note_id bigint, + mentioned_users_ids integer[], + mentioned_projects_ids integer[], + mentioned_groups_ids integer[] +); + +CREATE SEQUENCE public.alert_management_alert_user_mentions_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE public.alert_management_alert_user_mentions_id_seq OWNED BY public.alert_management_alert_user_mentions.id; + CREATE TABLE public.alert_management_alerts ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, @@ -7401,6 +7419,8 @@ ALTER TABLE ONLY public.abuse_reports ALTER COLUMN id SET DEFAULT nextval('publi ALTER TABLE ONLY public.alert_management_alert_assignees ALTER COLUMN id SET DEFAULT nextval('public.alert_management_alert_assignees_id_seq'::regclass); +ALTER TABLE ONLY public.alert_management_alert_user_mentions ALTER COLUMN id SET DEFAULT nextval('public.alert_management_alert_user_mentions_id_seq'::regclass); + ALTER TABLE ONLY public.alert_management_alerts ALTER COLUMN id SET DEFAULT nextval('public.alert_management_alerts_id_seq'::regclass); ALTER TABLE ONLY public.alerts_service_data ALTER COLUMN id SET DEFAULT nextval('public.alerts_service_data_id_seq'::regclass); @@ -8045,6 +8065,9 @@ ALTER TABLE ONLY public.abuse_reports ALTER TABLE ONLY public.alert_management_alert_assignees ADD CONSTRAINT alert_management_alert_assignees_pkey PRIMARY KEY (id); +ALTER TABLE ONLY public.alert_management_alert_user_mentions + ADD CONSTRAINT alert_management_alert_user_mentions_pkey PRIMARY KEY (id); + ALTER TABLE ONLY public.alert_management_alerts ADD CONSTRAINT alert_management_alerts_pkey PRIMARY KEY (id); @@ -9194,6 +9217,12 @@ CREATE UNIQUE INDEX index_alert_management_alerts_on_project_id_and_fingerprint CREATE UNIQUE INDEX index_alert_management_alerts_on_project_id_and_iid ON public.alert_management_alerts USING btree (project_id, iid); +CREATE UNIQUE INDEX index_alert_user_mentions_on_alert_id ON public.alert_management_alert_user_mentions USING btree (alert_management_alert_id) WHERE (note_id IS NULL); + +CREATE UNIQUE INDEX index_alert_user_mentions_on_alert_id_and_note_id ON public.alert_management_alert_user_mentions USING btree (alert_management_alert_id, note_id); + +CREATE UNIQUE INDEX index_alert_user_mentions_on_note_id ON public.alert_management_alert_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL); + CREATE INDEX index_alerts_service_data_on_service_id ON public.alerts_service_data USING btree (service_id); CREATE INDEX index_allowed_email_domains_on_group_id ON public.allowed_email_domains USING btree (group_id); @@ -12391,6 +12420,9 @@ ALTER TABLE ONLY public.design_user_mentions ALTER TABLE ONLY public.clusters_kubernetes_namespaces ADD CONSTRAINT fk_rails_8df789f3ab FOREIGN KEY (environment_id) REFERENCES public.environments(id) ON DELETE SET NULL; +ALTER TABLE ONLY public.alert_management_alert_user_mentions + ADD CONSTRAINT fk_rails_8e48eca0fe FOREIGN KEY (alert_management_alert_id) REFERENCES public.alert_management_alerts(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.project_daily_statistics ADD CONSTRAINT fk_rails_8e549b272d FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; @@ -12769,6 +12801,9 @@ ALTER TABLE ONLY public.merge_request_blocks ALTER TABLE ONLY public.protected_branch_unprotect_access_levels ADD CONSTRAINT fk_rails_e9eb8dc025 FOREIGN KEY (protected_branch_id) REFERENCES public.protected_branches(id) ON DELETE CASCADE; +ALTER TABLE ONLY public.alert_management_alert_user_mentions + ADD CONSTRAINT fk_rails_eb2de0cdef FOREIGN KEY (note_id) REFERENCES public.notes(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.ci_daily_report_results ADD CONSTRAINT fk_rails_ebc2931b90 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; @@ -13873,6 +13908,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200527151413 20200527152116 20200527152657 +20200527170649 20200527211000 20200528054112 20200528123703 @@ -13886,6 +13922,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200604145731 20200604174544 20200604174558 +20200605003204 20200608072931 20200608075553 20200609002841 diff --git a/doc/administration/troubleshooting/sidekiq.md b/doc/administration/troubleshooting/sidekiq.md index c04ffa743e1..5109a3baff2 100644 --- a/doc/administration/troubleshooting/sidekiq.md +++ b/doc/administration/troubleshooting/sidekiq.md @@ -29,9 +29,18 @@ Example: gitlab_rails['env'] = {"SIDEKIQ_LOG_ARGUMENTS" => "1"} ``` -Please note: It is not recommend to enable this setting in production because some -Sidekiq jobs (such as sending a password reset email) take secret arguments (for -example the password reset token). +This does not log all job arguments. To avoid logging sensitive +information (for instance, password reset tokens), it logs numeric +arguments for all workers, with overrides for some specific workers +where their arguments are not sensitive. + +Example log output: + +```json +{"severity":"INFO","time":"2020-06-08T14:37:37.892Z","class":"AdminEmailsWorker","args":["[FILTERED]","[FILTERED]","[FILTERED]"],"retry":3,"queue":"admin_emails","backtrace":true,"jid":"9e35e2674ac7b12d123e13cc","created_at":"2020-06-08T14:37:37.373Z","meta.user":"root","meta.caller_id":"Admin::EmailsController#create","correlation_id":"37D3lArJmT1","uber-trace-id":"2d942cc98cc1b561:6dc94409cfdd4d77:9fbe19bdee865293:1","enqueued_at":"2020-06-08T14:37:37.410Z","pid":65011,"message":"AdminEmailsWorker JID-9e35e2674ac7b12d123e13cc: done: 0.48085 sec","job_status":"done","scheduling_latency_s":0.001012,"redis_calls":9,"redis_duration_s":0.004608,"redis_read_bytes":696,"redis_write_bytes":6141,"duration_s":0.48085,"cpu_s":0.308849,"completed_at":"2020-06-08T14:37:37.892Z","db_duration_s":0.010742} +{"severity":"INFO","time":"2020-06-08T14:37:37.894Z","class":"ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper","wrapped":"ActionMailer::DeliveryJob","queue":"mailers","args":["[FILTERED]"],"retry":3,"backtrace":true,"jid":"e47a4f6793d475378432e3c8","created_at":"2020-06-08T14:37:37.884Z","meta.user":"root","meta.caller_id":"AdminEmailsWorker","correlation_id":"37D3lArJmT1","uber-trace-id":"2d942cc98cc1b561:29344de0f966446d:5c3b0e0e1bef987b:1","enqueued_at":"2020-06-08T14:37:37.885Z","pid":65011,"message":"ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper JID-e47a4f6793d475378432e3c8: start","job_status":"start","scheduling_latency_s":0.009473} +{"severity":"INFO","time":"2020-06-08T14:39:50.648Z","class":"NewIssueWorker","args":["455","1"],"retry":3,"queue":"new_issue","backtrace":true,"jid":"a24af71f96fd129ec47f5d1e","created_at":"2020-06-08T14:39:50.643Z","meta.user":"root","meta.project":"h5bp/html5-boilerplate","meta.root_namespace":"h5bp","meta.caller_id":"Projects::IssuesController#create","correlation_id":"f9UCZHqhuP7","uber-trace-id":"28f65730f99f55a3:a5d2b62dec38dffc:48ddd092707fa1b7:1","enqueued_at":"2020-06-08T14:39:50.646Z","pid":65011,"message":"NewIssueWorker JID-a24af71f96fd129ec47f5d1e: start","job_status":"start","scheduling_latency_s":0.001144} +``` When using [Sidekiq JSON logging](../logs.md#sidekiqlog), arguments logs are limited to a maximum size of 10 kilobytes of text; diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 9c478f2d56e..8e812fd2a5e 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -168,7 +168,7 @@ type AdminSidekiqQueuesDeleteJobsPayload { """ Describes an alert from the project's Alert Management """ -type AlertManagementAlert { +type AlertManagementAlert implements Noteable { """ Assignees of the alert """ @@ -210,6 +210,31 @@ type AlertManagementAlert { details: JSON """ + All discussions on this noteable + """ + discussions( + """ + Returns the elements in the list that come after the specified cursor. + """ + after: String + + """ + Returns the elements in the list that come before the specified cursor. + """ + before: String + + """ + Returns the first _n_ elements from the list. + """ + first: Int + + """ + Returns the last _n_ elements from the list. + """ + last: Int + ): DiscussionConnection! + + """ Timestamp the alert ended """ endedAt: Time @@ -240,6 +265,31 @@ type AlertManagementAlert { monitoringTool: String """ + All notes on this noteable + """ + notes( + """ + Returns the elements in the list that come after the specified cursor. + """ + after: String + + """ + Returns the elements in the list that come before the specified cursor. + """ + before: String + + """ + Returns the first _n_ elements from the list. + """ + first: Int + + """ + Returns the last _n_ elements from the list. + """ + last: Int + ): NoteConnection! + + """ Service the alert came from """ service: String @@ -11764,11 +11814,6 @@ type TestReport { id: ID! """ - Pipeline that created the test report - """ - pipeline: Pipeline - - """ State of the test report """ state: TestReportState! diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 90aaeac3c27..3f86c47c334 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -578,6 +578,63 @@ "deprecationReason": null }, { + "name": "discussions", + "description": "All discussions on this noteable", + "args": [ + { + "name": "after", + "description": "Returns the elements in the list that come after the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "before", + "description": "Returns the elements in the list that come before the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "first", + "description": "Returns the first _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "last", + "description": "Returns the last _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + } + ], + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "OBJECT", + "name": "DiscussionConnection", + "ofType": null + } + }, + "isDeprecated": false, + "deprecationReason": null + }, + { "name": "endedAt", "description": "Timestamp the alert ended", "args": [ @@ -674,6 +731,63 @@ "deprecationReason": null }, { + "name": "notes", + "description": "All notes on this noteable", + "args": [ + { + "name": "after", + "description": "Returns the elements in the list that come after the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "before", + "description": "Returns the elements in the list that come before the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "first", + "description": "Returns the first _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "last", + "description": "Returns the last _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + } + ], + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "OBJECT", + "name": "NoteConnection", + "ofType": null + } + }, + "isDeprecated": false, + "deprecationReason": null + }, + { "name": "service", "description": "Service the alert came from", "args": [ @@ -760,7 +874,11 @@ ], "inputFields": null, "interfaces": [ - + { + "kind": "INTERFACE", + "name": "Noteable", + "ofType": null + } ], "enumValues": null, "possibleTypes": null @@ -23587,6 +23705,11 @@ "possibleTypes": [ { "kind": "OBJECT", + "name": "AlertManagementAlert", + "ofType": null + }, + { + "kind": "OBJECT", "name": "Design", "ofType": null }, @@ -34806,20 +34929,6 @@ "deprecationReason": null }, { - "name": "pipeline", - "description": "Pipeline that created the test report", - "args": [ - - ], - "type": { - "kind": "OBJECT", - "name": "Pipeline", - "ofType": null - }, - "isDeprecated": false, - "deprecationReason": null - }, - { "name": "state", "description": "State of the test report", "args": [ diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 7387dfaaa55..2abb07183b9 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -1745,7 +1745,6 @@ Represents a requirement test report. | `author` | User | Author of the test report | | `createdAt` | Time! | Timestamp of when the test report was created | | `id` | ID! | ID of the test report | -| `pipeline` | Pipeline | Pipeline that created the test report | | `state` | TestReportState! | State of the test report | ## Timelog diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md index 1b3278c0eb9..7ae3c9e9de2 100644 --- a/doc/development/sidekiq_style_guide.md +++ b/doc/development/sidekiq_style_guide.md @@ -518,6 +518,34 @@ job needs to be scheduled with. The `context_proc` which needs to return a hash with the context information for the job. +## Arguments logging + +When [`SIDEKIQ_LOG_ARGUMENTS`](../administration/troubleshooting/sidekiq.md#log-arguments-to-sidekiq-jobs) +is enabled, Sidekiq job arguments will be logged. + +By default, the only arguments logged are numeric arguments, because +arguments of other types could contain sensitive information. To +override this, use `loggable_arguments` inside a worker with the indexes +of the arguments to be logged. (Numeric arguments do not need to be +specified here.) + +For example: + +```ruby +class MyWorker + include ApplicationWorker + + loggable_arguments 1, 3 + + # object_id will be logged as it's numeric + # string_a will be logged due to the loggable_arguments call + # string_b will be filtered from logs + # string_c will be logged due to the loggable_arguments call + def perform(object_id, string_a, string_b, string_c) + end +end +``` + ## Tests Each Sidekiq worker must be tested using RSpec, just like any other class. These diff --git a/lib/gitlab/error_tracking.rb b/lib/gitlab/error_tracking.rb index b893d625f8d..a19ce22e53f 100644 --- a/lib/gitlab/error_tracking.rb +++ b/lib/gitlab/error_tracking.rb @@ -26,10 +26,13 @@ module Gitlab # Sanitize fields based on those sanitized from Rails. config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s) + config.processors << ::Gitlab::ErrorTracking::Processor::SidekiqProcessor # Sanitize authentication headers config.sanitize_http_headers = %w[Authorization Private-Token] config.tags = { program: Gitlab.process_name } config.before_send = method(:before_send) + + yield config if block_given? end end diff --git a/lib/gitlab/error_tracking/processor/sidekiq_processor.rb b/lib/gitlab/error_tracking/processor/sidekiq_processor.rb new file mode 100644 index 00000000000..272cb689ad5 --- /dev/null +++ b/lib/gitlab/error_tracking/processor/sidekiq_processor.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'set' + +module Gitlab + module ErrorTracking + module Processor + class SidekiqProcessor < ::Raven::Processor + FILTERED_STRING = '[FILTERED]' + + def self.filter_arguments(args, klass) + args.lazy.with_index.map do |arg, i| + case arg + when Numeric + arg + else + if permitted_arguments_for_worker(klass).include?(i) + arg + else + FILTERED_STRING + end + end + end + end + + def self.permitted_arguments_for_worker(klass) + @permitted_arguments_for_worker ||= {} + @permitted_arguments_for_worker[klass] ||= + begin + klass.constantize&.loggable_arguments&.to_set + rescue + Set.new + end + end + + def self.loggable_arguments(args, klass) + Gitlab::Utils::LogLimitedArray + .log_limited_array(filter_arguments(args, klass)) + .map(&:to_s) + .to_a + end + + def process(value, key = nil) + sidekiq = value.dig(:extra, :sidekiq) + + return value unless sidekiq + + sidekiq = sidekiq.deep_dup + sidekiq.delete(:jobstr) + + # 'args' in this hash => from Gitlab::ErrorTracking.track_* + # 'args' in :job => from default error handler + job_holder = sidekiq.key?('args') ? sidekiq : sidekiq[:job] + + if job_holder['args'] + job_holder['args'] = self.class.filter_arguments(job_holder['args'], job_holder['class']).to_a + end + + value[:extra][:sidekiq] = sidekiq + + value + end + end + end + end +end diff --git a/lib/gitlab/exception_log_formatter.rb b/lib/gitlab/exception_log_formatter.rb index 4b1cb379911..2da1b8915e4 100644 --- a/lib/gitlab/exception_log_formatter.rb +++ b/lib/gitlab/exception_log_formatter.rb @@ -14,18 +14,11 @@ module Gitlab payload.delete('extra.server') - # The raven extra context is populated by Raven::SidekiqCleanupMiddleware. - # - # It contains the full sidekiq job which consists of mixed types and nested - # objects. That causes a bunch of issues when trying to ingest logs into - # Elasticsearch. - # - # We apply a stricter schema here that forces the args to be an array of - # strings. This same logic exists in Gitlab::SidekiqLogging::JSONFormatter. payload['extra.sidekiq'].tap do |value| if value.is_a?(Hash) && value.key?('args') value = value.dup - payload['extra.sidekiq']['args'] = Gitlab::Utils::LogLimitedArray.log_limited_array(value['args'].try(:map, &:to_s)) + payload['extra.sidekiq']['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor + .loggable_arguments(value['args'], value['class']) end end diff --git a/lib/gitlab/sidekiq_logging/json_formatter.rb b/lib/gitlab/sidekiq_logging/json_formatter.rb index 64782e1e1d1..8894b48417c 100644 --- a/lib/gitlab/sidekiq_logging/json_formatter.rb +++ b/lib/gitlab/sidekiq_logging/json_formatter.rb @@ -18,10 +18,15 @@ module Gitlab when String output[:message] = data when Hash - convert_to_iso8601!(data) - convert_retry_to_integer!(data) - stringify_args!(data) output.merge!(data) + + # jobstr is redundant and can include information we wanted to + # exclude (like arguments) + output.delete(:jobstr) + + convert_to_iso8601!(output) + convert_retry_to_integer!(output) + process_args!(output) end output.to_json + "\n" @@ -56,8 +61,11 @@ module Gitlab end end - def stringify_args!(payload) - payload['args'] = Gitlab::Utils::LogLimitedArray.log_limited_array(payload['args'].map(&:to_s)) if payload['args'] + def process_args!(payload) + return unless payload['args'] + + payload['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor + .loggable_arguments(payload['args'], payload['class']) end end end diff --git a/lib/gitlab/utils/log_limited_array.rb b/lib/gitlab/utils/log_limited_array.rb index e0589c3df4c..fbbba568d14 100644 --- a/lib/gitlab/utils/log_limited_array.rb +++ b/lib/gitlab/utils/log_limited_array.rb @@ -9,14 +9,14 @@ module Gitlab # to around 10 kilobytes. Once we hit the limit, add the sentinel # value as the last item in the returned array. def self.log_limited_array(array, sentinel: '...') - return [] unless array.is_a?(Array) + return [] unless array.is_a?(Array) || array.is_a?(Enumerator::Lazy) total_length = 0 limited_array = array.take_while do |arg| total_length += JsonSizeEstimator.estimate(arg) total_length <= MAXIMUM_ARRAY_LENGTH - end + end.to_a limited_array.push(sentinel) if total_length > MAXIMUM_ARRAY_LENGTH diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 3db4723a1e9..52e91f31ec1 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -17,6 +17,7 @@ FactoryBot.define do factory :note_on_project_snippet, traits: [:on_project_snippet] factory :note_on_personal_snippet, traits: [:on_personal_snippet] factory :note_on_design, traits: [:on_design] + factory :note_on_alert, traits: [:on_alert] factory :system_note, traits: [:system] factory :discussion_note, class: 'DiscussionNote' @@ -145,6 +146,10 @@ FactoryBot.define do end end + trait :on_alert do + noteable { association(:alert_management_alert, project: project) } + end + trait :resolved do resolved_at { Time.now } resolved_by { association(:user) } diff --git a/spec/frontend/alert_management/components/alert_management_system_note_spec.js b/spec/frontend/alert_management/components/alert_management_system_note_spec.js new file mode 100644 index 00000000000..87dc36cc7cb --- /dev/null +++ b/spec/frontend/alert_management/components/alert_management_system_note_spec.js @@ -0,0 +1,34 @@ +import { shallowMount } from '@vue/test-utils'; +import SystemNote from '~/alert_management/components/system_notes/system_note.vue'; +import mockAlerts from '../mocks/alerts.json'; + +const mockAlert = mockAlerts[1]; + +describe('Alert Details System Note', () => { + let wrapper; + + function mountComponent({ stubs = {} } = {}) { + wrapper = shallowMount(SystemNote, { + propsData: { + note: { ...mockAlert.notes.nodes[0] }, + }, + stubs, + }); + } + + afterEach(() => { + if (wrapper) { + wrapper.destroy(); + } + }); + + describe('System notes', () => { + beforeEach(() => { + mountComponent({}); + }); + + it('renders the correct system note', () => { + expect(wrapper.find('.note-wrapper').attributes('id')).toBe('note_1628'); + }); + }); +}); diff --git a/spec/frontend/alert_management/mocks/alerts.json b/spec/frontend/alert_management/mocks/alerts.json index 60bf075016c..312d1756790 100644 --- a/spec/frontend/alert_management/mocks/alerts.json +++ b/spec/frontend/alert_management/mocks/alerts.json @@ -8,7 +8,8 @@ "startedAt": "2020-04-17T23:18:14.996Z", "endedAt": "2020-04-17T23:18:14.996Z", "status": "TRIGGERED", - "assignees": { "nodes": [] } + "assignees": { "nodes": [] }, + "notes": { "nodes": [] } }, { "iid": "1527543", @@ -18,7 +19,23 @@ "startedAt": "2020-04-17T23:18:14.996Z", "endedAt": "2020-04-17T23:18:14.996Z", "status": "ACKNOWLEDGED", - "assignees": { "nodes": [{ "username": "root" }] } + "assignees": { "nodes": [{ "username": "root" }] }, + "notes": { + "nodes": [ + { + "id": "gid://gitlab/Note/1628", + "author": { + "id": "gid://gitlab/User/1", + "state": "active", + "__typename": "User", + "avatarUrl": "https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon", + "name": "Administrator", + "username": "root", + "webUrl": "http://192.168.1.4:3000/root" + } + } + ] + } }, { "iid": "1527544", @@ -28,6 +45,22 @@ "startedAt": "2020-04-17T23:18:14.996Z", "endedAt": "2020-04-17T23:18:14.996Z", "status": "RESOLVED", - "assignees": { "nodes": [{ "username": "root" }] } + "assignees": { "nodes": [{ "username": "root" }] }, + "notes": { + "nodes": [ + { + "id": "gid://gitlab/Note/1629", + "author": { + "id": "gid://gitlab/User/2", + "state": "active", + "__typename": "User", + "avatarUrl": "https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon", + "name": "Administrator", + "username": "root", + "webUrl": "http://192.168.1.4:3000/root" + } + } + ] + } } ] diff --git a/spec/graphql/types/alert_management/alert_type_spec.rb b/spec/graphql/types/alert_management/alert_type_spec.rb index 1ef2e63f47e..5acbf8ebb7a 100644 --- a/spec/graphql/types/alert_management/alert_type_spec.rb +++ b/spec/graphql/types/alert_management/alert_type_spec.rb @@ -25,6 +25,8 @@ describe GitlabSchema.types['AlertManagementAlert'] do created_at updated_at assignees + notes + discussions ] expect(described_class).to have_graphql_fields(*expected_fields) diff --git a/spec/graphql/types/notes/noteable_type_spec.rb b/spec/graphql/types/notes/noteable_type_spec.rb index e673076d8a7..88d8eae56d1 100644 --- a/spec/graphql/types/notes/noteable_type_spec.rb +++ b/spec/graphql/types/notes/noteable_type_spec.rb @@ -17,6 +17,7 @@ describe Types::Notes::NoteableType do expect(described_class.resolve_type(build(:issue), {})).to eq(Types::IssueType) expect(described_class.resolve_type(build(:merge_request), {})).to eq(Types::MergeRequestType) expect(described_class.resolve_type(build(:design), {})).to eq(Types::DesignManagement::DesignType) + expect(described_class.resolve_type(build(:alert_management_alert), {})).to eq(Types::AlertManagement::AlertType) end end end diff --git a/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb new file mode 100644 index 00000000000..da7205c7f4f --- /dev/null +++ b/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb @@ -0,0 +1,165 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rspec-parameterized' + +RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do + after do + if described_class.instance_variable_defined?(:@permitted_arguments_for_worker) + described_class.remove_instance_variable(:@permitted_arguments_for_worker) + end + end + + describe '.filter_arguments' do + it 'returns a lazy enumerator' do + filtered = described_class.filter_arguments([1, 'string'], 'TestWorker') + + expect(filtered).to be_a(Enumerator::Lazy) + expect(filtered.to_a).to eq([1, described_class::FILTERED_STRING]) + end + + context 'arguments filtering' do + using RSpec::Parameterized::TableSyntax + + where(:klass, :expected) do + 'UnknownWorker' | [1, described_class::FILTERED_STRING, described_class::FILTERED_STRING, described_class::FILTERED_STRING] + 'NoPermittedArguments' | [1, described_class::FILTERED_STRING, described_class::FILTERED_STRING, described_class::FILTERED_STRING] + 'OnePermittedArgument' | [1, 'string', described_class::FILTERED_STRING, described_class::FILTERED_STRING] + 'AllPermittedArguments' | [1, 'string', [1, 2], { a: 1 }] + end + + with_them do + before do + stub_const('NoPermittedArguments', double(loggable_arguments: [])) + stub_const('OnePermittedArgument', double(loggable_arguments: [1])) + stub_const('AllPermittedArguments', double(loggable_arguments: [0, 1, 2, 3])) + end + + it do + expect(described_class.filter_arguments([1, 'string', [1, 2], { a: 1 }], klass).to_a) + .to eq(expected) + end + end + end + end + + describe '.permitted_arguments_for_worker' do + it 'returns the loggable_arguments for a worker class as a set' do + stub_const('TestWorker', double(loggable_arguments: [1, 1])) + + expect(described_class.permitted_arguments_for_worker('TestWorker')) + .to eq([1].to_set) + end + + it 'returns an empty set when the worker class does not exist' do + expect(described_class.permitted_arguments_for_worker('TestWorker')) + .to eq(Set.new) + end + + it 'returns an empty set when the worker class does not respond to loggable_arguments' do + stub_const('TestWorker', 1) + + expect(described_class.permitted_arguments_for_worker('TestWorker')) + .to eq(Set.new) + end + + it 'returns an empty set when loggable_arguments cannot be converted to a set' do + stub_const('TestWorker', double(loggable_arguments: 1)) + + expect(described_class.permitted_arguments_for_worker('TestWorker')) + .to eq(Set.new) + end + + it 'memoizes the results' do + worker_class = double + + stub_const('TestWorker', worker_class) + + expect(worker_class).to receive(:loggable_arguments).once.and_return([]) + + described_class.permitted_arguments_for_worker('TestWorker') + described_class.permitted_arguments_for_worker('TestWorker') + end + end + + describe '.loggable_arguments' do + it 'filters and limits the arguments, then converts to strings' do + half_limit = Gitlab::Utils::LogLimitedArray::MAXIMUM_ARRAY_LENGTH / 2 + args = [[1, 2], 'a' * half_limit, 'b' * half_limit, 'c' * half_limit, 'd'] + + stub_const('LoggableArguments', double(loggable_arguments: [0, 1, 3, 4])) + + expect(described_class.loggable_arguments(args, 'LoggableArguments')) + .to eq(['[1, 2]', 'a' * half_limit, '[FILTERED]', '...']) + end + end + + describe '#process' do + context 'when there is Sidekiq data' do + shared_examples 'Sidekiq arguments' do |args_in_job_hash: true| + let(:path) { [:extra, :sidekiq, args_in_job_hash ? :job : nil, 'args'].compact } + let(:args) { [1, 'string', { a: 1 }, [1, 2]] } + + it 'only allows numeric arguments for an unknown worker' do + value = { 'args' => args, 'class' => 'UnknownWorker' } + + value = { job: value } if args_in_job_hash + + expect(subject.process(extra_sidekiq(value)).dig(*path)) + .to eq([1, described_class::FILTERED_STRING, described_class::FILTERED_STRING, described_class::FILTERED_STRING]) + end + + it 'allows all argument types for a permitted worker' do + value = { 'args' => args, 'class' => 'PostReceive' } + + value = { job: value } if args_in_job_hash + + expect(subject.process(extra_sidekiq(value)).dig(*path)) + .to eq(args) + end + end + + context 'when processing via the default error handler' do + include_examples 'Sidekiq arguments', args_in_job_hash: true + end + + context 'when processing via Gitlab::ErrorTracking' do + include_examples 'Sidekiq arguments', args_in_job_hash: false + end + + it 'removes a jobstr field if present' do + value = { + job: { 'args' => [1] }, + jobstr: { 'args' => [1] }.to_json + } + + expect(subject.process(extra_sidekiq(value))) + .to eq(extra_sidekiq(value.except(:jobstr))) + end + + it 'does nothing with no jobstr' do + value = { job: { 'args' => [1] } } + + expect(subject.process(extra_sidekiq(value))) + .to eq(extra_sidekiq(value)) + end + end + + context 'when there is no Sidekiq data' do + it 'does nothing' do + value = { + request: { + method: 'POST', + data: { 'key' => 'value' } + } + } + + expect(subject.process(value)).to eq(value) + end + end + + def extra_sidekiq(hash) + { extra: { sidekiq: hash } } + end + end +end diff --git a/spec/lib/gitlab/error_tracking_spec.rb b/spec/lib/gitlab/error_tracking_spec.rb index 4d04045a715..c40369f5965 100644 --- a/spec/lib/gitlab/error_tracking_spec.rb +++ b/spec/lib/gitlab/error_tracking_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' +require 'raven/transports/dummy' + describe Gitlab::ErrorTracking do let(:exception) { RuntimeError.new('boom') } let(:issue_url) { 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' } @@ -22,7 +24,9 @@ describe Gitlab::ErrorTracking do allow(described_class).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn) allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') - described_class.configure + described_class.configure do |config| + config.encoding = 'json' + end end describe '.with_context' do @@ -181,13 +185,26 @@ describe Gitlab::ErrorTracking do end context 'with sidekiq args' do - let(:extra) { { sidekiq: { 'args' => [1, { 'id' => 2, 'name' => 'hello' }, 'some-value', 'another-value'] } } } - it 'ensures extra.sidekiq.args is a string' do + extra = { sidekiq: { 'class' => 'PostReceive', 'args' => [1, { 'id' => 2, 'name' => 'hello' }, 'some-value', 'another-value'] } } + + expect(Gitlab::ErrorTracking::Logger).to receive(:error).with( + hash_including({ 'extra.sidekiq' => { 'class' => 'PostReceive', 'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value'] } })) + + described_class.track_exception(exception, extra) + end + + it 'filters sensitive arguments before sending' do + extra = { sidekiq: { 'class' => 'UnknownWorker', 'args' => ['sensitive string', 1, 2] } } + expect(Gitlab::ErrorTracking::Logger).to receive(:error).with( - hash_including({ 'extra.sidekiq' => { 'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value'] } })) + hash_including('extra.sidekiq' => { 'class' => 'UnknownWorker', 'args' => ['[FILTERED]', '1', '2'] })) described_class.track_exception(exception, extra) + + sentry_event = Gitlab::Json.parse(Raven.client.transport.events.last[1]) + + expect(sentry_event.dig('extra', 'sidekiq', 'args')).to eq(['[FILTERED]', 1, 2]) end end end diff --git a/spec/lib/gitlab/sidekiq_logging/json_formatter_spec.rb b/spec/lib/gitlab/sidekiq_logging/json_formatter_spec.rb index 283140d7fdf..10354147cf9 100644 --- a/spec/lib/gitlab/sidekiq_logging/json_formatter_spec.rb +++ b/spec/lib/gitlab/sidekiq_logging/json_formatter_spec.rb @@ -14,6 +14,7 @@ describe Gitlab::SidekiqLogging::JSONFormatter do let(:hash_input) do { foo: 1, + 'class' => 'PostReceive', 'bar' => 'test', 'created_at' => timestamp, 'enqueued_at' => timestamp, @@ -42,21 +43,47 @@ describe Gitlab::SidekiqLogging::JSONFormatter do expect(subject).to eq(expected_output) end - context 'when the job args are bigger than the maximum allowed' do - it 'keeps args from the front until they exceed the limit' do - half_limit = Gitlab::Utils::LogLimitedArray::MAXIMUM_ARRAY_LENGTH / 2 - hash_input['args'] = [1, 2, 'a' * half_limit, 'b' * half_limit, 3] + it 'removes jobstr from the hash' do + hash_input[:jobstr] = 'job string' - expected_args = hash_input['args'].take(3).map(&:to_s) + ['...'] + expect(subject).not_to include('jobstr') + end - expect(subject['args']).to eq(expected_args) - end + it 'does not modify the input hash' do + input = { 'args' => [1, 'string'] } + + output = Gitlab::Json.parse(described_class.new.call('INFO', now, 'my program', input)) + + expect(input['args']).to eq([1, 'string']) + expect(output['args']).to eq(['1', '[FILTERED]']) end - it 'properly flattens arguments to a String' do - hash_input['args'] = [1, "test", 2, { 'test' => 1 }] + context 'job arguments' do + context 'when the arguments are bigger than the maximum allowed' do + it 'keeps args from the front until they exceed the limit' do + half_limit = Gitlab::Utils::LogLimitedArray::MAXIMUM_ARRAY_LENGTH / 2 + hash_input['args'] = [1, 2, 'a' * half_limit, 'b' * half_limit, 3] + + expected_args = hash_input['args'].take(3).map(&:to_s) + ['...'] + + expect(subject['args']).to eq(expected_args) + end + end + + context 'when the job has non-integer arguments' do + it 'only allows permitted non-integer arguments through' do + hash_input['args'] = [1, 'foo', 'bar'] + hash_input['class'] = 'WebHookWorker' - expect(subject['args']).to eq(["1", "test", "2", %({"test"=>1})]) + expect(subject['args']).to eq(['1', '[FILTERED]', 'bar']) + end + end + + it 'properly flattens arguments to a String' do + hash_input['args'] = [1, "test", 2, { 'test' => 1 }] + + expect(subject['args']).to eq(["1", "test", "2", %({"test"=>1})]) + end end context 'when the job has a non-integer value for retry' do diff --git a/spec/models/alert_management/alert_spec.rb b/spec/models/alert_management/alert_spec.rb index 322ae0acf06..c7e21dddd53 100644 --- a/spec/models/alert_management/alert_spec.rb +++ b/spec/models/alert_management/alert_spec.rb @@ -7,6 +7,8 @@ describe AlertManagement::Alert do it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:issue) } it { is_expected.to have_many(:assignees).through(:alert_assignees) } + it { is_expected.to have_many(:notes) } + it { is_expected.to have_many(:user_mentions) } end describe 'validations' do diff --git a/spec/models/alert_management/alert_user_mention_spec.rb b/spec/models/alert_management/alert_user_mention_spec.rb new file mode 100644 index 00000000000..cce090a2231 --- /dev/null +++ b/spec/models/alert_management/alert_user_mention_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AlertManagement::AlertUserMention do + describe 'associations' do + it { is_expected.to belong_to(:alert_management_alert) } + it { is_expected.to belong_to(:note) } + end + + it_behaves_like 'has user mentions' +end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 3d204aa90c6..af3fdcfaa2e 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -829,6 +829,10 @@ describe Note do it 'returns commit for a commit note' do expect(build(:note_on_commit).noteable_ability_name).to eq('commit') end + + it 'returns alert_management_alert for an alert note' do + expect(build(:note_on_alert).noteable_ability_name).to eq('alert_management_alert') + end end describe '#cache_markdown_field' do diff --git a/spec/requests/api/graphql/project/alert_management/alerts_spec.rb b/spec/requests/api/graphql/project/alert_management/alerts_spec.rb index b27d3774c4f..fe84046d8e4 100644 --- a/spec/requests/api/graphql/project/alert_management/alerts_spec.rb +++ b/spec/requests/api/graphql/project/alert_management/alerts_spec.rb @@ -10,6 +10,8 @@ describe 'getting Alert Management Alerts' do let_it_be(:resolved_alert) { create(:alert_management_alert, :all_fields, :resolved, project: project, issue: nil, severity: :low) } let_it_be(:triggered_alert) { create(:alert_management_alert, :all_fields, project: project, severity: :critical, payload: payload) } let_it_be(:other_project_alert) { create(:alert_management_alert, :all_fields) } + let_it_be(:system_note) { create(:note_on_alert, noteable: triggered_alert, project: project) } + let(:params) { {} } let(:fields) do @@ -75,6 +77,8 @@ describe 'getting Alert Management Alerts' do 'updatedAt' => triggered_alert.updated_at.strftime('%Y-%m-%dT%H:%M:%SZ') ) + expect(first_alert['notes']['nodes'].first).to include('id' => system_note.to_global_id.to_s) + expect(second_alert).to include( 'iid' => resolved_alert.iid.to_s, 'issueIid' => nil, diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index a3262f2e9de..1701790c441 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -40,6 +40,7 @@ describe AlertManagement::Alerts::UpdateService do let(:params) { { title: nil } } it 'results in an error' do + expect { response }.not_to change { alert.reload.notes.count } expect(response).to be_error expect(response.message).to eq("Title can't be blank") end @@ -72,6 +73,14 @@ describe AlertManagement::Alerts::UpdateService do expect(response).to be_success end + it 'creates a system note for the assignment' do + expect { response }.to change { alert.reload.notes.count }.by(1) + end + + it 'adds a todo' do + expect { response }.to change { Todo.where(user: user_with_permissions).count }.by(1) + end + context 'with multiple users included' do let(:params) { { assignees: [user_with_permissions, user_without_permissions] } } @@ -80,10 +89,6 @@ describe AlertManagement::Alerts::UpdateService do expect(response).to be_success end end - - it 'adds a todo' do - expect { response }.to change { Todo.where(user: user_with_permissions).count }.by(1) - end end end end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index d6e867ee407..195783c74df 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -40,6 +40,12 @@ describe 'Every Sidekiq worker' do end end + it 'has a value for loggable_arguments' do + workers_without_defaults.each do |worker| + expect(worker.klass.loggable_arguments).to be_an(Array) + end + end + describe "feature category declarations" do let(:feature_categories) do YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).map(&:to_sym).to_set |
