Allow projects to easily configure linting jobs to fail the pipeline

It seems to be possible with https://docs.gitlab.com/ee/ci/yaml/#rulesallow_failure

@fjgarlin I think the question is just the other way round: how to get a pipeline to show red instead of green, when a lint job failed.

@alexpott we're doing that on all our modules with these lines in our gitlab-ci config:

eslint:
  allow_failure: false
phpcs:
  allow_failure: false
phpstan:
  allow_failure: false

That way, we get red widgets, when either of those jobs failed. Sample: https://git.drupalcode.org/project/eca/-/pipelines

@jurgenhaas yes that would work but I wonder if rather than doing that we should have variables as in the MR.

But as I wrote in the issue summary: This is compounded by the fact that drupal.org visually adds a green tick pipelines where there is a warning (which is another probably more important bug).

I understood it well from the beginning. That's not a problem. We can just default to true.

The suggested MR has invalid syntax.

It'll need something like

  rules:
     - if: '$_ALLOW_FAILURE_VALIDATE_JOBS == "1"'
        allow_failure: true

And then remove the line containing allow_failure in the job.

The MR is looking much better.

For the sake of consistency, we do have 1 job in the validate section for Drupal 7 and we should allow that too.

And also, as the changes are very much related, we also have currently allow_failure: true for the "next minor", "next major"... for "nightwatch" and "phpunit", so I wonder if we should tackle those too here while we are at it.

Pipelines are fully failing with defaults. eg: https://git.drupalcode.org/project/decoupled_pages/-/pipelines/110638 and https://git.drupalcode.org/project/keycdn/-/pipelines/110705

I think it might be due to the order. I'll try to switch the rule to the beginning.

I appreciate the work you've put into this, @alexpott, but I've got to question is this needed?

I've been using the syntax @jurganhaas posted in #3 on a couple of modules, and am currently implementing on Project Browser.

For me, this meets the requirement in the title, in that it's easily configurable. If you see it in a .gitlab-ci.yml file, it's immediately obvious what it's doing.

I'm not sure adding variables is as clear as a syntax, and it adds a "Drupalism" that doesn't seem necessary.

My other concern is will this break projects already using the existing technique? I don't know how many projects are using it, but if they all stop requiring linting overnight, it would be disruptive. If it doesn't break existing projects, then we have two syntaxes that do the same thing, which could be confusing.

For me the only thing that's needed is to better document the existing technique. It's in GitLab documentation, but I don't think it's currently mentioned in the Drupal document.

We should agree on the best approach, and add that to the document.

Changing the order of the rules made the difference. It works as expected now. eg: https://git.drupalcode.org/project/decoupled_pages/-/pipelines/111444 and https://git.drupalcode.org/project/keycdn/-/pipelines/111453

I think this is ready for further testing and review.

Agree as well with @lostcarpark at #9. Doing something like the syntax in #3 is extremely easy and I agree that we wouldn't be adding extra variables.

I also checked the documentation to see what happened if BOTH ways were set, and we'd actually be breaking behaviour.

The rule-level rules:allow_failure overrides the job-level allow_failure, and only applies when the specific rule triggers the job.

Due to this alone I'd be inclined to do a "Won't fix", since there is already an easy way to achieve it and we'd have no way to keep backward compatibility.

--

We do have a few issues already created to create/improve documentation of the different jobs. eg: #3423402: Document how to use the CSPELL job → , #3424756: Create a space to document how to solve common issues for each type of linter → and #3423238: Create a space to document how to customize each of the GitLab CI jobs → .

#11 is a good argument to close won't fix. I've ummed and ahhed when filing the issue. The thing that gives me pause if that we’re adding jobs all the time and maybe there will be a time when we want to use the same variable in multiple jobs.

But I think in this case #11 trumps that concern.

The Slack discussion https://drupal.slack.com/archives/CGKLP028K/p1735644012530259 brought me to this issue, which I had not really looked at before. I have an idea that would allow a similar approach as above but still maintain full BC with existing pipelines. The key difference is that we keep all the existing allow_failure: true in the jobs, not remove them as in the MR. Then we add two parts to the new rule:

    - if: '$PHPCS_ALLOW_FAILURE == "1"'
      allow_failure: true  
    - if: '$PHPCS_ALLOW_FAILURE == "0"'
      allow_failure: false  

The default value of the new variable would be blank, so that if it not added at all in the UI or custom .gitlab-ci then neither of those two parts are matched, and it reverts to the job-level setting (thus maintaining BC)

I think this could be a good feature to add, and would make it possible to achive the scenarios described by @berdir. So if there is interest in this, let me know and I will work on it.

If we are able to keep BC that would be ok, but if we are also going to work on this, how about also adding a variable to set all linting jobs to allow failure or not? I see that most maintainers will want to allow all or none, rather than individually, so if we are going to add the ability we can aim for both granular and global, while keeping BC.

jonathan1055 → changed the visibility of the branch main to hidden.

I have taken the existing MR and made some initial changes just for the stylelint and cspell jobs. There is one new variable $ALL_VALIDATE_ALLOW_FAILURE which can be 1 or 0, with blank as the default. The previously added variables now have blank as the default, and these can be used to override the 'all' variable, which itself can override the setting in the actual pipeline.

The new rules look like this:

- if: $STYLELINT_ALLOW_FAILURE == "1" || ($ALL_VALIDATE_ALLOW_FAILURE == "1" && $STYLELINT_ALLOW_FAILURE != "0")
  allow_failure: true
