Define extra rule to jobs that have "needs: composer"

This is now fixed and the MR can be reviewed.

A sample pipeline with this in action can be reviewed at https://git.drupalcode.org/project/eca/-/pipelines/76382

So, while this fixes the bug, the 3 jobs composer-lint, phpcs and eslint are not being executed either, although it would be possible in the context of e.g. next major. The question is, if we should make those 3 more flexible to pick one of the composer jobs, but not necessarily the composer job as that can be skipped.

I will probably merge #3412915: Redefine phpunit rules with reusable references → today, and this other issue makes the rules reusable, and this issue is a perfect example of where we can use them.

I'll mark it as "Needs work" as it will need rebasing and then using the new rules.

That's OK. But there is some config changes in the other issue, that are (hopefully) unintended. E.g. if SKIP_COMPOSER is set there, it also skips composer (next minor) and composer (next major). That is a major change and would no longer allow to only run against future Drupal versions but not the current, which is possible now if the bug of this issue is being resolved.

The use case for such a scenario is to test a future module version against a future Drupal version only, to be prepared early.

We talked about that change and thought that it made more sense this way and that it was more of a bug than a feature. Skip composer would just skip composer for all "composer" jobs.

Overriding that should be relatively easy with the reusable rules for those cases:

composer (previous minor):
  rules:
    - !reference [ .opt-in-previous-minor-rule ]
    - when: always

or even

composer (previous minor):
  rules:
    - when: always

It's also consistent with the way that the "skip_phpunit" and "skip_phpstan" works.

I have rebased the MR and added some more &skip-....-rules for some more jobs as a follow-up to #3412915: Redefine phpunit rules with reusable references → where we still used the previous method; just for consistency.

Also, I removed the new constraint from the composer (next minor) and composer (next major) jobs, so that one can skip the current Drupal version but test against a next minor and/or major version.

A sample pipeline for that can be seen at https://git.drupalcode.org/project/eca/-/pipelines/77360

I think, this doesn't break anything else but allows for what was possible before but was broken because of incomplete constraints for dependent jobs.

What currently doesn't work yet, are the phpcs and all the linter jobs. They currently all depend on the regular composer job only, but I'd like to make them dependent upon one of the composer jobs, not just the single one. What do you think?

Yeah, we only migrated the rules that were used 2 or more times, but I'm happy with the refactoring of the other rules for consistency and readability.

I still have my doubts about the other change. You see it as a feature that was available, I see it as a bug that was present and needed fixing, but I think both cases can be right too. So I see your point, if you want something for "next-minor", then by turning the flag for "next-minor" on, you should get it, regardless of what the "current" settings are.

In any case, I think that the code looks good, and I'm fine with the interpretation of the composer+phpunit going hand in hand here, so I'm going to mark it RTBC.

I don't know if we will have many more variations and we might need to review the rules, but we can leave that for a separate issue, if needed.

Thanks a lot @fjgarlin for supporting the idea. I've looked into it again and found 2 more areas, where I had to add another commit:

• The same, that applies to next Drupal releases should also apply to previous Drupal releases, if someone wants to test a module that only supports D7.
• I also found a nice way to make sure that PHPCS, STYLELINT, ESLIST, COMPOSERLINT and NIGHTWATCH can be executed, even if SKIP_COMPOSER is set to 1: I've added a re-usable rule .needs-composer-rule which is added to the base composer job to make sure that this always runs, if at least one of those depending jobs is enabled.

Setting back to NW for a hopefully last review.

D7? There is a whole separate file in the templates for Drupal 7. I don't think that we can get any kind of support for Drupal 7 by using the normal "includes/include.drupalci.main.yml". I don't see any code around this case either in the previous commit.

I like the extra rule to calculate whether to run composer or not, but I'm not sure if we should do it though. If a user sets SKIP_COMPOSER=1 and gets a failure in the jobs that need composer, that's probably expected and it will make the user change the variable value, but if a user sets SKIP_COMPOSER=1, and composer actually runs, that's not expected. I know that your suggestion is more robust, but I don't know if we should make it the code more robust around this, as we want maintainers to do a good CI setup.

D7? There is a whole separate file in the templates for Drupal 7

Confused myself with that. It's not about D7, it's just about a previous (modern) Drupal release. Just similar to next Drupal release.

If a user sets SKIP_COMPOSER=1 and gets a failure in the jobs that need composer, that's probably expected

Well, the problem is, if you do this, the pipeline wouldn't even start because it's invalid. See https://docs.gitlab.com/ee/ci/yaml/#needsoptional :

If a needed job has optional: false, but it was not added to the pipeline, the pipeline fails to start with an error similar to: 'job1' job needs 'job2' job, but it was not added to the pipeline.

Optional defaults to FALSE and that's why our pipelines would fail with that error message. That's how I actually came to this issue in the first place.

So, if either of the jobs that needs composer is enabled, then composer has to run. That's why I think this change should actually be made. But I can see the potential confusion that you describe. To resolve this, we could create a new job with a label like "Composer for linters" that only runs in these special circumstances and with that different label, it should be understandable why that job was run. It's not the one that is skipped because of the SKIP_COMPOSER variable.

Would that make sense?

I think it might be just easier/simpler to:
- Leave the rules as you suggest
- Do a check inside the composer job for SKIP_COMPOSER and if it's set to 1, then show output explaining why it's being run (ie: "[WARNING] Composer step is running because one of the required jobs needs it."). We can do that check here: https://git.drupalcode.org/project/gitlab_templates/-/blob/1.0.x/include... (before or after that line).

Good idea. We also need to check the CI_JOB_NAME there; otherwise the warning may get printed too often.

I've been following through this conversation, and the idea in #15 (to run composer even if skip_composer=1 but give a big message) seems to be a good solution to both sides of the argument. Having a job run when you explicitly have set it not to run would be confusing, but if there is a clear message why then it is all explained.

I can test the latest changes, but it seems the MR is not the usual naming? I think I need to use ref: 1.0.x not ref: nnnnnnn-the-branch-name which we often have for MR branches.

Test on current commited repo - lots of jobs failed, no pipeline ran
https://git.drupalcode.org/project/scheduler/-/pipelines/77983

Test using this MR

  - project: issue/gitlab_templates-3414391
    ref: 1.0.x

https://git.drupalcode.org/project/scheduler/-/pipelines/78020

All seems OK. Just a suggestion, the message is not very prominent. Could we use ********** to box around it, like in https://git.drupalcode.org/project/gitlab_templates/-/blob/1.0.x/include...

Yes, it seems that issue fork handling is nowadays far less automated so it's up to the individual to name things like project and branch.

I'm surprised you got it working by editing the include in the gitlab-ci file, since to my experience, we need to set a project variable like this to make it work correctly:

_GITLAB_TEMPLATES_REPO; issue/gitlab_templates-3414391

This is because certain parts of the templates use that variable to load more components from that repository.

All seems OK. Just a suggestion, the message is not very prominent. Could we use ********** to box around it

Sure, good idea. I've just updated the MR.

Yeah, for many issues, editing the ".gitlab-ci.yml" file as @johathan1055 says, it's more than enough. Some issues, especially ones dealing with the symlink script or the composer building php script, need @jurgenhaas suggestion. So both would work for most cases, and in some trickier ones we'd need to go with @jurgenhaas approach.

Just ran again. The message looks good
https://git.drupalcode.org/project/scheduler/-/jobs/643275

I should have thought of this before, but the default position of the gitlab log is fully scrolled to the end when you first visit the log. Therefore it could easily be missed. So I think it would be good to repeat the message as the last line of the script: in addition to where it is at the start.

Could also expand the message with the $SKIP_COMPOSER in the middle of the text, so devs know what they have done to cause it. Something like:

