Review and try to simplify SKIP and OPT_IN variables

Note that we might not even need to deprecate the old variables if we don't want to, we could just use them to calculate the RUN_XX ones. But marking as deprecated also make sense to me.

I think that all SKIP_XX/OPT_IN_XX/RUN_XX are easy to use and understand, but only the RUN_XX ones give us full flexibility to control whether we want it on or off by default.

Basically:
- All SKIP_XX ones will have a matching RUN_JOB_XX one.
- All OPT_IN_XX ones will have a matching RUN_VARIANT_XX one.

I like this approach. I can see why the OPT_IN_ and SKIP_ names got set that way during the initial development, but having the name contain a meaning which implies a default value does cause us problems when we want to start new things, as you have said with the upgrade_status job.

Setting the new variable values from the existing could be done in .pre but it could also be done in the .create-environment-variables section. We can see which is more appropriate when we start the MR. The decision on whether to give a deprecation warning or not may also influence the place where the new values are created.

When setting the new values, we could change the default values of the old variables to blank and move them into the hidden variables file. Or maybe just delete them completely. Either way, if we find a value (either 0 or 1) we know that it was set in the project's .gitlab-ci.yml and can take the appropriate action. What happens if someone has both the old and new and they conflict? for example OPT_IN_NEXT_MINOR: 0 and RUN_VARIANT_NEXT_MINOR: 1. I guess we decide which takes precedence. But given that we can't tell immediatly if a new variable was set in .gitlab-ci.yml or just defaulting, we might need to check the value against the default, in order to determine which to do. Or maybe we go with the old value, as if that has a value we know it has been set explicitly in the project file, and we shold respect that.

Yeah, good points on .create-environment-variables vs .pre. Let's just do it wherever makes the most sense.

Maybe we can do the variable transformation in .create-environment-variables and only use .pre to detect if something deprecated is used. In any case, once we start coding things we'll have a better idea about where goes what.

I also like your idea of turning those "0" to "" for easy detection.

As for variable precedence, I guess that if we find a newer variable, that should take priority I think. The changes about the "0" to "" should make it easier.

Overall, I'm glad we're on the same page. I think it's the right move going forward.

Thinking about this again, it will be too late to do the variable transformations in .create-environment-variables because that is executed in the Composer jobs, and so the new variables used in the composer rules: will already need to be set to the correct final values. So I think it will have to be done before any Composer job, which probably means in the .pre stage?

As for variable precedence, I guess that if we find a newer variable, that should take priority I think. The changes about the "0" to "" should make it easier.

In principle it would be nice to have the new variable value take precedence, but I don't think this will be possible in the scenario we are working in. I don't know any way that we can tell if a new variable has been set in the project's .gitlab-ci.yml file or is just taking the default value. But we can be sure when an old variable has been set in the project's file, because it will have a non-blank value. So we use that value to set the new variable. The other new variables are not touched, they will either be set in the project's file or will take the default.

True, then .pre stage it is. Once maintainers correct the variables to the new format/names, they won't see this anymore.

What I mean about the precedence is only when both the old and new value are found to be filled up explicitly in the .gitlab-ci.yml file, and yeah, it'll need to be filled up with the non-default value, otherwise it'll be impossible for us to detect that it's not coming from the defaults. If the value of the new is default, and the old format is filled up, then yeah, old format will determine the value of the new variable. I think we're thinking the same here too, but it's difficult to express in words with so many "old", "new", "default"... 😅

💯 👏 This looks like an excellent improvement! It'd indeed be much more consistent, easier to learn, and for sure easier to master.

IMHO we shouldn't support both simultaneously/in parallel. That too would be confusing, and would increase the maintenance burden significantly. I'd vote to remap the old variable names into the new ones. That'll also encourage everyone to gradually adopt the new names.

I have started this, just for one variable, converting SKIP_COMPOSER_LINT into RUN_JOB_COMPOSER_LINT

The first commit just makes the basic variable name change, modifies the .pre stage deprecation job and does the transformation. It is tested here
https://git.drupalcode.org/project/scheduler/-/pipelines/183501

Things to note

1. The deprecation warning shows up for every Composer variant, which needs to be fixed as that is just confusing. It should not be too difficult to solve.
2. The value of the new variable is written to >> build.env so is availbale in all subsequent jobs
3. However, it seems that the rule for determining if the Composer Lint job is added to the pipeline has already been 'executed' by that point, so it is too late to reply solely on the new variable in the rule

