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

Loading