printf "**********\n* [WARNING] Composer step is running even though \$SKIP_COMPOSER = $SKIP_COMPOSER because one of the required jobs needs it.\n**********\n";

I could get push access and do this, but it's probably quicker to leave it to you, then we don't have to negotiate clashing/overlapping.

That's true, and I have added that second instance of the warning just now. Also added a line break in the message, which should make it even more visible.

Personally, I think, we can do whatever we want, there will be many instances where the warning isn't noticed, still.

Nice. Tested here https://git.drupalcode.org/project/scheduler/-/jobs/644320
All good from me.

Tested again, skipping everything except phpunit

variables:
  SKIP_COMPOSER: 1
  SKIP_COMPOSER_LINT: 1
  SKIP_ESLINT: 1
  SKIP_PHPCS: 1
  SKIP_PHPSTAN: 1
  SKIP_STYLELINT: 1
  SKIP_PHPUNIT: 0
  OPT_IN_TEST_MAX_PHP: 0
  OPT_IN_TEST_PREVIOUS_MINOR: 1
  OPT_IN_TEST_PREVIOUS_MAJOR: 0
  OPT_IN_TEST_NEXT_MINOR: 0
  OPT_IN_TEST_NEXT_MAJOR: 0
 

The pipeline failed, could not be created. Is this what is expected? All the linting jobs can force Composer to run even when $SKIP_COMPOSER=1 but the PHPunit job does not have that power, and the pipeline just fails to be built. That might be what is intended, just checking
https://git.drupalcode.org/project/scheduler/-/pipelines/78310

I'd say that we need to add that case to the list that triggers the automatic run. Good find!

@jonathan1055 that pipeline failed for a different reason: you override the definition of the phpunit job in your gitlab-ci which didn't repeat all the rules that the original one had.

However, there was an inconsistency left: PHPUNIT and PHPSTAN had still been forced to be skipped when composer was skipped. I guess that was because it seemed more obvious that those require composer as they are PHP-based tasks, whereas for linters, this wasn't that obvious to most users.

But making such assumptions doesn't really make sense, I agree. So I've now updated the template such that it behaves similarly for all jobs that need composer:

• COMPOSER_LINT
• PHPSTAN
• PHPUNIT
• PHPCS
• STYLELINT
• ESLINT
• NIGHTWATCH

@jonathan1055 that pipeline failed for a different reason: you override the definition of the phpunit job in your gitlab-ci which didn't repeat all the rules that the original one had.

I don't know if you clicked "go to pipeline editor" button on the page I linked to which had the "pipeline cannot be created" error. But that 'pipeline editor' gives the committed main branch gitlab template, the template for the MR which just failed. Maybe you knew that anyway.

The MR for the pipeline that failed has this:

phpunit:
  <<: *scheduler-matrix
  rules:
    - !reference [ .skip-phpunit-rule ]
    - *phpunit-scheduler-rule

It failed because Composer was skipped and there was no override to make Composer run when Phpunit is not skipped. It needs the two extra conditions that you have added in the last commit.
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/108...

I will re-run and check, but I think your change will have fixed this. Thanks.

MR95 re-run and the pipeline is created as expected.
https://git.drupalcode.org/project/scheduler/-/pipelines/78785

Note that the development branch is now "main", as per #3404175: Adopt (semver) versioning for gitlab_templates → , so the target of the MR needs to change. Thanks.

Also I tried to test just for a 'next' version and skip the 'current' setting, to replicate the test in #9
https://git.drupalcode.org/project/eca/-/pipelines/77360
and comment in #10

So I see your point, if you want something for "next-minor", then by turning the flag for "next-minor" on, you should get it, regardless of what the "current" settings are.

but it no longer works. See
https://git.drupalcode.org/project/scheduler/-/pipelines/78818

I think this was jurgenhaas's original requirement. If this is still wanted, it looks like we just need to delete all the *skip-phpunit-rule for all the phpunit variants. The they will be run only on opt-in.
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/108...

stale form data. should be 'needs work'.

@jonathan1055 I can't reproduce that problem in #30. I have this pipeline (https://git.drupalcode.org/project/eca/-/pipelines/79166) with these variables:

variables:
  SKIP_COMPOSER: '1'
  OPT_IN_TEST_NEXT_MAJOR: '1'

That runs all the expected jobs.

This is with CI variables defined in the project like these:

_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3414391
_GITLAB_TEMPLATES_REF: 1.0.x