The second commit therefore adds back the old variable into the rule. This has the desired effect, it means that the existing $SKIP_COMPOSER_LINT value is respected and the job is not run in this case. Tested here:
https://git.drupalcode.org/project/scheduler/-/pipelines/183952

There may be a better way to do the transformations, so that we can properly remove the old variables now, instead of leaving them in the rule, deprecated, and having to remove them in gitlab_templates version 2.0

However, it seems that the rule for determining if the Composer Lint job is added to the pipeline has already been 'executed' by that point, so it is too late to reply solely on the new variable in the rule

Yeah, I'm finding this behaviour in more places in GitLab CI. Now I understand why bringing the old rule back.

There may be a better way to do the transformations, so that we can properly remove the old variables now, instead of leaving them in the rule, deprecated, and having to remove them in gitlab_templates version 2.0

If we want to keep BC then that might need to be the way, keeping the old and the new. I cannot think of a way right now where we can just get rid of them.

I've been thinking a bit more about the new variable names, and have the following ideas:

1. Given that the variables are going to be renamed, now would be a very good time to add the leading underscrore, so that all variables which are user-facing and intended to be changed start with _ This makes it simpler to remember, when copying a variable or duplicating a line in .gitlab-ci.yml to add a new onew. The work we did in #3426292: Deprecate and rename some variables that were wrongly named → means that of the non-hidden variables it is only the SKIP_ and OPT_IN variables, plus the one recent new addition RUN_JOB_UPGRADE_STATUS which do not start with underscore.
2. I am not sure of the benefit of having "JOB" and "VARIANT" in the variable name. I know the two types of variable fall into these categories, but it just adds extra length to everything you need to type. I don't think it adds much - people are not going to confuse 'CSPELL' with 'NEXT_MAJOR' as that text gives enough explanation as to what the variable is used for.
3. Following on from (2) I suggest that the "job" variables start with _RUN and the "variant" variables start with _TEST because you "run" a job but "test" against a version/variant. There is no specific benefit in having the varibales all start with 'RUN' when half of them are for specifying a version.

Taking all these three together, in summary it means that the SKIP_xxx variables are renamed _RUN_xxx and the OPT_IN_TEST_xxx variables become simply _TEST_xxx. The _RUN_ variables need to have the value swapped from 1 to 0, 0 to 1 compared to their original SKIP_ value, but the _TEST_ variables keep the same value as their OPT_IN_TEST_ original. This seems easier to explain in the support we'll need to give, and easier for maintainers to convert from the old names.

1. Agree. great point.
2. It's not about confusing, it's more about making sure people understand what the toggle variable will do. I'd rather keep the _JOB_ and _VARIANT_ in the name to make them verbose and make people understand what they are enabling. A variant can be a set of jobs (ie: phpstan + PHPUnit + nightwatch). I think we have a deeper understanding of the variables, but other people might not.
3. Now if we do it this way, that will give enough differentiation and I'm totally cool with this suggestion. I like it. Good thinking!

So yeah, 100% agree with your final paragraph. We can go ahead with that approach (maybe update the issue description to reflect that too). Thanks for your thinking around this, really appreciated!

OK, that sounds as if you agree with my points 1 & 3 but not with 2. That's fine. So before I update the issue summary and the title, just to confirm

• SKIP_xxx variables are renamed _RUN_JOB_xxx
• OPT_IN_TEST_xxx variables become _TEST_VARIANT_xxx

Sorry, I wasn’t clear in my comment. I would have disagreed with 2 in isolation, but when you presented 3 then that superseded 2 in my mind.

So you could say that I agree with 1, 2 and 3, and go fully with your suggested naming.

Thanks. I have updated the issue summary and title to relect this.

Just done a bit more testing on a custom job, and variables written to >> build.env in the .pre job are not created for use in the Comopser jobs. Maybe it is a different file, for passing from .pre into build stage? If we found a different way to do that (the variable transformations) it might mean that the rules: could be changed to only use the new variables? If you have any ideas, let me know. I'm having trouble finding any documentation about the '.pre' stage.

The main problem I've seen is that these "dotenv" variables can be used in the "script" section, but not in the "rules" section. That's a big challenge here. There are workarounds for this (ie: downstream pipelines) but they are not ideal. Right now, I haven't investigated much purely because we didn't need to.

