Add custom variables to set before_script commands in upstream project

I have pushed a phpcs-inactive.xml file to d9-basic to start this work.

https://git.drupalcode.org/project/gitlab_templates_downstream/-/commit/...

We may want to have a MR here, instead of doing direct commits.

Change of design - instead of renaming the file (which has complications due to the symlinks and directory paths) the plan is now to have a phpcs.xml file active all the time, with a rule that can be edited to make the phpcs job fail when required.

Plain test of MR1 using defaults, but with other jobs temporarily skipped, and with _PHPCS_EXTRA: -v for verbose output. Pipeline passes green
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

Pipeline submitted via UI with custom variable entered in form PHPCS_BEFORE_SCRIPT with value sed -i 's/80/30/g' phpcs.xml to reduce the line limit to 30 and cause a phpcs warning required
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

Pipeline on this MR1 branch using PHPCS_BEFORE_SCRIPT set as variable in the triggering job. Fails as required, without any commits to the downstream repository.
https://git.drupalcode.org/issue/gitlab_templates-3380694/-/pipelines/41...

Triggered from upstream the PHPCS_BEFORE_SCRIPT variable. The log shows that the variable is empty, no sed is executed and the job pass green as expected, without any commits to the downstream repository.
https://git.drupalcode.org/issue/gitlab_templates-3380694/-/pipelines/41...

This is ready for review and feedback.

instead of renaming the file (which has complications due to the symlinks and directory paths)

Which complications? Doesn't a phpcs:before_script where we rename the file solve the issue?

That eval makes me a bit nervous.

If the command is just a rename, then it executes in the top-level $CI_PROJECT_DIR folder. The real file is renamed, because we see renamed 'phpcs-not-active.xml' -> 'phpcs.xml' in the log. But the symlink (which is what is being used by phpcs) is not renamed, and we see

lrwxrwxrwx 1 root root 48 Feb  2 16:59 gitlab_templates_downstream.info.yml -> ../../../../gitlab_templates_downstream.info.yml
lrwxrwxrwx 1 root root 46 Feb  2 16:59 gitlab_templates_downstream.module -> ../../../../gitlab_templates_downstream.module
lrwxrwxrwx 1 root root 32 Feb  2 16:59 phpcs-not-active.xml -> ../../../../phpcs-not-active.xml

in the log. Then the job still downloads the default assets/phpcs.xml which is not what we want.
/builds/issue/gitlab_templates_downstream-3503851/web/modules/custom/gitlab_templates_downstream-3503851

I tried to change directory within the command string, to effectively have cd $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME; mv -v phpcs-not-active.xml phpcs.xml but I could not find a way for those variables to be escaped so that they resolve to the downstream values. I tried one backslash but obviously that did not work. I then had the idea of editing the file not renaming, and that works, but the more complicated command required eval. I understand the concern about that.

So, we need to find a way to escape CI variables in the string passed to the downstream project. Or we have the cd $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME in the downstream job, which always gets executed, then conditionally do the PHPCS_BEFORE_SCRIPT command. That is fine here, as it is quite a niche requirement. We know that we want to change directory. That actually makes the command simple. I did have that in an earlier test version, so it's easy to recover it.

Why don't we try adding to the repo a .gitlab/before_script.sh which we call under a variable switch (for example PHPCS_BEFORE_SCRIPT).

phpcs:
  before_script:
    - |
       if [[ "$PHPCS_BEFORE_SCRIPT" == "1" && -f $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME/.gitlab/before_script.sh ]]; then
        $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME/.gitlab/before_script.sh
      else
        echo "No before script actions."
      fi

In this case, the variables inside the script should expand accordingly and we can just turn on/off the script execution.

We could even expand on this for multiple cases with switches and scripts, but wanted to run the idea past you first to see your thoughts.

I've put back the working example of this into the MR. Essentially it is

phpcs:
  before_script:
    - cd $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME && pwd
    - |
      if [[ $PHPCS_BEFORE_SCRIPT != "" ]]; then
        echo "Executing command PHPCS_BEFORE_SCRIPT=$PHPCS_BEFORE_SCRIPT"
        $PHPCS_BEFORE_SCRIPT
      fi

This does work for this scenario, but it is not very generic, as it assumes that the command(s) contained in $PHPCS_BEFORE_SCRIPT should always be run in $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/$CI_PROJECT_NAME. It also divides the active functionality between the upsteram (where the command is set) and the downstream (which changes the directory). So I don't think this is the best way forward.

Your idea of scripts in the downstream project sounds like it would work, but my original idea was that the upstream project could define any commands it needed, and we'd not have to commit changes to downstream. But maybe we should have scripts (or just re-usable code references) each designed to achieve a specific thing. It could be called something like "make-phpcs-fail" which is all that we actually want at this time. This would be clearer to understand in the upstream project trigger definition. It shows what we want, and does not have to be concerned with how that is actually donw. Then instead of $PHPCS_BEFORE_SCRIPT being a command or a 1/0 flag it could be the name of that re-usable reference in the downstream project.

With the above approach:
- Let's not worry about the active directory at all. If a script needs running, it should include the necessary logic to go to the right place. That way the code will be simpler. We know that before_script will always start at $CI_PROJECT_DIR.
- With that syntax, it'd be great to try running a command, but also making reference to a script in the downstream pipeline (eg: .gitlab/before_script.sh - where that is the relative path within the repo).