I then checked the settings for this pipeline (https://git.drupalcode.org/project/scheduler/-/pipelines/78822) and saw that all jobs are skipped:

  SKIP_COMPOSER: 1
  SKIP_COMPOSER_LINT: 1
  SKIP_ESLINT: 1
  SKIP_PHPCS: 1
  SKIP_PHPSTAN: 1
  SKIP_STYLELINT: 1
  SKIP_PHPUNIT: 1

That's actually what I intended here. But that helped me understand, why PHPSTAN and PHPUNIT didn't have the power to enforce composer before we changed that in #26. I think we should revert that. Here is why:

These jobs only ever run once, and they require the composer job:

  SKIP_COMPOSER: 1
  SKIP_COMPOSER_LINT: 1
  SKIP_ESLINT: 1
  SKIP_PHPCS: 1
  SKIP_STYLELINT: 1

That's why they need to enforce the composer job when they are enabled; otherwise they couldn't run.

On the other hand, the following jobs run for each enabled release, when they are enabled:

  SKIP_PHPSTAN: 1
  SKIP_PHPUNIT: 1

So, those should not enforce the composer job, they just work on composer, when it's not skipped, and in addition to that on all opted in composer jobs, next or previous.

Therefore, I have reverted that enforcement and this is available for review again.

@fjgarlin I have also changed the taret branch and rebased the MR.

After the latest change, I'm getting this pipeline (https://git.drupalcode.org/project/eca/-/pipelines/79176) which enforces composer because of PHPCS, etc. but PHPSTAN and PHPUNIT only runs on the next major. That's what I originally wanted to achieve. Hope this is what you're going to get as well in your project's tests.

I've retested the same MR95 as before, and I only get the 'Composer next major' job, no phpunit.
https://git.drupalcode.org/project/scheduler/-/pipelines/79224

The template has, you saw above:

variables:
  SKIP_COMPOSER: 1
  SKIP_COMPOSER_LINT: 1
  SKIP_ESLINT: 1
  SKIP_PHPCS: 1
  SKIP_PHPSTAN: 1
  SKIP_STYLELINT: 1
  SKIP_PHPUNIT: 1

  OPT_IN_TEST_MAX_PHP: 0
  OPT_IN_TEST_PREVIOUS_MINOR: 0
  OPT_IN_TEST_PREVIOUS_MAJOR: 0
  OPT_IN_TEST_NEXT_MINOR: 0
  OPT_IN_TEST_NEXT_MAJOR: 1

So I am effectively trying to do what you are doing, that is I just want PHPunit on the next major, not the main branch. The reason yours works, I think, is because you only set

variables:
  SKIP_COMPOSER: '1'
  OPT_IN_TEST_NEXT_MAJOR: '1'

Which means that SKIP_PHPUNIT takes the default value, which is 0 not 1. You correctly do not get the basic phpunit job, but it is because you have SKIP_COMPOSER=1. If you added SKIP_PHPUNIT=1 you would not get the PHPunit next major job either. I'm fine with this, and I think it is actually what we had before, that is, setting SKIP_PHPUNIT=1 will skip phpunit jobs. To get just a PHPunit variant via OPT_IN (and not get the basic phpunit) it needs SKIP_COMPOSER=1 but SKIP_PHPUNIT set to 0.

PHPStan should follow the same pattern, but I'm not sure it does yet, so I will check that too.

I tried not skipping phpunit skip_phpunit=0 and the pipeline failed to get built:
https://git.drupalcode.org/project/scheduler/-/pipelines/79242

It needs at least one of the jobs that can force composer. I changed skip_eslint from 1 to 0, and this worked. This is what you also have, by default.
https://git.drupalcode.org/project/scheduler/-/pipelines/79254

Yes, those samples do behave as intended and your tests and description is correct. I could describe it in other words as well, maybe that makes it a bit clearer:

• If you want to run any linters (composer lint, phpcs, eslint, style lint, etc.), just set/keep their SKIP variable at 0. No matter what's configured for any other stuff-
• If you want to run any tests (phpstan, phpunit, etc.), set/keep their SKIP variable at 0 and those tests will then run on all enabled composer variations (current, previous, next)

Having said that, I wonder if lighthouse wouldn't fall under linter (as it is handled now) but should be moved to the test category. But that would be out of scope for this issue, since that requires more changes to build jobs for the various composer variations first, which isn't available either.

Thanks, yes with the current MR we can achieve what you wanted, to run 'phpunit next major' without having to run the basic composer. So it all works OK.

The only thing missing from your two bullet points, and the slight anomoly in the variables, is that if you dont want the basic phpunit but do want one or more of the op-in phpunits, you need to set skip_composer=1. That is not very intuitive, so it will need to be expained somewhere.

The problem is that we have 'opt-in' variables for all the variants, but the only way to "opt out" of the basic phpunit and phpstan is to have skip_composer=1. An idea just dawned on me, if we had an 'opt in' for the current core version testing, that is set to 1 by default, this might solve the problem. Setting OPT_IN_TEST_CURRENT=0 would mean that the current composer would not get run (unless forced by the linters) and that would mean the current phpunit/phpstan would not get run as required. That is more intuitive that setting skip_composer=1. I think it is a small change, building on what we've got so far, but making it more understandable and user-friendly. I can try this out on the MR if you think it is a good idea.

Couldn't agree more. However, this would be a breaking change since everybody already using the templates relies on the SKIP_COMPOSER and the OPT_IN_* variables. Don't think we can change that easily now, although I wish we did it your way right from the beginning.

Trying to get the variables more in-line with each other should then be a separate issue, I think.

I've now also updated the release numbers to fix the pipeline error.

I don't think it would be a breaking change, at least not how I envisaged it. Let me look at it again tomorrow, and test, or are you in a hurry to commit this?

I don't think it would be a breaking change

I'm interested to see how that can be possible. Since the existing SKIP_COMPOSER can't be removed but introducing a new variable OPT_IN_TEST_CURRENT would be redundant, how should a script then be deterministic if anyone uses both of them in conflicting ways?

I'm keeping an eye on the back and forth, thanks both!!

Re

I've now also updated the release numbers to fix the pipeline error.

You can just rebase, I fixed/merged those values yesterday.

I just want to try out some ideas. I'm not saying we can do this, or that it will be better, but I'd like to see the differences, so I'm going to create a separate MR. Then we can test certain scenarios on both, and compare them again the current "main". I accept that it may be a dead-end.

Maybe we are overcomplicating things?

Would a more simpler approach of:
- SKIP_* variables will skip that job for all variants
- OPT_IN_* will try to run the available jobs in the variant specified

As a new case, which is "current", which also is what we default to, we can introduce the suggested OPT_IN_TEST_CURRENT and set it to 1.

The "needs-composer-rule" is useful and that relies on the SKIP_* variables.

So with this scenario, we'd have full control about which jobs will be run, and which variant.

Maybe I'm missing something but I think that'll sort the situation without breaking changes. Skip will skip for all, opt_in will add a new variant for the non-skipped jobs.

Maybe I'm missing something but I think that'll sort the situation without breaking changes. Skip will skip for all, opt_in will add a new variant for the non-skipped jobs.

Agreed. The only remaining question to me: what if SKIP_COMPOSER and OPT_IN_TEST_CURRENT are both set to 1? Which one wins?

In that case, OPT_IN_TEST_CURRENT=1 will qualify the job to be run it, but if SKIP_COMPOSER is set to 1, then it should skip it (unless we force it via the “needs-composer-rule”).

In that example, it won't run, unless forced by needs-composer-rule. As I see it then, SKIP_COMPOSER always supersedes OPT_IN_TEST_CURRENT. IN that case, we don't need that extra variable, it won't have any power, ever.

Yes, you are right, in that case.

But if you reverse it, you can have OPT_IN_TEST_CURRENT=0 (non-default) and SKIP_COMPOSER=0 (default), and you can get full runs for the variants (next minor, next major...), which would solve the main problem presented in #6.

...allow to only run against future Drupal versions but not the current...
The use case for such a scenario is to test a future module version against a future Drupal version only, to be prepared early.

So just by doing OPT_IN_TEST_CURRENT: 0 you'd be getting the future versions, but not the current one. That's where I see that the simpler approach would make it more flexible.

This gets confusing now. SKIP_COMPOSER 0 or 1 does not have any impact on tests for next or previous versions. They are controlled by their OPT_IN_ variables only.

I think, all of the use cases, we had in this issue, are already resolved. There is no need for further improvements regarding functionality. Also, #6 is resolved by SKIP_COMPOSER=1 and opting in for next major.

What came up in #37 is about a potential variable name confusion, where current composer is controlled by a SKIP variable, but previous and next composer is controlled by OPT_IN_ variables. Addressing that would not add functionality that's not already there.

Ok, I think I kind of went back to the early discussion with that suggested approach.

I still like the simplicity of the suggestion in #44, where you can toggle on/off tasks for all variants and you can toggle on/off full variants (including current). I think it can make the code simpler and easier to understand/maintain. But this could be a future issue if needed.

I don't want to interfere with the current back and forth in this issue between you two, so feel free to disregard the above exchange since #44.

I did say that I had some ideas, and wanted to try them out, so please let me do so. I have had time to make some changes locally for this, pretty much exactly as described above I think, but I had my mail and notifications off, so did not see any of the above conversation. I worked it all out, I think it will provide the new functionality needed and not break anything, but can't test locally, so would like to push it here. I had to create a new branch locally from 'main' because the UI 'create branch' here does not have 'main' as the target. How do we get that added to the issue for here? I can try to push my branch, but it will probably conflict with the '1.0.x' base which is the only target available here.

Please push Jonathan. I'd love to see your ideas on this.

You can create your branch, rebase from the "main" canonical repo and then push your branch to this fork. The final MR, which is what you need to worry about, should be fine and show the right changes. The target "main" should show up when creating the MR.

No worries @fjgarlin, I think it's important to pick on the details and your participation in this is more than welcome, too.

Still, I don't get the simplification yet. In #49 I wanted to make sure that we all know, that functionality is now provided, and the naming clean-up is what we should try to get at, but I don't see that happening by providing a new variable which conflicts with an already existing one.

The second example in #48 to turn off current tests with OPT_IN_TEST_CURRENT: 0 makes the variable SKIP_COMPOSER redundant. To me, that would be the ideal solution, but I consider that a breaking change. It would look like this:

• OPT_IN_TEST_* determines, which Drupal versions get tested. Which tests will be performed is determined by SKIP_* for phpstan and phpunit. So, the combination of those 2 sets of variables creates a matrix for what gets tested.
• Then there are SKIP_* variables for linters (phpcs, stylelint, eslint, etc.) which determine which of those tests/linters will be run. If at least one is enabled, the current composer needs to run.

Those 2 rules describe all possible scenarios. And the SKIP_COMPOSER variable is not needed for that.

So, I'd suggest, if we introduce OPT_IN_TEST_CURRENT, then we should deprecate SKIP_COMPOSER.

Sorry, posted this without seeing the 2 previous comments that came in while I was writing. I didn't mean to discourage a new MR, I just wanted to provided a clear technical description for what we hopefully find an agreement upon, because that also needs to be documented then.

I'd be 100% ok with #53 (pending review of Jonathan's suggestion).

