Skip to content

CI: Fail jobs on RuboCop warnings

Peter Leitzen requested to merge pl-fail-on-rubocop-warnings-take-2 into master

What does this MR do and why?

In !106285 (merged) we add fail_on_warnings to detect warnings on RuboCop jobs and fail.

See suggestion gitlab-org/quality/engineering-productivity/master-broken-incidents#75 (comment 1167203415).

In #386259 (comment 1215776389) we decided to revert that functionality via !107442 (merged) to avoid flaky/broken master.

This MR reintroduces fail_on_warnings and moves the logic of ignoring known warnings from scripts/static-analysis to fail_on_warnings via scripts/allowed_warnings.txt. It also wraps the call scripts/static-analysis as fail_on_warnings scripts/static-analysis to take advantage of this change.

How to set up and validate locally

  1. Create a test script foo.sh
#!/bin/bash

warn() {
  echo "$*" >&2
}

echo "OK"
warn "Something else"
warn "Type application/netcdf is already registered as a variant of application/netcdf."
warn "Type DIFFERENT is already registered as a variant of application/netcdf."
warn "as long at it matchesType application/netcdf is already registered as a variant of application/netcdf.with a suffix"
warn 'Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`'
  1. Make it executable chmod a+x foo.sh
  2. Start a subshell via bash
  3. Add fail_on_warnings source scripts/utils.sh`
  4. Run foo.sh with and without fail_on_warnings
$ ./foo.sh
OK
Something else
Type application/netcdf is already registered as a variant of application/netcdf.
Type DIFFERENT is already registered as a variant of application/netcdf.
as long at it matchesType application/netcdf is already registered as a variant of application/netcdf.with a suffix
Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`
peter@happy ~/devel/gitlab/gdk/gitlab on (pl-fail-on-rubocop-warnings-take-2?)
$ echo $?
0
peter@happy ~/devel/gitlab/gdk/gitlab on (pl-fail-on-rubocop-warnings-take-2?)
$ fail_on_warnings ./foo.sh
OK
There were warnings:
Something else
Type DIFFERENT is already registered as a variant of application/netcdf.
peter@happy ~/devel/gitlab/gdk/gitlab on (pl-fail-on-rubocop-warnings-take-2?)
$ echo $?
1
  1. Patch scripts/static-analysis to run only foo.sh
Click to expand
diff --git a/scripts/static-analysis b/scripts/static-analysis
index 0d03dd42c73c..61396457ae7e 100755
--- a/scripts/static-analysis
+++ b/scripts/static-analysis
@@ -38,21 +38,7 @@ class StaticAnalysis
   # contain values that a FOSS installation won't find. To work
   # around this we will only enable this task on EE installations.
   TASKS_WITH_DURATIONS_SECONDS = [
-    (Gitlab.ee? ? Task.new(%w[bin/rake gettext:updated_check], 360) : nil),
-    Task.new(%w[yarn run lint:prettier], 200),
-    Task.new(%w[bin/rake gettext:lint], 105),
-    Task.new(%W[scripts/license-check.sh #{project_path}], 200),
-    Task.new(%w[bin/rake lint:static_verification], 40),
-    Task.new(%w[bin/rake config_lint], 10),
-    Task.new(%w[bin/rake gitlab:sidekiq:all_queues_yml:check], 15),
-    (Gitlab.ee? ? Task.new(%w[bin/rake gitlab:sidekiq:sidekiq_queues_yml:check], 11) : nil),
-    Task.new(%w[yarn run internal:stylelint], 8),
-    Task.new(%w[scripts/lint-conflicts.sh], 1),
-    Task.new(%w[yarn run block-dependencies], 1),
-    Task.new(%w[yarn run check-dependencies], 1),
-    Task.new(%w[scripts/lint-rugged], 1),
-    Task.new(%w[scripts/gemfile_lock_changed.sh], 1),
-    Task.new(%w[scripts/lint-vendored-gems.sh], 1)
+    Task.new(%w[./foo.sh], 2)
   ].compact.freeze
 
   def run_tasks!(options = {})
  1. Run scripts/static-analysis with and without fail_on_warnings
$ scripts/static-analysis
Total expected time: 2.0; ideal time per job: 2.0.

Tasks to distribute:
* ./foo.sh (2s)

Assigning tasks optimally.
Assigning ["./foo.sh"] (2s) to node #1. Node total duration: 2s.

Expected duration for node 1: 2 seconds
* ./foo.sh (2s)

$ ./foo.sh
==> Finished in 0.001152218 seconds (expected 2 seconds)


===================================================
Node finished running all tasks in 0.001185997 seconds (expected 2)


All static analyses passed successfully with warnings.

**** ./foo.sh had the following warning(s):
Something else
Type application/netcdf is already registered as a variant of application/netcdf.
Type DIFFERENT is already registered as a variant of application/netcdf.
as long at it matchesType application/netcdf is already registered as a variant of application/netcdf.with a suffix
Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`
peter@happy ~/devel/gitlab/gdk/gitlab on (pl-fail-on-rubocop-warnings-take-2 ?M)
$ echo $?
0
peter@happy ~/devel/gitlab/gdk/gitlab on (pl-fail-on-rubocop-warnings-take-2 ?M)
$ fail_on_warnings scripts/static-analysis
Total expected time: 2.0; ideal time per job: 2.0.

Tasks to distribute:
* ./foo.sh (2s)

Assigning tasks optimally.
Assigning ["./foo.sh"] (2s) to node #1. Node total duration: 2s.

Expected duration for node 1: 2 seconds
* ./foo.sh (2s)

$ ./foo.sh
==> Finished in 0.000920208 seconds (expected 2 seconds)


===================================================
Node finished running all tasks in 0.000970504 seconds (expected 2)


All static analyses passed successfully with warnings.

There were warnings:
**** ./foo.sh had the following warning(s):
Something else
Type DIFFERENT is already registered as a variant of application/netcdf.
peter@happy ~/devel/gitlab/gdk/gitlab on (pl-fail-on-rubocop-warnings-take-2 ?M)
$ echo $?
1

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Peter Leitzen

Merge request reports