Do not use "before_script" or "after_script" here to allow contributors to hook there if needed

We are trying NOT to use before_script to allow people to add commands. We are only using after_script once because otherwise we wouldn't have the logs of failed phpunit jobs, but we are also trying to avoid the usage of this.

My concern would be to offer hooks for one job and not for others as well, as the experience wouldn't be consistent. My recommendation would be to stick to before_script (most use cases might fall here) or after_script.

I was also concerned with the proposed additions, as it seemed to be giving too many extra places to add things. @bbrala could you give examples (apart from the known single usage of after_script in phpunit) where you cannot achieve what you need with the existing hooks?

We are trying NOT to use before_script

We are only using after_script once

This is good info. Maybe we can make this explicit in the docs? That gitlab templates will not use before_script, and also document the single place when it does use after_script. Maybe there is something creative we can do to get round that usage, or if not then maybe provide just the single new hook for that scenario?

Its fine if we decide never to use before script. But i was not aware of that policy. If we start recommending this, we cannot say we "try" not to use before_script, because if we start to, then we will break contrib.

Unforeseen needs arise. Breaks happen. Mistakes happen. Its OK to say "try" IMO.

You have other ways to add extensions if you wish, like using own Dockerfile to extend the web image. I'm experimenting with that for memcache module - https://git.drupalcode.org/project/memcache/-/merge_requests/18/diffs#64...

Minimalist docs for adding php extensions are in our our Overrride section https://www.drupal.org/node/3356364#s-overriding-parts-of-the-template → (item 6)

Ok then. Let's keep that as the advise.

@moshe said

Unforeseen needs arise. Breaks happen. Mistakes happen. Its OK to say "try" IMO.

I think that is a bit harsh. I know that unforeseen needs arise but can we not be a little more amenable and try to minimise the disruption if/when that were to happen.

Could we not at least have the existing .phpunit-base after_script be defined as an or using << so that if contrib wants to use after_script it is just one line to include everything that gitlab_templates needs, and that would help to future-proof if gitlab_temaplates has to make additions to that after_script.

This pattern could also be the policy if gitlab_templates ever needed to use other before_script or after_script in the future. Just an idea ...

The "phpunit > script" can be slightly refactored so we don't need to use the "after_script". We can do as we did with the "phpstan" job where we capture the exit code but don't fail, then do the rest, and have the last step returning the captured exist code.

Let's use this issue for it so we can have a clean "before_script" and "after_script" repo.

Updated issue description.

I think this cleans up the "after_script" that was used in the ".phpunit-base". It's the only instance where we were using it.
There is a "before_script" in another job, but this other job extends "phpunit", so in this case, using it is fine.

This is ready to test. MR: https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/81

Note that we should test this in both successful and failing phpunit jobs to ensure that we are capturing and returning correctly the job status.
Please test/review.

This is excellent work @fjgarlin. I will test and report back.

Tested on https://git.drupalcode.org/project/scheduler/-/pipelines/45445 and I got

 Unable to create pipeline
Project `gitlab_templates-3399313` not found or access denied! Make sure any includes in the pipeline configuration are correctly defined.

Is this because the MR is only 'draft'?

My error, I did not prefix the repo with issue/

D7 with forced failure fails as required
https://git.drupalcode.org/project/scheduler/-/pipelines/45529

Same with D10
https://git.drupalcode.org/project/scheduler/-/pipelines/45499

The logs clearly show the additional || EXIT_CODE=$? and also _SHOW_ENVIRONMENT_VARIABLES is on, to check the repo and branch is this mr.

Setting to RTBC from the requested testing point of view.

Thanks for testing all cases (success and fail) for both D10 and D7.
Merging this and marking it as fixed.

That also means that we have all the "before_script" and "after_script" slots available for all main tasks.

That's great. I just realised that actually in the D10 version I only tested with concurrent=1 and there is the corresponding change in the non-concurrent block. I will check that too, but it looks fine.

One other question, in the IS proposed resolution you mentioned:

- Clean up the "before_script" for "phpunit (next major)".

I don't exactly know what this means, but there is only one change in the merge, so it that something else that needs to be done?

Good points. Please check that other case. Hopefully it will be all good too.

I'll update the IS, that "before_script" is a legit one and it's a job extending the "phpunit" one, so it's fine in this case. I just forgot to update the IS.

Done and all OK. D10 using concurrent=0 forced failure red as required
https://git.drupalcode.org/project/scheduler/-/pipelines/46101

and with the test fixed, it is green
https://git.drupalcode.org/project/scheduler/-/pipelines/46131

Thanks for explaing about that 'before_script'. So now that both the before_script and after_script are free to be used for any step, should we state somewhere in the docs that the intention is to keep it this way, to allow Contrib developers to use those steps with (reasonable) confidence?

Yeah, agree, it'd be great to have a little paragraph here https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → explaining that most jobs can be tweaked by adding some "before/after_script" sections, in case new packages are needed, etc.

This was already implied by us linking to MongoDB which uses this technique. But I have now made it explicit. https://www.drupal.org/node/3356364/revisions/view/13301062/13302270 →

Thanks so much to all of you, I really appreciate all the code, the reviews, and the documentation updates!!

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