In terms of deprecation, I'd just change the description and add a message informing maintainers to stop using it in favor of the new one. I think this skip-variable is probably never turned off, since that will prevent the installation of the required code for all the other tasks.

The only use case where "composer" is not needed is for the GitLab "pages", but that job does not depend on any of the opt_in or skip variables, so that's not a problem.

I'd be 100% ok with #53

Sounds good to me. I had a quick look into MR!112 but that feels a bit further away from what we've already achieved in MR!108. If removing SKIP_COMPOSER wasn't a problem, as I understand #56, then that would be a relatively small change in MR!108 and I'd be happy to provide that. But waiting for Jonathan's input on that too before I move on.

Testing is underway. The new MR no longer has the needs-composer-rule to force composer. With the new opt_in_current I see no need to force composer when skip_composer=1. When setting skip_composer=0 of the jobs that need composer are now skipped, by adding the skip-composer-rule to them all. The only job that runs when skip_composer=1 is Pages, which can be seen here:
https://git.drupalcode.org/project/scheduler/-/pipelines/79837 (I just forced a pages job, do not have the file so ignore the red)

This is (currently) the only useful scenario where skip_composer=1 is ever needed. But there could be more, so we do not need to deprecate the variable or change it in any way.

Second test run, with skip_composer=0, skipping all except slint=0 and phpunit=0
https://git.drupalcode.org/project/scheduler/-/pipelines/79840

I have to leave now, but back later to test the rest of the scenarios.

I’m out now as well. I like #58 suggestion (from reading the comment only) as well.

I’ll look at the code in around 2h from now.

We can then see pros/cons of both suggestions.

I'm really confused, sorry. There are too many cases. I think we need a spec and/or end user documentation that cover all those cases first; otherwise we're not getting anywhere.

Agree. We should do a table/matrix with options available now vs suggested and the result to see the real change.

I can help with that later.

Here is a first attempt, the document can be added collaboratively or downloaded here: https://cloud.lakedrops.com/index.php/s/n2GBe7QAdcxrdwR

The yellow values are the defaults. The red blocks don't execute, but the green ones do.

So, by default we get all the tests for current. And with the given parameters, everything can be turned on or off.

This approach seems to be easy to understand by end users, it has consistent variable names and it doesn't require SKIP_COMPOSER.

Thanks for creating the cloud doc, that looks helpful. Below is a summary of my testing to far. All scenarios have run the expected jobs, so I have not found any problem yet. I have not updated the new doc as I am not sure how Jurgen was intending for the test results to be displayed.

SKIP_COMPOSER=1, no other variables
https://git.drupalcode.org/project/scheduler/-/pipelines/79911
Correct: Nothing is run except 'Pages' as that does not need Composer

All the follwing tests below have SKIP_COMPOSER=0. I have put the details in the commit message, which you can see at the top of the linked pipeline page.

SKIP_ESLINT=0, SKIP_PHPUNIT=0, OPT_IN_TEST_NEXT_MAJOR=1
https://git.drupalcode.org/project/scheduler/-/pipelines/79840
As expected: two composer jobs, eslint and two phpunit jobs. The current phpunit is run by default, and the extra one is due to opt_in_next_major

SKIP_ESLINT=0, SKIP_PHPUNIT=0, OPT_IN_TEST_NEXT_MAJOR=1, OPT_IN_TEST_CURRENT=0
Intention: dont run phpunit on the current branch, only next major
https://git.drupalcode.org/project/scheduler/-/pipelines/79863
As expected: two composer jobs, eslint and just PhpUnit Next Major. Standard Phpunit is not run because OPT_IN_TEST_CURRENT=0

SKIP_ESLINT=0, SKIP_PHPUNIT=0, OPT_IN_TEST_NEXT_MAJOR=0, OPT_IN_TEST_CURRENT=1
Intention: change phpunit from next major to current
https://git.drupalcode.org/project/scheduler/-/pipelines/79871
As expected - one composer, eslint and one phpunit (current). The phpunit next major is not run.

ESLINT=0 PHPSTAN=0 PHPUNIT=1 NEXT_MAJOR=1 OPT_IN_CURRENT=0
Intention: dont run any phpunit jobs. Try phpstan only on next major
https://git.drupalcode.org/project/scheduler/-/pipelines/79879
correct: two composer, eslint and phpstan next major

ESLINT=0 PHPSTAN=0 PHPUNIT=1 NEXT_MAJOR=0 OPT_IN_CURRENT=1
Intention: Swap phpstan back to current only
https://git.drupalcode.org/project/scheduler/-/pipelines/79883
correct: one composer, eslint and phpstan

ESLINT=0 PHPSTAN=0 PHPUNIT=0 NEXT_MAJOR=1 OPT_IN_CURRENT=1
Intention: Run phpstan and phpunit jobs, on both current and next major
https://git.drupalcode.org/project/scheduler/-/pipelines/79889
correct: two composer, eslint, two phpunit, two phpstan

Everything default, no skip or opt-in variabes
https://git.drupalcode.org/project/scheduler/-/pipelines/79892
Correct: 1 composer, 3 x lint jobs, phpcs, 1 phpstan (on current), and 1 phpunit (on current)

Next I will write a short paragraph on how we describe how to use the SKIP and OPT_IN variables. I think it will be quite simple now.

Here is a first (very rough) draft of how we explain the testing config. Some of this is already in https://www.drupal.org/node/3356364/#s-variables-and-when-to-override-them → so we will need to update that page. But I wanted to make sure we all understand and agree on how it works now, with MR112

• PHPUNIT and PHPSTSAN tests can be run on a variety of core versions (previous, current and next) and Max PHP. The default is to run only on the current core version. See the OPT_IN_TEST_{} variables for how to add or remove variants.
• ESLINT, COMPOSER_LINT, STYLELINT, PHPCS and NIGHTWATCH are only run on the current core version
• Any jobs can be skipped using the SKIP_{} set of variables. These are all set to 0 by default, so that all jos will be run if appropriate.
• Setting SKIP_PHPUNIT=1 will skip all PHPUnit variants, overriding the settings of OPT_IN_TEST_{}. Likewise with SKIP_PHPSTAN
• SKIP_COMPOSER=1 means that no Composer jobs will be run, and therefore every job that needs Composer will also not be run. Currently this leaves just the Pages job, as this does need Composer

I think we keep coming back to the same thing that triggered this issue to start with:

SKIP_COMPOSER=1 means that no Composer jobs will be run, and therefore every job that needs Composer will also not be run. Currently this leaves just the Pages job, as this does need Composer