I haven't reviewed the code yet but I'm glad to see that progress is being made here.

Re downstream pipelines and dotenv (https://docs.gitlab.com/ee/ci/pipelines/downstream_pipelines.html?tab=Pa...), just thinking out loud, so feel free to use it or totally discard it.

We could have a variable called _CALCULATE_DEPRECATIONS (set to 1 by default, and the name is just a suggestion). All "current" jobs would have a rule to not run if the variable is set to 1, except two new jobs, which would only run when the value is set to 1.

Now, the default behaviour would be to calculate deprecations, to help people upgrading to the new variables. The two new jobs that would run "on the first iteration" would be "build_vars" (a job to detect known deprecated variables, then display a warning message and build a dotenv file with calculated values) and "trigger_jobs" (a job to trigger the project's pipeline that would set _CALCULATE_DEPRECATIONS: 0, therefore making all the other jobs run).

All the jobs that won't run in the first iteration, but will on the second would have something like this (untested) to require the artifacts generated:

needs:
    - project: $CI_PROJECT_PATH
      job: build_vars
      ref: $CI_COMMIT_REF_NAME
      artifacts: true

This means that all calculated dynamic variables should be able to be used, even in the "rules" section. At least this is according to the documentation and some suggested solutions found on StackOverflow (or similar).

--

As I said at the beginning, this is just thinking out loud. It might be that we don't need it at all, but I wanted to write down thoughts on passing variables between jobs which would be available not only in the "script" section.

I would suggest whatever the new variables are it may be better to switch to YES/NO instead of 1/0 and set them as explicit options with drop-down menus for use inside of the GitLab Pipeline display.

@fjgarlin your idea in #19 is definitely worth looking at. I had also seen the parent/child pipeline documentation etc, but was thinking it might be overkill, but now that you have suggested it let's give it a go. It would certainly mean that the rules: in the main jobs will be kept simple and only need to use the new names. The work I started shows that each of the 6 OPT_IN rules and the 10 SKIP rules would need to be doubled up to cater for the old variable and new variable. This would, at some time in the future, get removed maybe in v2.0 but with the new way the code is much cleaner, and hence understandable.

Two questions

1. With the two new jobs in the first iteration, if there are no deprecations found, does the "build_vars" still get run? Is it building everything, even for clean projects ? Or will is only be triggered by rules that detect a deprecated variable, like the deprecation job(s) we have now.
2. What is the relationship between the two new jobs? I presume they run in different stages, with "trigger_jobs" waiting on the success of "build_vars"

Depending on the answers to these, maybe the functions of the two new jobs could be squashed into one job? It would run regardless of whether deprecations exist, would calculate the transformations if necessary, end green if clean but amber with messages if deps found, then trigger the "real" pipeline.

1. I'd say so, even for clean projects. That job will run and just add converted values if present, otherwise everything will stay as is. It'll be a kind of "preparation/checks" job. So I see it running with only one rule: "CALCULATE_DEPRECATIONS == 1". Maybe we can just call that variable PHASE and when it's 1 is the "preparation/checks" jobs, when it's 2 it'll be the actual jobs.

2. The reason for the two initial (phase 1) jobs is because we can't probably just do it in 1 job only (according to the example here https://docs.gitlab.com/ee/ci/pipelines/downstream_pipelines.html?tab=Pa... and probably the limitations of the "trigger" syntax). So yeah:
- Phase 1: "Building variables" > "Deploying pipeline"
- Phase 2 (what we have now): "composer jobs..." > "linting jobs..." > "testing jobs..."

OK thanks. Sorry, I did not read your linked doc in detail. We can experiment see what works. I like the "Phase" name, that is more intuitive givem that it's not only about deprecated variabls.

I have just pushed an update to do the processing for one OPT_IN variable, using the structure I already had. I also derived the new variable from the old regarless of value, as we agreed that if the old variable has been overriden in the project .gitlab-ci.yml then it should take precedence. That's nice a simple to explain, is predictable (now that the default for the old is blank) and does exactly what we want.

I have left the PHPCS rule only using the new variable, so this can be used as the test when we make the new pipeline structure.

If we do this as part of a 2.x effort, we wouldn't need to worry about BC and just name the variables how we feel they should be named.