If this work, maybe that's all we need really, as we could either run commands directly or call a script present in the downstream pipeline, so we don't need if/else logic or other edge cases.

I pushed the change yesterday to MR1 and it works. I just did it in the template step, not a separate script.

Pipeline with no variable, using 'default-ref'
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Triggered from upstream MR with PHPCS_BEFORE_SCRIPT = make-phpcs-fail
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

Pipeiline via UI with
_GITLAB_TEMPLATES_REPO = issue/gitlab_templates-3380694
_GITLAB_TEMPLATES_REF = 3380694-run-phpcs-in-project-folder
PHPCS_BEFORE_SCRIPT = make-phpcs-fail
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

This is quite simple, and we only have one use-case right now. But we can add 'make-phpunit-fail', 'make-phpstan-fail' etc when we need them. Do we still need to investigate the separate script idea?

Oh cool, I think I understand now fully the whole logic. I saw yesterday's commit but I think I misunderstood it a bit.

So from the gitlab_templates (or UI with GTD), we can set PHPCS_BEFORE_SCRIPT to be a string value, and then we will use this value in the GTD project before_script to run certain options/tests/changes.

I left some small feedback on MR1.

I don't think we need to investigate the separate script idea. This is good as the logic lays in GTD, and then we can control which variable to use in gitlab_templates.

It was also my fault in #8 as I showed my earlier idea which I then went on to reject, so I'm not surprised I confused you. The script idea could be useful in more complex requirements, which can investigate if/when that need arises.

I have moved the phpcs config file to a new hidden .gitlab/ folder, as you suggested. I would like to suggest two more ideas which I have committed and should make this better:

1. I can envisage scenarios where more than one thing needs to be done in the before_script when we are testing MRs, for example when testing a change in cspell we may want an action 'use-unrecognized-word' which causes the job to fail, then we run the test again with 'use-unrecognized-word' and 'update-project-dictionary' to demonstrate that this now fixes the job. It would therefore be useful to allow PHPCS_BEFORE_SCRIPT to contain a list of actions, not just one, to avoid duplication in the GTD template. It would be a simple pattern match if [[ $PHPCS_BEFORE_SCRIPT =~ "make-phpcs-fail" ]] instead of checking exact equality.
2. Having a list of actions would mean that there is no real need to have a separate "before script" variable for each job. Instead of having $PHPCS_BEFORE_SCRIPT, $CPSELL_BEFORE_SCRIPT, $ESLINT_BEFORE_SCRIPT, etc we can have just one variable, say $BEFORE_SCRIPT_ACTIONS, which holds any and all actions we want. This will make it easier to copy/paste when setting up the template and the steps in GTD. There is only one variable to set and to check, so we are less likely to make mistakes on either side of the link between the two projects.

I have pushed the changes, and will add an exampls for CSpell and run some tests.

1. That's a great idea! We can separate by space, or comma, or pipe.
2. 100% with that, I like it better this way, it's more generic and still easy to understand.

Tested here - plain test, no variables
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

UI with

BEFORE_SCRIPT_ACTIONS = other-thing phpcs-fail
_GITLAB_TEMPLATES_REF = main

https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...
phpcs job fails as required

Added actions for cspell. Plain pipeline
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

UI with

BEFORE_SCRIPT_ACTIONS = phpcs-fail cspell-use-madeupword
_GITLAB_TEMPLATES_REF = main

both jobs fail as required
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

UI with

BEFORE_SCRIPT_ACTIONS =  cspell-use-madeupword cspell-project-words something-else

https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...
No failures, green as expected

This is ready for review. I am also going to test it as a downstream project in MR325 on ✨ Add basepath parameter to PHP Code Sniffer Active

Run as a downstream pipeline with the following

'→ GTD D9 Basic':
  variables:
    # Force phpcs to fail, to demonstrate the xml report and artifacts.
    BEFORE_SCRIPT_ACTIONS: 'phpcs-fail'

and it fails as required
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

The changes in this MR on the d9-basic branch will also be needed in the other branches. Once it is merged, is the best way just to do a simple git cherry-pick? The branches are mostly aligned, so that should not be a problem. It will encourage us to keep things the same over the branches where possible.

This looks good to me RTBC. Thanks for the tests. This gives us a lot of flexibility and the possibility of adding new test cases as we find new bugs in the "GitLab Templates" project.

Feel free to do the merge and the cherry-picking (when possible) to other branches.

Thanks for the review. I have merged MR1 into d9-basic and cherry-picked onto d10-plus

I'm going to do a MR for the first Drupal 7 branch, as that will require editting due to no cspell job, so would be good to make sure I've got that correct before committing.

The d7 version works. Tested here using downstream pipeline from ✨ Add basepath parameter to PHP Code Sniffer Active and setting

'→ GTD D7 Basic':
    # @todo reset this back to default repo and branch when issue 3503851 is merged.
    project: issue/gitlab_templates_downstream-3503851
    # cspell:disable-next-line
    branch: 3503851-customisable-before-script-d7
  variables:
    # Force phpcs to fail, to demonstrate the xml report and artifacts.
    BEFORE_SCRIPT_ACTIONS: 'phpcs-fail'

https://git.drupalcode.org/issue/gitlab_templates_downstream-3503851/-/p...

Merged MR3 into d7-basic and cherry-picked onto d7-composer.
Thanks for you help and reviews.

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

jonathan1055 → closed merge request !4