I kind of see it the same way "no composer means no composer anywhere", but @jurgenhaas sees it differently (see #6 or even why the issue was created).

I also like that, on that approach, that we are not force-running composer. If you decide to skip, it will skip, but obviosly jobs that depend on it cannot run. The "needs-composer-rule" was a workaround for that different point of view, but I also think it complicates the code a bit.

I am going to follow up with a comment about both suggested MRs with what I believe are pros and cons about each.

In any case, I shouldn't focus on the MR specifics right now, we need to all agree on the goal to achieve, as that should drive the solution.

We need to agree on what needs to run when, and then iterate from there, regardless of what’s on the current implementation or any of the MRs. The goal is the most important part now.

Thank you both for all of this. To get us started, here is my suggestion for the goals:

Goals to achieve

1. Allow PHPUNIT and PHSTAN to be run on any combination of the core version variants and the current version. This includes the scenario of not running at the current version (which is not possible right now, and is what caused this issue to be created)
2. Independently of the version variants that PHPUNIT and/or PHPSTAN are being run at, allow ESLINT, COMPOSER_LINT, STYLELINT, PHPCS and NIGHTWATCH jobs to be run. These all use the current core version, there are no variants
3. Allow any job (or sets of job variants) to be skipped. Skipped jobs should not be forced internally to run. "Skip means Skip"
4. Allow the Composer job(s) to be skipped so that just jobs which do not depend on Composer can be run in isolation (e.g Pages)

Here's my summary of MR112 and the key points that it addresses. I should maybe have written this earlier, explaining the changes from MR108. I could have just taken MR108 and pushed the changes to it, but I thought that would not be polite, and also having both means we can test a scenario against both. When I started the new changes I was not certain it would all work, but I think it does.

Summary of MR112

1. To solve the problem of wanting to run phpunit and phpstan on alternative varaints but not the 'current' core version we introduce a new variable OPT_IN_TEST_CURRENT
2. The default value is 1 to match the existing behavior, but it can be set to 0 to avoid running phpunit and phpstan at the current core version
3. A new new re-useable rule *opt-in-current-rule is added to .phpunit-base and .phpstan-base
4. The SKIP_ variables take precedence over the OPT_IN_ variables in every case, no exceptions. SKIP_{something} will mean that all the {something} jobs will be skipped, and accordingly so will all jobs that need the {something}
5. A key point is that whereas the Composer variants all contain the appropriate *opt-in-{x}-rule so that they only run when needed, the main Composer jobs does not have *opt-in-current-rule. This means that the 'current' composer job is always run so that the LINT/PHPCS/NIGHTWATCH jobs that need it can run
6. There is a possibility that this 'current' Composer job is executed when not actually required, in the scenario that skip_eslint, skip_composer_lint, skip_stylelint, skip_phpcs and skip_nightwatch are all set to 1 and opt_in_test_current is set to 0. To avoid this overhead (or to save it failing if for some reason it is not designed to be run with the current core version) it would be easy to add a rule within .composer-base to test for all these variables and set 'when: never'.
7. MR112 was built on MR108 taking the bulk of it (all the best bits) but removing the internal forced running of Composer, in favour of a new 'opt_in_test_current' variable, which simplified the processing, and made the code more understandable.

I may add the extra rule mentioned in 6 above, just for completeness and safety. I'm happy to fill in the google sheet or the icloud sheet, which ever you would like. I will document my own testing of MR112 anyway so that I have a record of what I've done.

For the sake of linking my own thoughts too. This is without looking at any MR, just what I think, and it's not even a final proposal.

Instead of a matrix, is just a list of jobs and the rules that will trigger them or not.

See here.
https://docs.google.com/spreadsheets/d/1vgOIzyjQP2-lytqoqPcBdhXXGXKaEy_X...

I have all the jobs listed and what I think the rules could be, with all available variables listed underneath and also possible issues with the approach and suggested fixes.

I think, the goal definition in #68 is a great starting point, and we should focus on that to be finalized before going into implementation details. So here are my comments on that:

Allow PHPUNIT and PHSTAN to be run on any combination of the core version variants and the current version. This includes the scenario of not running at the current version (which is not possible right now, and is what caused this issue to be created)

Yes, agreed, just one question for clarification: what exactly means "run on any combination of the core versions" mean? Is it e.g. PHPSTAN to be run on current and next major while PHPUNIT to be run on all core versions? Or does it mean that if PHPSTAN test is turned on, it will be tested on all enabled core versions, and the same for PHPUNIT.

Independently of the version variants that PHPUNIT and/or PHPSTAN are being run at, allow ESLINT, COMPOSER_LINT, STYLELINT, PHPCS and NIGHTWATCH jobs to be run. These all use the current core version, there are no variants

Yes, just that NIGHTWATCH should be shifted over to the first "camp" of PHPSTAN and PHPUNIT, as it should be possible to run nightwatch on different versions, whereas linters are static code analysis tools that really only need to run once. Unless somebody argues that it should be possible to e.g. run PHPCS against different PHP versions, which might be a meaningful requirement.

Allow any job (or sets of job variants) to be skipped. Skipped jobs should not be forced internally to run. "Skip means Skip"

Yes, but "Don't skip means don't skip" is equally correct. So, that means we have to define our variables such that we don't get conflicting scenarios like e.g. SKIP_COMPOSER=1 and SKIP_PHPCS=0, because in that scenario one of the instructions will be ignored, and that's no good.

Allow the Composer job(s) to be skipped so that just jobs which do not depend on Composer can be run in isolation (e.g Pages)

Yes.

Thanks Jurgen. Good questions, those do need clarification.

PHPSTAN and PHPUNIT ... run on any combination of the core versions

I meant as current, ie they both use the same combinations of versions, selected by the OPT_IN variables. I don't think we need to cater for more variations.

Yes, for Nightwatch there is an open issue to have the variants #3397270: Nightwatch testing against all opted in versions →

"Don't skip means don't skip" is equally correct

Yes, and but I tried to explain that in the 4th point. When you skip Composer you would only do that if you dont want composer and all of the jobs that depend on it. So SKIP_COMPOSER=1 would override any setting of SKIP for the jobs, eg PHPCS, that need it. I think dev's would not be surprised at that, even if they set SKIP_PHPCS=0

These clarifications are good, and MR112 currently follows all of this.

Point 1. Agree that phpunit, phpstan (and nightwatch as mentioned - after #3397270: Nightwatch testing against all opted in versions → ) should be able to be run per variant.

Point 2. We don't have those variants yet (except the wip for nightwatch linked above), so let's not worry about those yet. In any case, I think we're setting the logic on how to get these in the future, if needed.

Point 3. Both points of view are right. However, we give a "working base" by default, and you need to set the skip jobs, so probably the "skip means skip" might be the "expected" one.
- A possible suggestion might be to create a "Debug: composer disabled" (or "MR debug") job, where we just "echo" that all jobs that depend on "composer" won't run unless the variable is set back to 0. However, that'll show an unnecessary job in the pipeline (maybe we limit it only to MRs...).
- Not doing the above would be correct too, I think, as I also think that somebody skipping composer won't be surprised to not have phpcs or related tools shown.

Point 4. Yes too.

If it's a conscious decision to have the same core versions for PHPSTAN, PHPUNIT and NIGHTWATCH, I think that's OK.

WRT to the SKIP topic, I strongly disagree. If a user defines SKIP_PHPCS=0, they should not expect that to be ignored. And it is so simple to achieve by removing that SKIP_COMPOSER variable completely, as it is not in line with everything else, that's already there. Replacing that with OPT_IN_TEST_CURRENT does such a beautiful job and everything would be so logical, easy to document, and would never have any discussions, which perspective is right or wrong.

I also think that #73 might be the easiest. Adding OPT_IN_TEST_CURRENT would give us all we need to skip the current variant, and deprecating SKIP_COMPOSER might be really useful, as "composer' is just a base dependency for some jobs. That means that if "phpcs" or "phpunit" is required to run, then "composer" should just run, no questions asked.

I'd be ok deprecating SKIP_COMPOSER as the only legit use so far, would have been to skip the current version, and that'd be achieved by the new variable.

All three of us agreed on #73.

We need to:
- Remove / deprecate the SKIP_COMPOSER variable. Add message in "composer" job
- Change the rules to run the composer job
- Introduce the new OPT_IN_TEST_CURRENT variable and add the logic in the right places

Each of the suggested MRs is close to the above, but still needs to iterate to get there, so I'm marking this as "Needs work".
Once we have a proposal or proposals, we can compare and see if all the goals are achieved.

Thanks!

So, this seems to be looking good now. I've tried 3 variations for testing (note: I've cancelled the pipelines to safe resources):

• https://git.drupalcode.org/project/eca/-/pipelines/80644 - default settings: runs composer, all tests and all linters
• https://git.drupalcode.org/project/eca/-/pipelines/80645 - opt into next major and opt out of current: runs composer current and next major, runs tests on next major, runs all linters
• https://git.drupalcode.org/project/eca/-/pipelines/80647 - opt out of current: runs composer and all linters

Just commenting here permanently to record my agreement (as I said on slack) that we do not need skip_composer. Following the work done on the MRs and Jurgen's eloquent reasoning, it became clear that it was unnecessary and also a possible source of confusion. It was only going to be useful if you wanted to quickly run the entire list of jobs that need composer (which is everything except Pages). If you want to just test Pages, you can temporarily set the remaining 7 SKIP variables to 1.

I have updated the issue summary with the goals (modified from #73 to reflect the final decisions.

Feedback on the MR. Needs work.

Have successfully repeated the tests from #76 with the latest version in MR!108. Keeping the issue at NR so that @jonathan1055 and @fjgarlin can also test again before we move this forward.

I've added a changelog entry and updated the readme file with additional information.

I also left some minor feedback in the MR about deprecations. The testing done so far looks good.

I have done lots of testing, and not surprisingly, it is working perfectly as expeced so far.

In the tests linked below the commit message shows which variables have been added, ie are not default. I have also written out the full set of OPT_IN and SKIP_ values in the composer after_script, and highlighted which have been changed from the default. This gives a quick way to double-check what used in the pipeline, see attached screengrab.

Everything default, no skip or opt-in variabes
https://git.drupalcode.org/project/scheduler/-/pipelines/81243
1 composer, 1 pages, 4 x lint jobs, 1 phpstan, 1 phpunit, 1 nightwatch

OPT_IN_TEST_NEXT_MINOR=1, OPT_IN_TEST_CURRENT=0
https://git.drupalcode.org/project/scheduler/-/pipelines/81247
2 x composer, pages, all linting, but phpunit and phpstan are only at 'next' not current because OPT_IN_TEST_CURRENT=0. No Nightwatch because that is only at Current core (at the moment).

SKIP_PHPSTAN=1 SKIP_PHPUNIT=1 SKIP_NIGHTWATCH=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81250
1 x composer, pages, 4 x lint. No phpunit or phpstan or nightwatch.

OPT_IN_TEST_CURRENT=0 SKIP_ESLINT=1 SKIP_COMPOSER_LINT=1 SKIP_STYLELINT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81254
Composer, pages, phpcs. No phpunit or phpstan or nightwatch because no core versions are being tested

OPT_IN_TEST_NEXT_MAJOR=1 SKIP_ESLINT=1 SKIP_COMPOSER_LINT=1 SKIP_STYLELINT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81265
2 x Composer, pages, phpcs, 2 x phpstan, 2 x phpunit, 1 x nightwatch

All the above test combinations work as expected so it looks good.

We have 8 SKIP_ variables and 6 OPT_IN_ variables. So in an attempt to show that each variable alone is having the desired effect I am going to run 14 tests with each of these in turn is chagned from the default and nothing else is altered. Results to follow.

Here are the 8 jobs each testing a single SKIP_ variable and no other skip_ or opt_in variables. All correct.

SKIP_COMPOSER_LINT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81281
4 valiadate jobs not 5, composer_lint is not run.

SKIP_ESLINT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81294
4 valiadate jobs not 5, eslint is not run.

SKIP_STYLELINT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81296
4 valiadate jobs not 5, stylelint is not run.

SKIP_PHPCS=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81298
4 valiadate jobs not 5, phpcs is not run.

SKIP_PHPSTAN=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81299
4 valiadate jobs not 5, phpstan is not run.

SKIP_PHPUNIT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81310
1 testing job not 2, phpunit is not run.

SKIP_NIGHTWATCH=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81308
1 testing job not 2, nightwatch is not run.

SKIP_PAGES=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81311
Build job Pages is not run. 5 valiadate jobs, 2 testing jobs.

Here are the six tests, each having one OPT_IN_ variable set differently from the default. All correct.

OPT_IN_TEST_CURRENT=0
https://git.drupalcode.org/project/scheduler/-/pipelines/81349
Composer, pages, 4 x validate jobs not 5 (phpstan is not run), 0 test jobs because no core versions are being checked

OPT_IN_TEST_PREVIOUS_MINOR=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81359
Composer, pages, 5 x validate jobs, 3 test jobs (phpunit at current and previous minor, and one nightwatch)

OPT_IN_TEST_NEXT_MINOR=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81461
Composer, pages, 6 x validate jobs (because phpstan runs at current and next minor), 3 test jobs (phpunit at current and next minor, and one nightwatch)

OPT_IN_TEST_PREVIOUS_MAJOR=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81427
Composer, pages, 5 x validate jobs, 3 test jobs (nightwatch and 2 x phpunit)

OPT_IN_TEST_NEXT_MAJOR=1
https://git.drupalcode.org/project/scheduler/-/pipelines/81462
Composer, pages, 6 x validate jobs (because phpstan runs at current and next major), 3 test jobs (phpunit at current and next major, and one nightwatch)

OPT_IN_TEST_MAX_PHP:
https://git.drupalcode.org/project/scheduler/-/pipelines/81465
Composer, pages, 5 x validate jobs, 3 test jobs (phpunit at current and max php, and one nightwatch)

Following up the discussions on Slack, I have tested what happens if deprecated rule snippet is being used in a contrib template.

All composer jobs defined in .main.yml (in the current committed 'main' branch) have *skip-composer-rule and the variants also have *opt-in_

  rules:
    - *skip-composer-rule
    - *opt-in-next-minor-rule
    - when: always

The contrib template used in these tests has $OPT_IN_TEST_NEXT_MINOR=1 and a custom Composer (Next Minor) job which contains the deprecated .skip-composer-rule reusable snippet. The reason this is there is to repeat the actual rules in .main.yml then to chain on to an extra one which sets a variable using conditional logic such as whether _PHPUNIT_CONCURRENT is 1 or 0. I used this example because that is what Scheduler needed to do, but it could be used to check anything and alter/add any subsequent processing in the script step. The template used in these tests skips nearly eveything else just for speed and not to waste resources, there is just phpstan which is not skipped, to show the effect of jobs that need Composer.

Testing on 'main' branch

Start by running composer and phpstan on the current branch and next minor
https://git.drupalcode.org/project/scheduler/-/pipelines/82634
There are two composer jobs and two corresponding phpstan jobs.
At the end of the composer next minor log we see "_CUSTOM_VAR=Doing something, because _PHPUNIT_CONCURRENT=0"

Then change to concurrent=1 to show the difference
https://git.drupalcode.org/project/scheduler/-/pipelines/82636
Composer next minor has "_CUSTOM_VAR=This is different, due to _PHPUNIT_CONCURRENT=1"

Testing on MR108

Running the same tests on MR108 where the rule is defined with

.skip-composer-rule:
  if: $OPT_IN_TEST_CURRENT == "0"
  when: never

MR108: OPT_IN_TEST_NEXT_MINOR=1 OPT_IN_TEST_CURRENT=1 (default)
https://git.drupalcode.org/project/scheduler/-/pipelines/82640
The custom Composer Next Minor is working ok, but there is no indication that anything is wrong, no deprecation message in the log.

Then the maintainer decides to use the new functionality and turn off testing against current, so sets OPT_IN_TEST_CURRENT=0
MR108: OPT_IN_TEST_NEXT_MINOR=1 OPT_IN_TEST_CURRENT=0
https://git.drupalcode.org/project/scheduler/-/pipelines/82643
The pipeline fails to get built:

"phpstan (next minor)' job needs 'composer (next minor)' job, but 'composer (next minor)' is not in any previous stage"

The developer had no prior warning that this was going to happen, and their testing pipeline is broken.

Testing on MR112

The proposed solution is to remove if/when:never in the deprecated .skip-composer-rule, and add back the _DEPRECATION_MESSAGE so it is:

.skip-composer-rule:
  variables:
    _DEPRECATION_MESSAGE: "**********\n* The .skip-composer-rule is deprecated .....

There should be no conditions attached to the deprecated rule, so that it just sets the message in all scenarios. This change has been made in MR112 to demonstrate the result.

Re-run the tests starting with opt_in_test_next_minor=1 and the default opt_in_test_current=1 as before
MR112: OPT_IN_TEST_NEXT_MINOR=1 OPT_IN_TEST_CURRENT=1 (default)
https://git.drupalcode.org/project/scheduler/-/pipelines/82649
Composer next minor now shows the _DEPRECATION_MESSAGE in the log.

The maintainer does the same as before, decides to turn off testing at current core
MR112: OPT_IN_TEST_NEXT_MINOR=1 OPT_IN_TEST_CURRENT=0
https://git.drupalcode.org/project/scheduler/-/pipelines/82653
The pipeline still runs (compared to the test on mr108 where it failed to build) and we correctly get 1 composer 'next minor' and 1 phpstan 'next minor'. Composer next minor shows the rule deprecation message in log.

If instead, they decide to turn off testing at next minor and revert to current core only
MR112: OPT_IN_TEST_NEXT_MINOR=0 OPT_IN_TEST_CURRENT=1
https://git.drupalcode.org/project/scheduler/-/pipelines/82660
This is where we get the unnecessary run of Composer (next minor) but it is being run precisely because they are using deprecated code and to inform the maintainer that they have this. The extra job is not run for any template that does not have custom code that uses the deprecated rule, and even then it will only be "extra and unnecessary" if the maintainer tries to turn off testing by setting a OPT_IN to 0. In this circumstance I suggest that this is worth it. The prescence of this job will also alert the maintainer to look in the log to see why it ran, and that will help them to see the deprecation message.

Also a key point is that there is correctly only one phpstan job, because there is no custom phpstan job in the contrib template that uses the depreacted rule.

We no longer have the concept of "skip composer" so it is not a valid task to try to repliacte the exact same behavior as if we did still have it. That's not possible. If we focus on what is best for the end-user/maintainer we should try to (a) inform them they are using deprecated code and (b) not make pipelines break unnecessarily. The "change of behavior" in the extra step being run is a small price to pay for a better maintainer experience, and to reduce support requests when pipelines fail to build with no prior deprecation warnings.

I can provide a patch between MR108 and MR112 to add these changes, if that would be helpful.

I've compared MR108 and MR112 and also seen the above tests. At first, we thought that

.skip-composer-rule:
  variables:
    _DEPRECATION_MESSAGE: "**********\n...

could be invalid code because it's inside a rule, but it's actually a valid rule syntax, and I think it does exactly what we need it to do:
- Nothing in terms of variables, it ignores the old and the new.
- Creates the deprecation message that we could then display inside the "composer" job.

@jurgenhaas - please review. I think it makes more sense to have the rule behave that way, and as shown above, it will avoid a potential situation where a pipeline can be totally broken.

Here's a patch for the changes. I have also expanded the deprecation message to include the url to this issue. We can update the IS with what to do if you see this message (don't need a full change record though).

In the exitsing .pre job I have set allow_failure:false and added exit 1 so that it ends in amber warning, not green to draw attention to it. https://git.drupalcode.org/project/scheduler/-/pipelines/81926

The patch brings good additions in my opinion.

Re change records, we can use the changelog.md as needed instead.

I can't reproduce the error reported against MR!108 in #85. In the pipeline https://git.drupalcode.org/project/eca/-/pipelines/82753, I used these settings and that behaves as expected:

variables:
  OPT_IN_TEST_NEXT_MINOR: '1'
  OPT_IN_TEST_CURRENT: '0'

I suspect there is something wrong in the overrides of the schedule project, and then it's great to have a failing pipeline, which forces the user to fix that.

As for how we handle deprecations, I'd like to copy this comment from the Slack thread:

Had long thoughts about it last night, and again this morning while walking the dog :wink: Here is my proposal with reasoning:

• every approach would be non-perfect in a sense that users (module maintainers) can't rely on the system to definitely warn them when they use deprecated components
• I'm saying that with the out view to the future that there will be more stuff like this, and we will never be able to catch them all
• as developers tend to be positively lazy in a way that they don't pay attention if they get the impression, the system would warn them
• therefore, we should not try and do "just the best possible", instead we should send the signal: dear developer, it's your responsibility! But: we can still make it easy to get them the relevant information.
• So, we should define an extensible message snippet that contains the list of all deprecated components
• That message should be printed by every job (top, bottom or both)
• And: a month before the next major release, when the deprecations get removed, we add a separate job which gets more exposure. That job only prints the deprecation message, and it gracefully fails so that it shows the orange exclamation mark.

That way, we have easy to find documentation always just one click away. And we don't change the current behaviour.

To me, this still holds. As we see with all the approaches we started with, none of them is able to be perfect, i.e. we can't satisfy all requirements. Therefore, a clear policy to provide the necessary information everywhere and keep the maintainers responsible for making the necessary changes, I think we would be doing our best.

If we can't get to an agreement on that, I propose that we split the topics and open a new issue for deprecation handling in general, since it isn't necessary something for this one.

Oh, there have been new comments coming in while I was reviewing and writing. Strange that I have not received email notifications for them.

Anyways, the addition of the deprecation warning job is great and fits with the last proposal, that we discussed. I'm happy with that, and I'm just unhappy with the printf "$_DEPRECATION_MESSAGE" part in the composer job, as stated too many times before.

I'm going to add the new job and we can then decide on that final piece.

I've merged the proposed changes from #87 and also added another condition to the deprecated rules like this:

.skip-composer-rule:
  if: $OPT_IN_TEST_CURRENT == "0" || $SKIP_COMPOSER == "1"
  when: never

This way, it still behaves like before and the use gets that nice yellow warning deprecation warning in the separate job.

It's now down to the question, if .skip-composer-rule makes sure to behave like before, or if it prints a message in the composer pipeline, where it is very likely to be missed. But the new warning in the job by @jonathan1055 makes it visible to everyone.

Agree that we are not going to find a perfect solution, but the progress so far is great and we are covering/testing many cases and combinations.

The way I see it. If they use SKIP_COMPOSER=1 they will get the warning in a separate job (nice!).

Using .skip-composer-rule template would only make sense in the above case, so I'm okay with not printing the message, especially if that's going to stall the progress here., and considering that we have the other separate job.

There is some really minor feedback in the MR about a typo, but other than that I really think we are there with the latest additions.

A more sophisticated approach to all deprecations can be made in the future if needed.

I think we're taking the best suggestions from #87 into MR108, and the only outstanding discrepancy might not even be that relevant (as explained in #92).

All these latest changes were about the BC layer, and I think the MR covers the most important bits (and opens the door to a future most sophisticated job).

The testing in #82, #83, #84, and #85 is just amazing and covers a great deal of scenarios.
I'm really tempted to mark this as RTBC, but I won't merge anything (on Monday) unless I have both of your approvals too.

RTBC +1

What an achievement! Thank you both @fjgarlin and @jonathan1055 for all your great input and patience.

I can't reproduce the error reported against MR!108 in #85. ... I suspect there is something wrong in the overrides of the scheduler project

There is nothing wrong with the overrides in that scheduler test MR. I am using the chained rule snippets precisely as they were designed to be used as in the current 'main' branch. But when I run it with MR108 and turn off OPT_IN_TEST_CURRENT=0 that is what causes the broken pipeline and the error. I thought I had explained all that in #85, as I went in to a lot of detail to demonsrate the problem.

I also thought that in the final paragraph of #85 I had written quite a good reasoning for why we can't replicate the exact same behavior as now, and so our focus should be to help maintainers get alerted of the use of deprecated code and not break their pipelines. And it seemed like in #86 and #88 I had convenced @fjgarlin of the point of it.

However, I'm not going to argue anymore, so lets just go ahead with the MR as it stands and not spend any more effort on this.
RTBC +1

I was basing my RTBC on that case not breaking, but if it is, let’s fix it.

I’m just not too caught up (as in “I’m happy either way”) on the extra message as we already have the extra job that will most likely catch it (#92).

Let’s just focus on that case now as the last step. No need to rush after all this effort from all. 99% RTBC, 1% Needs work.

Can we isolate and reproduce that case on both scheduler and eca? Then once the broken pipeline shows, we try to fix it. I’m happy to help with the code or just review. The goal now is to address that one case.

I’ll be online tomorrow (at a Global Contribution Weekend event) so I’ll try to work on this.

I explained it in #85, it is due to the condition in the rule we are deprecating. MR108 has

.skip-composer-rule:
  if: $OPT_IN_TEST_CURRENT == "0"
  when: never

and the fix was to remove the conditions, as per the patch in #87, so that we had

.skip-composer-rule:
  variables:
     _DEPRECATION_MESSAGE: "**********\n* The .skip-composer-rule is deprecated in version 1.1.0 and reference to this rule should be deleted.\n* See https://www.drupal.org/project/gitlab_templates/issues/3414391\n**********\n"

The actual condition causing the problem was $OPT_IN_TEST_CURRENT == "0" but now that || $SKIP_COMPOSER == "1" has also been added, it would probably be enough just to remove $OPT_IN_TEST_CURRENT == "0" and leave $SKIP_COMPOSER == "1" in. That makes the rule look exactly like it used to do, in the committed main branch, so should satisfy the need to keep things the same. It would also not break the pipeline in the test. I may have time to try that right now, or will do in the morning.

The extra condition || $SKIP_COMPOSER == "1" added to the deprecated rule in this recent MR108 commit
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/108... actually made things worse. The pipeline fails to get built if you run it specifing $skip_composer=1, see https://git.drupalcode.org/project/scheduler/-/pipelines/82998 so not only do we break the pipeline but the maintainer also does not get to see the new .pre 'deprecation warning' job.

So please undo that latest change, which was made after my testing in #85. The deprecated rule snippet should really not have any conditions attatched to it, as I explained above. This avoids breaking any pipelines and allows us to write a deprecation message to the log. I cannot name any scenario where has a bad effect. But at minimum, please remove the newly added second condition.

True. We currently have in "main" something like this:

Pipelines should not break if they add this rule in any of the composer jobs. This rule should just do nothing, the same way that we are no longer checking SKIP_COMPOSER to run composer.

The goal is that the pipeline does not break. Checking no variables and letting the other rules play is by far the easiest way to achieve this.
I thought that the previous || condition added would help, but yeah, it is not enough.

At this point, I'm a bit lost and confused between feedback rounds, tests, etc.
MRs 108 and 112 are almost identical. The only difference:

.skip-composer-rule:
-+  if: $OPT_IN_TEST_CURRENT == "0" || $SKIP_COMPOSER == "1"
-+  when: never
++  variables:
++    _DEPRECATION_MESSAGE: "**********\n* The .skip-composer-rule is deprecated in version 1.1.0 and reference to this rule should be deleted.\n* See https://www.drupal.org/project/gitlab_templates/issues/3414391\n**********\n"
 

Plus the displaying of the message (which is useful as it is in the .composer-base, so it should appear in all composer jobs). This behaves similarly to other deprecations we already have in the code.

The above version can break pipelines and skip jobs that are needed, the below does not and leaves the calculation of the jobs to be automatic.

The only difference between MR108 and MR112 is the one described above. I'd say that MR112 needs review.

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

jonathan1055 → changed the visibility of the branch 3414391-opt-in-current to hidden.

Suggested patch for MR108: https://www.drupal.org/files/issues/2024-01-27/3414391.mr108-mr122-diffs... →

I'm lost on this one as I don't understand why we are doing it that way. But I have applied the patch from #105 to MR!108 for the sake that we can finalize this one.

It all just comes from the fact that we should have never had the option to skip composer in the first place. If it's needed, it should be run, if it's not, it shouldn't. Allowing people to use that probably created confusion, even between us, as we saw it as a way to achieve other things (ie: skip current).

So, in this improved version, any existing variable or rule that tried to do something with it should do nothing, so the new logic that we've introduced in this issue kicks in.

I know that there has been a lot of back-and-forth on this issue, with different points of view, but always the same goal. Whilst the deprecations part has probably taken a lot of our energy during the past week, know that that's just 5% of the issue, whereas the new rules and logic (done before that) are 95% of it and that part is amazing!

I think we've all tried our best and the current state of MR108 reflects that. The nearly 250 comments (issue + MR comments) and the long slack threads also reflect that.

I'm marking it again RTBC. I will update the changelog with the date before I commit it.

Thank you both. This is very good now. As a final check, given that its always worthwhile to test after any change, I ran a pipeline trying to use both the deprecated rule and the deprecated variable, and it all ran as expected :-)
https://git.drupalcode.org/project/scheduler/-/pipelines/84200
RTBC+1

RTBC+1 from here as well.

Has been challenging, I agree. Lession learned over here it to be more careful in keeping to the purpose of an issue when ever possible. Breaking stuff out into specific topic issues is what should avoid confusion.

However, what we've achieved together feels like a great foundation for even more enhancements to come for the GitLab templates in general. Thank you both for getting this done.

The work here is now merged.

A new 1.1.0 tag has been created: https://git.drupalcode.org/project/gitlab_templates/-/tags

Note that this change will only affect right now people that have main or 1.x-latest in the ref: section of their templates.

I will announce it on slack.

THANKS! I really appreciate all the work and effort that has gone here.

I'm going to change both branches of Scheduler to use 'main' to give me (and hence us) warning of any problems in general, not just for this issue because I'm not expecting any here at all :-)

