I've removed the unwanted 3rd test case and the pipeline passes all jobs now. Ready for review.
I have rebased again, given the 26 new commits since last time. Now all the GTD pipelines pass, including the two 'max php' ones which failed last time.
Ah, OK yes I see. I have pushed the addition of those two examples. They all pass the Drupal sniffs, but the version with // @phpstan between the function block and the #[attribute] fails with
-----------------------------------------------------------------------------------
[x] Expected 1 blank line before function; 0 found
(Squiz.WhiteSpace.FunctionSpacing.Before)
-----------------------------------------------------------------------------------
See https://github.com/pfrenssen/coder/actions/runs/15210892656/job/42784511...
We can't change that as it is an external sniff. But I think that is OK, as it's not really what we want to allow. The main thing is that the modified sniff allows the @phpstan comment to preceed the function definition, both with and without the #[attribute]
I will remove that failing test case then check again. There may be other problems, as the logs are obscured by lots of deprecation messages.
To demonstrate the situation as-is I creatd a new GTD merge request MR17.
Setting OPT_IN_TEST_CURRENT: 0 to just test PHPunit with the Max PHP variant, the pipeline still runs phpunit at 'current'
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
Adding SKIP_PHPCS:1, the phpcs job is correctly removed, but we still wrongly get two composer jobs and two phpunit jobs
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
With a change to a test file we also get the 'test-only' job, which is wrong as this should only be run with 'current' variant (as is done in D10+)
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
Then switching this branch to be tested via MR361, by modifying the '→ GTD D7 Composer' job project and branch
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503337/-/p...
This is correct - we only get composer and phpunit for 'max php version' and no 'test-only changes'
So this is ready for review
jonathan1055 → changed the visibility of the branch 3503337-testing-d9-basic to hidden.
jonathan1055 → changed the visibility of the branch main to hidden.
The test-only way would be to open a new pull request just with the test additions, can't think of a better way right now.
OK that's fine. I just wanted to know if you already have this type of test automated. I may try to get some ideas together on how to do this automatically in another job, alongside the other tests, in a similar way to how I adapted the core test to make the 'test-only changes' job for Gitlab CI. But that's a separate task.
For the test cases you requested:
1. phpstan ignore before function
2. phpstan ignore before annotation and function
3. phpstan ignore between annotation and function
I have already added:
/**
* Test for @phpstan-ignore-next-line.
*/
// @phpstan-ignore-next-line missingType.return
public function ignore_phpstan_comment() {
}
This seems to cover 1 and 3
For case 2 are you saying:
// @phpstan-ignore missingType.return
/**
* Test
*/
public function ignore_phpstan_comment() {
}
I don't understand how this is affected by the problem. This would pass anway, before these changes. Or maybe I mis-understood your request.
jonathan1055 → changed the visibility of the branch 3356800-general to hidden.
jonathan1055 → changed the visibility of the branch 3356800-spelling to hidden.
It is also "fixed" in Contrib too, see 📌 Add blacklist and whitelist to flagWords Active
Thanks for pointing out those bits. Yes the doc page could do with some re-work, as you are correct - those lines are specific to internal plugins provided by scheduler. I wrote it up from my notes when making the first plugin, media, and then refined it as we added commerce_product and taxonomy_term.
Is it enough to avoid the steps which require modification of the scheduler module, for our custom plugin implementation?
In general, yes. But there may be some things that you need to do in your module instead. An example is the first on your list - you will want to have all scheduler settings available in your content type. Whether you simply copy those from the schuduler schema or somehow get them defined dynamically to avoid duplication, you can decide on how you want to tackle it.
I think we can use this issue to make some changes to the documentation, to explain when some steps are not required and when the change needs to be done in the 3rd-party module.
This is great. Thank you for letting me investigate this change and for working out a good solution to the forward progress of CORE_NEXT_MINOR. I hope this will help more maintainers and developers to get an earlier view of the upcoming release, but also not cause BC problems by using the main dev 11.x-dev branch any longer than is necessary.
I have updated the issue summary to say what was changed.
Hi lrwebks,
Did you see the Scheduler documentation site https://project.pages.drupalcode.org/scheduler/plugin_create/ ?
I can't tell from your comments above, but you may have seen this, as it was the result of the first part of
#3249560: Write documentation for the entity plugin →
which you have linked to.
I'd be happy to help with questions and to update the documentation if there are specific things that are missing. If you could use those steps and note down any places where you get stuck, or it is not clear, then we can improve those pages.
Other issues on creating a plugin which may (or may not!) be helpful:
✨
Support for Block (display) entities
Active
✨
Paragraph scheduler plugin
Needs work
✨
Scheduler plugin for Commerce Product Variation
Active
I have tidied up the unused beta logic and removed the --dev 11.3.x test
Locally I also tested by adding $del = array_shift($dev_releases); at the end of gather_releases() to simulate the situation from a few days ago before 11.2.x-dev was created. The existing value of CORE_NEXT_MINOR: 11.x-dev is correctly derived as the expected value and no alert is shown.
Are the updated descriptions of the variables OK?
Ready for review
Thanks. I have changed the script to check for latest dev (not wait for beta) and modified the variable descriptions. The job should fail now, and tell us that CORE_NEXT_MINOR should be updated to 11.2.x-dev
I also updated the output following your MR comment, and this is ready for review and feedback.
From the agreement on Slack, here is my proposal for when CORE_NEXT_MINOR is changed
Data from sidebar on
https://www.drupal.org/project/drupal/releases/11.2.x-dev →
Have there been edits in the time since we created this?
That's a good question. I have changed the issue summary tasks into a numbered list and added a new one for this. We can now refer to the steps by number and re-order if necessary.
Given that the pages will be created by the "pages" job in GitlabCI I do not quite understand why leaving that taskl is put right at the end. How will the pages be generated if we don't use the pre-supplied "pages" job?
Thanks for resolving. That make is easy to see there is currently nothing remaining.
I resolved the threads. Can't others resolve them?
No, not everyone can. I've not worked out the rules fully, but I think it depends a few things (a) who opened the MR (b) who is a maintainer of the project (c) who started the thread. In general I try to close/resolve them if it was me who started the query/review and the issue has been address and I have the permission to close it.
Yes I will add the '(unused)' and also remove the temporary --verbose --beta
But I have asked a question in #slack about the timing of the change from 11.x-dev to 11.N.x-dev
The follow-up for Contrib
📌
Add blacklist and whitelist to flagWords
Active
has just gone live.
Here's the
change record →
We also added suggested replacements, which are shown in the cspell log.
I have added 📌 Check when core .cspell.json changes and keep the assets files aligned Active following the MR discussion.
jonathan1055 → created an issue.
Good spot on the : syntax. I have made that change, I just used : as the replacement for -> instead of + then tokenized on :
I also used the : syntax in one set of the suggested words in the assets files, to prove the change covers both syntaxes.
Ready for review.
I have created the separate public static function and added one test case. Ready for feedback before I add the other test cases.
Do we have a 'test-only' way to show that this would fail without the enhancement?
From #8
If this approach is acceptable then maybe we can make that conditional into a function that is callable from other places
Is there a place where utility functions like this can be added, so they can be used in other sniffs in future? Or shall I just leave it hard-coded in this file for now?
I have also committed a project .cspell.json to the d10-theme downstream branch, with only a couple of the default flagwords. This improves test coverage, and shows the advice message
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/52...
When we return to working on this, ✨ Add jq and yq packages to images Active has just been committed (thanks @cmlara for pushing that one). So now we have more tools to do this job.
I have added a notice when the projects file does not contain all of the default flagwords.
Here's the first test and rather ironically the test failed, not because of the new processin, but because the MR branch name contains the word 'flagwords' which is not recognised in the dictionaries. This only happens because the MR branch name is written into the .gitlab-ci.yml, it will not be a problem for real.
I added that to the scheduler dictionary and here is the second test
This is ready for first review and feedback. There are lots of commits as I have tried to show in separate stages what is done.
Test with current data - passes green, we see $expected_value = 11.x which is correct, as we are currently only at alpha for 11.2
https://git.drupalcode.org/issue/gitlab_templates-3524366/-/jobs/5280567
But using the new --beta 11.2.0-beta1 argument to simlate the future tagging of a 11.2 beta release the job shows
$expected_value = 11.2.x
- [ERROR] CORE_NEXT_MINOR (11.x-dev) does not match the expected value: 11.2.x-dev
and ends red as required.
https://git.drupalcode.org/issue/gitlab_templates-3524366/-/jobs/5280611
detect this situation and if the flagwords that we recommend are not present then show a big warning message
That is a good idea. We don't alter their .cspell.json but if any of the flagwords in the default are missing from their custom file we recommend they add them. Better to do that in this MR, then update the Change Record to reflect it.
Thanks. Yes those additions to the CR are good and I made a few edits.
It's a shame that if a project has their own .cspell.json then they do not get any of the improvement added to Core. An alternative way to add this would have been via the $cspell_flagwords pre-processing in the prepare-cspell.php script. Then all Contrib would have got the changes even if they have their own file. Is that worth considering? Maybe I should have thought of that first? It would not be very hard to do.
I've pushed a couple of commits to the new MR, but this is a refactoring of the script, to add --verbose for detailed output, and to use global variables instead of repeating the getenv() dozen's of times. This will make the lines shorter and therefore easier to read and follow.
I have created a draft CR but it may need more detail.
https://www.drupal.org/node/3524446 →
The sample was too small (with too much variation) to be conclusive, so I don't want you to think I am stating that this change did nothing. But it will need more controlled testing and more runs, to get better average times, when we pick it up in future.
I think the diference should be noticed in the jobs using the artifacts.
On the small sample here, all of the linting jobs actully took longer in runtime in the 'after' pipeline.
Maybe we can do something in the script to check when 11.2.0-beta* is available.
Yes we can. You can merge this MR, but leave the issue open, as the background info is here.
Two "before" runs
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/496804
Composer runtime 1 min 27, artifact downloaded .zip was 158MB, unzipped 104,610 items, 717MB
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/496884
Composer runtime 1 min 42, artifact downloaded .zip same as above (not surprisingly)
Using this MR
https://git.drupalcode.org/issue/scheduler-3445052/-/pipelines/496898
Composer runtime 1 min 45, artifact downloaded .zip was the same 158MB (unzipped same as above)
Maybe I mis-understood what the effect of this MR would be. Do I need to look at the logs of the subsequent jobs that use the composer artifact?
Our GTD d9 branch uses the legacy driver, and those javascript tests passed, so that seems OK.
I've removed the temporary test lines.
NW because we were we going to create a Change Record
Changing the script constant from 11.2.x to const NEXT_MINOR_DEV = '11.3.x'; is enough to make 'check versions' pass. But this will not actually change any core versions being tested against.
CORE_NEXT_MINOR is still set to 11.x-dev. The description of this variable is
"Once the current minor-dev branch enters a beta phase, this is the dev branch for the *next* minor version."
The script allows this to be changed now to 11.2.x - I cannot see any prompt in the script that would alert us to changing this when we get to 'beta'. Is that something to add in?
Yes we can improve the visibility of those messages, both in the .php script and the template.
jonathan1055 → created an issue.
jonathan1055 → created an issue.
jonathan1055 → made their first commit to this issue’s fork.
Now we do not get suggestions for 'blocklisted'
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/52...
This is ready for review and feedback. Is it possible to verify the chromdriver change?
- - '--whitelisted-ips='
+ - '--allowed-ips='
Or do we just assume that because that is what was changed in core then it's correct here too?
That's even better. I have tested this locally, and found out that suggestions are only given when the specific word is supplied with a suggestion, you don't get derivative-created suggestions. I've pushed the changes and here's the test.
However, I have found that even though 'blocklist' is a known word, 'blocklists' and 'blocklisted' are not recognised.
spelling-test.md:2:40 - Unknown word (blocklisted) -- of whitelists and the blocklisted denylist allowlists
Suggestions: [blocklist, blockList, BlockList, blocklint, blacklist's]
So maybe we should not be adding those as suggestions, just 'blocklist' and all the 'denylist' variations.
I have added GTD before_script_action to all the D9 and D10+ branches to used flagged words. The jobs all fail as required. Here is one example
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/52...
Interesting that we get suggestions of the plural forms of the words:
spelling-test.md:1:23 - Forbidden word (blacklisted) -- Now we are using some blacklisted whitelist words.
Suggestions: [blacklists, blacklist's, backlist, backlists, blocklist]
spelling-test.md:1:35 - Forbidden word (whitelist) -- using some blacklisted whitelist words.
Suggestions: [whitelists, whitest, whitefish, whiteouts, whitebaits]
So I think we should add those forms too, then feed back to the c ore issue to add them also. There is no point in have 'blacklist' as a flagged word if we allow 'blacklists'
Adding the blacklist and whitelist words to /internal/.cspell.json made the 'check code' job fail as intended.
https://git.drupalcode.org/issue/gitlab_templates-3524087/-/jobs/5240491
I also re-ordered a couple of items in the file to make them the same as the default, for easier comparing.
Now that we have all the other flagWords, I had to add <!--- cspell:ignore grey queuing --> into the cspell.md file which is fine. But interestingly the word 'e-mail' was not reported as a flagged word. Probably the hyphen is treated as a word-break? Maybe this does not work in Core either?
Of course. I was thinking that we merged the values from the contrib default if they were not in the internal file. But we don't do that. We could I suppose. But the simple solution is to just put them in both files. I'll do that for all the flagWords.
Unintended change, sorry. New comments arrive, but the form values remain unchanged, so when I reply it makes the change back to the old value.
Before we change --whitelisted-ips in main.yml and main-d7.yml I would like to know why those did not fail the internal cspell job? We use the same scripts/prepare-cspell.php for our own checking, don't we?
I have added BEFORE_SCRIPT_ACTIONS to test the downstream projects, but they failed already even without that commit.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/52...
This highlights another problem I had already noted about cspell, that it is checking the contents of build.env when I do not think it should. The maintainer does not have control over what goes in to this file, and things like branch names and references are written which fail spelling. This confuses the log, and obscures the real spelling errors.
In this case, the job failed because that branch did not have its own .cspell.json so was taking the file from Core. The changes in this MR are needed when a project has its own .cspel.json which probably may not contain all the core flagWords.
If we added a change record, it could link to the core issues, and also explain about using cspell:ignore whitelist etc in places which cannot be changed yet (there are several in that Core MR)
Do we need to give notice of this upcoming chanage, say in Slack? The default allow_failure for cpsell is true, so unless any contrib maintainer has explicitly set it to false, then the pipeline will not fail.
Since preparing this issue, the Core issue has now been merges in 11.x so we should follow, as the original flagWords used in contrib pipelines was taken from the core file.
jonathan1055 → created an issue.
This all looks good. Updated the IS with the actual new variable.
@hamza_niazi can you add more details to explain how that is related to this issue?
All looks good and it was RTBC before that last change, so setting that again now.
I wonder how long you will have to keep rebasing this MR before it gets committed. This really is 'low hanging fruit' and the simplest of changes, so hopefully a Core maintainer will pick this up and merge it?
Yes I forgot the link, I have edited the comment now.
Here's a re-run using MR355, the tests are run not skipped (and the variable value is shown)
https://git.drupalcode.org/project/scheduler/-/jobs/5226529
Ready for review. When we work on ✨ Add coverage for test-only changes job Active we can include the javascript test too.
When the "test-only changes" job was first added in March 2024, Javacsript tests were run correctly, and not skipped. See the example in comment #47 on #3418831-47: Test-only job → so the error must have been introduced since then.
The cause of the new error is the recent issue
📌
Update PHPUnit jobs to new driver when D11 "current" is W3C-compliant
Needs work
where we introduced two new variables MINK_DRIVER_ARGS_WEBDRIVER_DEFAULT and MINK_DRIVER_ARGS_WEBDRIVER_LEGACY and set the correct one in a new reference snippet .phpunit-webdriver-variables which is called in the .phpunit-base script:.
The "test-only changes" job correctly extends phpunit, and uses most things from that job, but it has its own script: section. The call to the new .phpunit-webdriver-variables needs to be added here too. It's a one-line fix.
Here's a test showing that the ultimate value of the active variable MINK_DRIVER_ARGS_WEBDRIVER is blank. This is fine for run-tests.sh as it does not use this, but the plain phpunit binary needs this variable.
I have opened 🐛 FunctionalJavascript tests are skipped in test-only change when concurrent=0 Active
jonathan1055 → created an issue.
I have made the changes and here is the same branch test using concurrent=1. We see no warnings and instead the log shows junit.xml: found 1 matching artifact files and directories
Here is the test using concurrent=0. Again the warning is gone and we now see files/simpletest/phpunit-*.xml: found 1 matching.
This is ready for feedback and review.
On a separate note, there are two test files changed, and run-tests.sh (concurrent=1) runs them both correctly. But when using the phpunit binary (concurrent=0) the functionalJavascript test is skipped and we seeSSSSSSSS 8 / 8 (100%)
Time: 01:12.995, Memory: 6.00 MB
OK, but some tests were skipped!
Tests: 8, Assertions: 0, Skipped: 8.
This needs to be addressed, and could be done in this issue or separately. It may have been due to the driver changes recently, or may have never been working.
Two months without any response, therefore closing this.
If the changes are fixed, maybe you could add a comment and resolve the thread, then other reviewers will know that these things are not still outstanding. The closed threads can be viewed, and even re-opened, but it helps if they are closed when it it believed that no more changes are needed on them. I'm sure you know all this anyway, but just posting here for new-comers, as this project has not had Merge Requests and pipelines before :-)
Just seen there are review comments in the MR, so setting back to Needs Works.
OK, yes that's fine :-)
If this issue is just to take the existing pages as-is (which borisson_ says has been done) then you might as well merge this straight away. Then we'll be able to see the generated pages, test links and formatting and create follow-ups for the fixes. Gitlab Templates also has a script to check for incorrectly formetted urls in .md pages. That is only used internally in GT, but it may be useful to use that here too.
Here are the visual results from the sets of tests when we started with 4 #slow, then added the 5th and the 6th. "Total minutes" is the combined time of all tests, which is obviously much larger than the elapsed time, but I just added that for interest.
I have not done any more test runs with the two (7th and 8th) you mentioned in comment 23. There is some variation in the positions. The javascript and rules integration tests seem to have the most variability, looking the colours.
This is a great start. Some initial observations
The composer job shows that the project does not have a .info.yml file and whilst this is not a blocker it means that the assumption is that the project type is "module". Maybe it would be worth adding a coding_standards.info.yml and setting the appropriate values. Other project types are 'theme' and 'distribution' but neither of them are necessarily more correct than 'module'.
There are some spelling errors.
CSpell: Files checked: 36, Issues found: 105 in 17 files.
The number of distinct unrecognised/misspelled words is 51
Some of these are probably actual spelling mistakes but many are made-up words. It would be very important eventually to have the cspell job run green so that we can instantly see any new mistakes. I think we need to go through the list of unknown words (which is avaiable as an artifact file for download) and some of the instamces could be changed to use real words. Then the remainder can go into a custom project dictionary.
I notice that the actual mkdocs jobs is not run. This is actually by design in gitlab_templates, because issue forks do not have their own documentation url, so it would be re-building the live doc site on every MR pipeline. However, in this case I think its worrth building the initial site during these pipelines, to check menus and liks etc I can cusomise the mkdocs rules temporarily for that, if you like?
Six tests have @group #slow and we have _CONCURRENCY_THREADS: 8. So shall I mark those two you identified (LegacyHooks and Multilingual) with #slow, to make sure that they get run by the last two threads, and don't get picked together? Or is the tactic to always have more concurrency than #slow tests? I will try it anyway, to see what that shows us.
By the way, is there any way to widen the summatry output? The test classes are truncated at 60 chars in the sumary and three of them show as identical Drupal\Tests\scheduler_rules_integration\Functional\Schedule, and two as Drupal\Tests\scheduler\FunctionalJavascript\SchedulerJavascr
I was just doing a little bit of data collection and analysis, fiding average runtimes per test, but cannot programatically get one-to-one numbers when the test classes are not shown as distinct.
jonathan1055 → created an issue.
Sorry, yes you are right. Not sure what happened there, the tests were run with the correct change, so the results were right, I just had the wrong link. Here is the first of that run, and the two re-runs next door. 9 min 53 sec, 7 min 2 sec and 6 min 18
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5206786
jonathan1055 → changed the visibility of the branch mr352-3522611-curl-return-code to hidden.
jonathan1055 → changed the visibility of the branch mr340-3514999-w3c-driver-in-phpunit to hidden.
jonathan1055 → changed the visibility of the branch mr350-3521685-pin-webdriver to hidden.
jonathan1055 → changed the visibility of the branch mr348-3503613-refine-symlink-list to hidden.
jonathan1055 → changed the visibility of the branch mr349-3518751-extract-version to hidden.
jonathan1055 → changed the visibility of the branch mr250-3469828-concurrency to active.
jonathan1055 → changed the visibility of the branch mr250-3469828-concurrency to hidden.
Thanks. It was on my list of "this will be better for everyone in Gitlab Templates if I did it"
I'm sure you have this in hand, but just a reminder that CHANGELOG.md does not yet have these three commits:
https://git.drupalcode.org/project/gitlab_templates/-/compare/default-re...
sorry for the back and forth, this is what it was like trying to get core ...
No problem at all, I'm pleased you are giving good feedback and ideas.
With #slow added in SchedulerRequiredTest - 9 min 53 sec, 7 min 2 sec and 6 min 18 sec. Each re-run was faster, but I'm sure that is not salient.
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5198987
All feedback addressed, ready for final review and running all the downstream pipelines for a final check.
With SchedulerNonEnabledTypeTest also marked #slow, the times are 13 mins 50, 11 mins 48 and 13 mins 45
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5198370#L482
Thanks for the info. I marked those four tests with @group #slow and it was quicker. Initial run 17 min 16 sec, and re-run 20 min 36 sec, so it does appear faster with this.
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5196407#L482
Then I changed concurency from 4 (as in the MR) to 8 and got runtimes of 7 min 54 sec, 19 min 38 sec and 14 min 50 sec (I guess plenty of other factors are affecting the runtimes, hence why I did three)
https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/5196975#L482
the edits should be checked
It looks correct to me, exactly as in the issue summary.
For the Change Record, I think it would be worth adding that this was introduced in Coder 8.3.27 which was released on 8 Jan 2025
.get-utility-files as per MR commit
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
and with indentation fixed
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
get-file-via-curl.sh as per MR commit
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
and with indentation fixed
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
Filenames restored, all pipelines run green.
Yes that is tedious. This solution works for the single case reported, but ideally we should try to ignore all 'phpstan-...' inline comments. I think this is what Klausi was referring to, as being a lot of work. phpcs_codesniffer helpfully tokenizes their own phpcs- comments, so it is easy to disregard them using Tokens::$phpcsCommentTokens. But unfortunately the phpstan comments are not pre-tokenized, so we have to detect them individually, as I did in this PR.
If this approach is acceptable then maybe we can make that conditiona (if and preg_match) into a function that is callable from other places, so that the improvement can be utilized in other sniffs.
Yes that's clearer. I have applied the 3 suggestions as-is, but the indentation will need to be checked in the actual jobs, as the line breaks add one space at the start. So I'll re-do the false file names to check the failures, then reset.
Thanks. Good info in the issue summary
This has now even passed the usage of DrupalCI
That's great
Could you re-open this issue pease? We forgot to make the same fix in "test-only changes" - see this recent example. The job ended OK but we still see the warning
Uploading artifacts...
WARNING: junit.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/project/scheduler) D7 expected failure in .get-utility-files
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
Likewise D9-basic expected failure in .get-utility-files
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
When the script name is restored, the composer jobs run, and we get the intended failure in phpcs job (produced by the get-file-via-curl.sh script)
D7 https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
D10 https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
I've checked "test-only changes" with this Scheduler MR, all works OK
https://git.drupalcode.org/project/scheduler/-/jobs/5193386
All debug removed, and file names restored, so this is ready for final review and checking the downstream jobs.
This is a great idea. Would you like some help in getting a pipeline running, to actually build the documentation site?
This issue can probably be closed now?
Yes we can alter files in before_script but even if we changed the rules to trigger the job, I am not sure that this "diff" would pick up the changes when the pipeline is not running from a Merge Request. It might do, so worth a try. But also altering the rules kind-of invalidates the overall testing of the job.
I will try the above, but it might end up that we create a special MR here, marked as draft, which remains open and we run it in a new downstream pipeline (with other jobs skipped so we don't duplicate and hence waste resource each time)
With the .sh script file temporarily renamed in the repo the D10Profile composer job failed as intended
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
Likewise D7Basic composer failed as intended
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/51...
It's also great that we can cover all cases but 1 (the test only) via the GTD!
Yes it is. I have opened ✨ Add coverage for test-only changes job Active to see if we can automate that too.
The GTD pipelines (two of them) are all passing green, so I will rename two of the files to force a curl error, to check what we get.
jonathan1055 → created an issue.
I switched from a php script to a bash .sh as this replicates the existing curl statements more closely. Also addressed the feedback in the MR.
For test coverage, in main.yml for D10+ there are nine curl statements. Some of them are conditional on the project not having its own version of the file, as noted:
3 in composer
- scripts/get-gitlab-templates-version.php
- scripts/expand_composer_json.php
- scripts/symlink_project.php
All of these are executed in every Composer job
1 in phpcs
- assets/phpcs.xml.dist
Only executed when the project does not have its own phpcs.xml, so this is tested by GTD branches.
1 in phpstan
- assets/phpstan.neon
Only executed when the project does not have its own phpstan.neon, so this is tested by GTD branches.
2 in cspell
- scripts/prepare-cspell.php (always executed)
- assets/.cspell.json - only when the project has no .cspell.json, so is tested by GTD branches
1 in phpunit
- scripts/prepare-phpunit-xml.php (always executed)
1 in test-only changes
- scripts/test-only.sh
Not currently tested in any GTD - needs specific testing elsewhere
Yes I like that combination. The first curl (to get the 'curl' php script) is done at the start of the composer step, long-hand as in the POC alreday done. This php file will not be removed, and can then be used for all the subsequent curl requirements, of which some will be in conditional logic. It will be stored at the top-level $CI_PROJECT_DIR but won't be symlinked so should not interfer with anything.
Thanks for the feedback in MR. I have listed the nine curl statements that we have in main.yml. There are four in main-d7.yml.
Many of the curl statements are in conditional logic, which makes having a re-usable code snippet impossible for this task. At least I have never seen a reference used inside script logic, I don't think that can be done. I can think of two options to proceed (you may have more ideas)
- We replicate the return code checking in each of the nine
curlstatements. This is "simple" but also tedious, and it duplicates lines many times. Making an improvment would mean 9 x the changes - We have a php script that can be called in place of the
curlwhich can be inside a conditional block.
I favour option 2, not only because it will reduce the size of the template file and make maintenance easier, but it will also allow easy parameter input, making it more flexible, for example whether to exit or give a warning, the target folder for where to write the file, etc
I have pushed some changes to show proof of concept
Using GTD general testing MR8 with no customisations - composer is clean
gitlab_templates_downstream/-/pipelines/489340
Then with a temporary change to MR253 renaming get-gitlab-templates-version.php so that the curl fails, this is now detected and we can exit the job with a useful message.
gitlab_templates_downstream/-/pipelines/489373
I presume if a curl does fail we want to exit the job immediately? I think that is probably best, so it is clearer what has gone wrong. Some of the later processes in the job could probably succeed and would mask the actual problem.
Ready for feedback. If you are happy with this approach then I will look a making a reuable *execute-curl code snippet to use in all places.