- if: $STYLELINT_ALLOW_FAILURE == "0" || $ALL_VALIDATE_ALLOW_FAILURE == "0"
  allow_failure: false

The reason for the extra check on stylelint != 0 in the first rule is so that stylelint=0 can override all=1.

One other slight annoyance (as we have discovered before) is that Gitlab Pipelines do not provide the "not exists" functionality when checking for files. We can only check for the positive existence of files, which means we can't have one rule at the top saying "if there are no .css files then skip this job". We have to include the positive check for .css files in every rule that can match, because as soon as one rule matches the subsequent ones are skipped. Hence the repetition of exists: '**/*.css' in each of the new rules.

The names of the variables are not necessarily finalized, and we also need to decide if they should start with _ or not. Also the descriptions may be improved, but setting to NR to hear any feedback. Testing to follow ...

Left some feedback. The approach looks good in my opinion.

Each validation/linting job now has two new variables which can affect the allow_failure setting, and each of these variables can take three values (blank, 0 and 1) so there are 3 x 3 = 9 combinations of the two variables for each validation job. We can test two jobs (cspell and stylelint) in each pipeline run, and the repo has a customised allow_failure:false for stylelint but no custom setting for cspell, hence it uses gitlab templates default of allow_failure:true.

In this first round of testing none of the new variables are defined in the committed .gitlab-ci.yml, they are only set in the UI form when starting the pipeline. The branch has skip=1 for everything except stylelint and cspell, just to make it easier to see. Both of the jobs write out the variables values in after_script.

I have made the equivalent changes to the rules in phpcs, phpstan and eslint. Other things that are done

1. created resuable snipptes .php-files-exist, .ccs-files-exist and .js-and-yml-files-exist to make the expanded rules more readable. These use the <<: syntax. They are not rules in themselves, so they do not end int '-rule' and have no when: clause
2. moved the new variables to be with the other variables for the same job, as I think this is more logical when filling in the UI form
3. removed unrelated comments from the previous approach, as these changes do not affect phpunit or nightwatch
4. the .php-files-exist-rule is not used anymore, because it has to be replaced with the three usages of php-files-exist. But I don't think we can delete it, as it is likely to be used in some customized .gitlab-ci.yml, therefore moved it to BC section of the file

This is ready for a full review and more testing.

Is there any reason why the 'composer-lint' job was missed out of this process in the previous approach? I see no reason why it should be different. Having the _ALL_VALIDATE_ALLOW_FAILURE variable control all the validate jobs except composer-lint just makes it tedious to explain. I'm going to add it, but can remove if there is a good reason that I have overlooked.

I have added the variable for composer_lint, and done some more testing, now on all six linting jobs.

The testing branch .gitlab-ci.yml now has eslint and stylelint with allow_failure:false and everything else unchanged from the gitlab templates default. I have introduced a fault that will make each job report a failure.

First, with no variables set in the UI - we get eslint and stylelint are red as per the variables, composer-lint is red as per the gitlab templates default, and the other three are amber as per the default
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/396538

_ALL_VALIDATE_ALLOW_FAILURE=1 via form - every job ends amber
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/396545

_ALL_VALIDATE_ALLOW_FAILURE=0 via form - every job ends red
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/396556

_ALL_VALIDATE_ALLOW_FAILURE=0, and _PHPCS_ALLOW_FAILURE=1 & _STYLELINT_ALLOW=1 via form
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/396580
Stylelint ends amber which shows that the rule with the UI variables override the direct allow_failure:false in the customised pipeline
phpcs ends amber showing that the variable overrides the gitlab templates default

Now commit _ALL_VALIDATE_ALLOW_FAILURE:0 and _COMPOSER_LINT_ALLOW_FAILURE:1 into the repo
No other changes in the UI
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/396584
composer-lint ends amber, showing that variables in the .gitlab-ci.yml override the defaults. All other jobs red as intended.

As above but _COMPOSER_LINT_ALLOW_FAILURE=0 and _CSPELL_ALLOW_FAILURE=1 set in UI
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/396596
composer-lint is now red, showing that the UI variable overrides the projects .gitlab-ci.yml variable
cspell ends amber showing that the UI variable for a specifc job take precedence over the .gitlab-ci.yml 'all' setting

I have tried to cover all types of test and overrides, but there is always more that could be done. Are there any other combinations that we need to check?

#18 💙
#19 Thanks for the improvements.
#20 Agree.
#21 Thanks for the extra tests. I'm not sure we really need to cover more combinations, at least that I can think of right now.

I've checked the code and I think all the logic and changes are solid. I can't think of anymore feedback to add.

Downstream pipelines running here: https://git.drupalcode.org/issue/gitlab_templates-3425446/-/pipelines/39...

I'll set it to RTBC for now. This was a huge effort and it offers a lot of flexibility. Thanks so much!

Thanks, yes it was a good improvement to work on. Particularly when I realised that maintainers would like their weekly or monthly scheduled pipelines to have a different configuration from the commited repo settings, this feature provides that. Also it is even better with your idea to have a new "all" variable to provide a single customized setting.

I'll merge it to "main" now and announce it on the #gitlab channel, in case some people which have "main" as reference want to try it. I will not tag it yet, and when I do, it will be in 1.7.0 as this is a new feature.

Thanks so much for the work here and the testing, it's great to have this flexibility while preserving the previous behaviour.

Automatically closed - issue fixed for 2 weeks with no activity.