It's great that most of the contrib projects will be using the default $_GITLAB_TEMPLATES_REF. Do you have a timeframe in mind for when the recommended version gets increased from 1.0.0 to 1.1.0? How long do we need for any settling in? My thought is that if we leave it long, there is more time for contrib maintainers to inadvertently start using the deprecated variable and rule, so the longer we leave it the bigger the potential disruption ... but on the other hand we should not rush into recommending 1.1.0 too soon either.

Yeah, hard to find the right balance. I'd say that if we haven't found any blocker within the next two weeks, we could go ahead and update for all contrib.

That sounds like a good plan. Two weeks could also be the general policy, until we have any reason to change it. Of course we can always rush a new release and update the recommended value if anything urgent or major is broken. But a general two-week time lapse feels right, in all normal circumstances.

It would be very useful to know how many projects have 'main' and '1.x-latest' and thus how much exposure the new changes are getting, during the two weeks that pass.

I wish we could exclude comments! https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b...

I just did :-)
https://git.drupalcode.org/search?group_id=2&language%5B%5D=YAML&reposit...

I guess the problem is that setting those references in the gitlab-ci file is not working properly, they should be defined as variables in the project. Otherwise some scripts are loading from different references, and that's also how the documentation explains it, isn't it?

Doh! I keep forgetting that we use that variable to fetch the symlink and the expand_composer script. Luckily those files haven't changed much in a while.

In fact, checking the code, we still have the old 1.0.x reference:

_GITLAB_TEMPLATES_REF:
        value: "1.0.x"

Thanks for spotting it @jurgenhaas. I'm creating a follow-up issue to fix this.

#3417987: _GITLAB_TEMPLATES_REF value in variables needs updating →

cmlara → changed the visibility of the branch 1.0.x to hidden.

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

jonathan1055 → changed the visibility of the branch 3414391-opt-in-current to active.