jonathan1055 → changed the visibility of the branch 3463044-different-mysql-database to hidden.
We have two failures in d10-plus, without any modification:
Max php version https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/55...
Resolve all issues below to continue the installation. For help configuring your database server, see the installation handbook, or contact your hosting provider.
Database drupal_database not found. The server reports the following message when attempting to create the database: SQLSTATE[HY000]: General error: 1007 Can't create database 'drupal_database'; database exists.
previous minor https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/55...
Drupal\Core\Installer\Exception\InstallerException: Resolve all issues below to continue the installation. For help configuring your database server, see the installation handbook, or contact your hosting provider.
This is a shorter form of the same message,so assume its the same cause.
Just rebased, as the new MR362 was 170 commits behind (due to main being created in this fork nearly a year ago).
I think it might be worth trying to replicate the error from #23 in a GTD branch, then we can test it easily.
Yes the variants page would be a good place for something.
Also the validation jobs could have a paragraph, as that is where the problem started from. All linting jobs run with the 'current' variant, so if you want linting on a diferent one, you need to change what 'current' is percieved to be. The changes that rosk0 was trying to make was to alter the needs: for all the linting jobs, which was a perfectly reasonable thing to try.
Looking at the MR from the example pipeline, and the linked Slack thread, I can see what rosk0 was trying (to run all linting against D10 not D11) and it was not unreasonable to attempt it in that way. Maybe we can improve the documentation to help explain this, as rosk0 did take time to read the pages, he just made infererences that happend to be wrong.
We should definitely add a comment by that cd saying this job has to run in the top-level folder because cspell does not work with symlinked files. When I saw the "0 files checked" I had a vague recollection that we did know this once upon a time.
Here's the latest downstream job showing the updated failure message. I used your suggestion, and all I changed was the quotes, and to start a new line which was beter than wrapping.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/55...
I have removed the temporary lines, but decided to leave in _PHPSTAN_EXTRA: '--debug' as this will be useful for all testing downstream jobs.
Thanks for adding the verbose flags. I've run the downstream job, and we still get CSpell: Files checked: 0, Issues found: 0 in 0 files. I think this is because cspell cannot work with symlinked files, and I have just tested this locally and that is exactly what is happening. I get the same message, when all the files are symlinked. But if I create a new non-symlink file it gets checked.
So that means the whole approach of cd $DRUPAL_PROJECT_FOLDER needs to be undone, as we cannot run the command there. We will use the other approach (as mentioned in #2 and #3 above) to add the script name into the $ignore_files array.
Let me know if you still want to do this MR, you just need to revert the path changes in the cspell: section (but leave in the --progress change and the skip_ variables for now). Then you'll nee to make a change to prepare-cspell.php to add the .sh script name. Or if you do not want to do all this, that's fine, just say and we can pick it up instead. It's great having others collaborate on Gitlab Templates but equally there's no pressure to continue, just say.
Ready for initial feedback. The d9-basic job temporarily has DRUPAL_CORE: 9.5.11 and the failing job shows:
******************************************************************
The PHPSTAN binary was not found.
Expecting it in /builds/project/gitlab_templates_downstream/vendor/bin/phpstan
Set variable SKIP_PHPSTAN:1 to avoid running this job.
******************************************************************
The d10 job (which does not have any core modification) runs phpstan ok as usual.
I'm not sure how we can make the job end amber if the pipeline has already set allow_failure:false. Maybe we need to experiment with the exit code?
I've run the downstream pipelines for d9-basic and d10-theme for you (this can only be done by a maintainer of the downstream project). Also left another comment in the MR. The two jobs fail for different reasons, which is interesting and needs a closer look at.
Scrap that idea! I don't think it would work as the exists: has to check the files right at the start, as the pipeline is being created. At that point the vendor/bin/phpstan folder would not exist anyway.
So we can't conditionally prevent the job from being added, it will run and we have to give a warning message if phpstan is not available.
Whilst we are working on cspell, another feature that I've been wanting, and which would really help in testing this MR, is the ability to have the hard-coded --no-progress in the command. It should be a variable, which defaults to hiding the progress to maintain the current behavior, but can be set when doing testing like this, so that we see a list of the files being checked. Maybe it could be called CSPELL_SHOW_PROGRESS or CSPELL_VERBOSE or something. It defaults to 0 but can be set to 1 if required. Then the progress setting would be simply derived, in a similar way to what we do for CSPELL_SEARCH
OK that sounds reasonable. Then if a custom before_script loads it specifically, the job will run OK.
Another idea would be to add an exists: check in the phpstan job, so it would not even get added to the pipeline if there was no binary. Maybe that would confuse people, as it would not show why the job wasn't run. But it would be better in that there would be no amber warning, so the pipeline would be cleaner. The job should not end either green or red, so it would have to be amber, which indicates that the maintainer has to do something. I'd prefer just not to add the job at all. But happy to go with your preference if you convince me :-)
We might have just forgotten about cspell when we did the change
I don't think we forgot, as it was not an overall review of every job and where they execute. We were fixing things as and when the problems occured. Yes, lets try the cd way first.
I think the
$DRUPAL_PROJECT_FOLDERwas just introduced later than the latest changes we did in cspell job.
Yes the variable is new, but we did do earlier work to run eslint, phpstan and styleliny in the lower-level folder, before we had the variable.
Another angle to try might be to run cspell where it is now, but specify $DRUPAL_PROJECT_FOLDER as the path to check in. This would only involve changing the value of $CSPELL_SEARCH or maybe pre-pending the path to it in the execution call.
Thanks for opening the MR. I've made a couple of suggestions following the failed cspell jobs. I also pushed changes to not run all the unwanted jobs and variants. But I will leave the actual cspell work for you to continue on.
Hi avpaderno,
Thanks for raising this following this slack thread. You are right that we should not be including the internal files when checking spelling.
One very quick and simple way to fix this is to add the get-file-via-curl.sh file into the $ignore_files array in scripts/prepare-cspell.php.
But the problem stems from the fact that we run cspell from the top-level build directory $CI_PROJECT_DIR (which is /builds/project/seven in this case). We are already having to ignore "$webRoot", 'vendor', 'node_modules' and some other files such as 'build.env'. A more robust solution would be to run the cspell job in the Drupal project folder $DRUPAL_PROJECT_FOLDER (which is /builds/project/seven/web/themes/custom/seven in this case). Here we do not have the extra scripts and scaffolding folders, so it would be a cleaner way to execute the job.
There may be a good reason why this was not done in the first place, or it may have been investigated and rejected, so I can't say for ceertain that we can do this, but it is worth investigating.
Hi avpaderno,
The reason that PHPStan fails in this scenario is that Gitlab Templates is designed to run all linting jobs at it's "current" Drupal core version, which was Drupal 10 at the time the templates were being developed. Your pipeline is specifying 9.5.11 to be seen as the "current" core, which as you say, does not include PHPStan. There are (at least) three ways to solve this
- If you are not interested in the PHPStan output, then simply set variable
SKIP_PHPSTAN: 1and the job will not run - If you do want to run the job at Core 9.5 then you can probably load the requirements via a
phptsan: before_script - If the project is compatible with Drupal 10, then you could set "current" core to Drupal 10 and "previous major" to Drupal 9. Then all linting would run with D10, but you could still keep test coverage at D9 by using
OPT_IN_TEST_PREVIOUS_MAJOR: 1
In the case of the Seven theme → which I believe is the project you are tallking about, this does have compatibility with Drupal 10, so option (3) above might be your better choice. Or if this is not the project, please link to your pipeline / project where you found the problem.
It's all changed in the last two years anyway, because the link @drumm gave in #4 now produces
"core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php" did not exist on "11.x"
Hi liam morland,
Yes I confirm that the links with localhost are still shown in the Contrib test logs, but I am not sure that the printed output can be altered within Gitlab Templates. Or maybe it can, we will investigate.
In the meantime, you can go to 'artifacts' on the right and browse or download to see the generated .html files.
Adding that issue so we can get back to here when working on it.
Existing, with spelling failure has
This is .cspell-project-words.txt
madeupword
MR21 cspell log passes, because it has
This is .cspell-project-words.txt
webform
madeupword
I had an idea (after this was merged) that the new .cspell-project-words.txt might clash with the processing we do in < code>BEFORE_SCRIPT_ACTIONS to force and then fix a spelling error. And indeed it does cause the job to fail when run with action 'cspell-project-words'. It should be a simple fix, instead of creating the .txt file with > it should append with >> in case the file already exists.
Thank you. Merged and fixed.
I have just realised that the change to the pages job rules: was included in the merge.
We may want to look at that again, and change it. But it also may depend on if we c hange anything in 📌 Only rebuild documentation pages when source has changed Active which is another issue on my radar that I want to work on.
Thanks for the RTBC. Merged and fixed.
Those both work OK. In d10-plus 'webform' needed to be added to a custom .cspell-project-words.txt which is good, as we did not have any projects with that file, so this change increases test coverage of the spelling job.
In the d7-composer job we get a nice output confirming where the dependency modules have been located. It could be good to add a similar one- or -two-liner in the d10 template. But that's a separate issue. Both of these MRs are ready for review.
I think we only need to add dependencies in the d10-plus and d7-composer branches, as those have a composer.json file.
d9-basic and d7-basic do not have a composer.jon. I know that we can add dependencies via the .info(.yml) files but that is probably not needed here. Will two out of the four suffice?
Unticking that box caused the url to change from
https://gitlab-templates-downstream-1d35fd.pages.drupalcode.org/
to
https://project.pages.drupalcode.org/gitlab_templates_downstream/
... setting in the Gitlab repo admin area to fix the URL
By "fix" do you mean removing the extra -1d35fd in https://gitlab-templates-downstream-1d35fd.pages.drupalcode.org/
I was wondering what that string was. It's not related to a commit, as it is consitent over all commits in the MR.
I have just unticked that setting and will run the pipeline again to see. For reference, attached are before arnd after settings.
Docs done. RTBC (unless you would like the wording altered from this commit)
Just noticed that we need to update https://project.pages.drupalcode.org/gitlab_templates/jobs/phpcs
I've removed the temporary reference to MR18 in the "GTD Main" pipeline. Ready for merging.
Also updated the issue summary to explain why 'yml' is in the extensions list but not the rule for determining if the job is run.
This is now ready for review. The documentation test build is https://gitlab-templates-downstream-1d35fd.pages.drupalcode.org/
I added the README.md as a page, by copying the file into /docs in the before_script, but it gets ignored as the log says WARNING - Excluding 'README.md' from the site because it conflicts with 'index.md'. So I renamed it slightly when copying and that works fine.
I have also added a logo.png, which is a copy of the Gitlab Template logo, but with a spray-can effect using the Drupal official light blue, just to make it different.
Now adding the php-files-exist condition into the job rules
d7-composer still checks .info, .module and .test
d10-plus still checks .module, .php, .inc and .yml
d10-theme still checls .them, .engine and .yml
d10-profile still checks install, .profile and .yml
But main does not run the phpcs job at all, which is what this issue is all about.
This is ready for review.
With the updated GTD documentation files (see
📌
Add documentation .md files to test the pages job
Active
) I have added 'main' to the list of downstream branches that we can test. Here's the first run
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515276/-/p...
We see that the phpcs job has been added and runs. Ironically it does actually have a file to test against, because we have added .yml to the default list in assets/phpcs.xml. mkdocs.yml is the only file checked.
Doing that addition in phpcs.xml is correct, but I think we should not add yml to the list of extensions in $DRUPAL_PHP_FILE_TYPES because this should not be used to determine if the phpcs job gets added to the pipeline. It is correct that if other php files exist then the .yml files will also get checked in the job.
I've opened MR18 to use the 'main' branch. If this works, then when committed we can see if we can delete the new 'd10-docpages' branch. But leave it there until we know that the 'main' branch works OK for this purpose.
Here's the new repo
https://git.drupalcode.org/project/gitlab_templates_downstream/-/tree/d1...
Although I have just noticed that we had suggested using the 'main' branch for documentation. That may be a better idea, rather than have a new branch. I should have read the issue summary before working on it! I came from the other issue and just did it.
I have created a new branch called d10-docpages and pushed an initial commit. This branch does not have any .module or .info.yml or composer.json files. However it does have a mkdocs.yml and docs/index.md.
We can work on the changes neede to test the 'pages' job in due course. But the immediate reason for adding this branch is to allow testing of ✨ PHPCS is run even when it cannot check any files Active
I have added the other file types to the GTD branches and re-run the MR pipeline. We now see that:
d10-plus has .module, .php and .inc
d10-theme still only checks .theme, because our default assets/phpcs.xml does not (yet) contain .engine
d10-profile checks .install and .profile
Now adding the two missing extensions engine and yml into the assets/phpcs.xml we get
d7-composer has .info, .module and .test (no change)
d10-plus has .module, .php, .inc and .yml
d10-theme has .theme, and now .engine and .yml
d10-profile has .install, .profile and .yml
(the pipeline failure was unrelated - Nightwatch on Previous major. Re-run and it passed)
First commit just adds _PHPCS_EXTRA: '-v' so we can see which filetypes are currently being checked with no rules:
d7-composer has .info, .module and .test
d10-plus checks .module and .php
d10-theme checks .theme
d10-profile checks .install
We do not have any project with a .engine file so I will add one to the d10-theme branch. We have no .profile so I can add one in the d10-profile branch. We have no .inc so I can add that too.
But to add another angle to this, Core 11.1 phpcs.xml.dist has
arg name="extensions" value="engine,inc,install,module,php,profile,test,theme,yml"
This adds .yml into the list, and we should do the same. Coder does do sniffs on .yml files, for example
#2841649: Add sniff for _access: 'TRUE' in Drupal 8 routing.yml →
I have updated the issue summary to reflect the full list of extensions we are dealing with.
I used _PHPCS_EXTRA: -v and verified that the Coding Standards phpcs job is checking 0 files, just like our GTD branch
We can/should unify the extentions being checked
So is there any harm in adding 'info' and 'test' into $DRUPAL_PHP_FILE_TYPES and also adding 'engine' into the assets/phpcs.xml file? Then $DRUPAL_PHP_FILE_TYPES will work in D8+ and also in the D7 job. Or should the Drupal7 specifc extensions be added via hardcoding an extra line in the d7 phpcs job rule? If D8+ does not have any files ending .info or .test then I can't see any problem adding them into the list.
Maybe I will just start the work and see how it looks, the you can make a judgement when you can see the chnages :-)
The phpcs job has:
rules:
- *skip-phpcs-rule
- if: $_PHPCS_ALLOW_FAILURE == "1" || ($_ALL_VALIDATE_ALLOW_FAILURE == "1" && $_PHPCS_ALLOW_FAILURE != "0")
allow_failure: true
- if: $_PHPCS_ALLOW_FAILURE == "0" || $_ALL_VALIDATE_ALLOW_FAILURE == "0"
allow_failure: false
- when: on_success
There is no checking at all for what file types exist, so that is why the GTD main branch runs the jobs but checks zero files.
The default assets/phpcs.xml checks file extensions of php,inc,module,install,info,test,profile,theme
We currently have a rule .php-files-exist which checks for:
exists:
- '*.{$DRUPAL_PHP_FILE_TYPES}'
- '**/*.{$DRUPAL_PHP_FILE_TYPES}'
where $DRUPAL_PHP_FILE_TYPES is defined as php,module,install,inc,profile,theme,engine
These are nearly the same set. phpcs.xml has .info and .test which are not in $DRUPAL_PHP_FILE_TYPES. I think these are exclusively Drupal 7 filetype extensions, not used in Drupal 8+ ?
However $DRUPAL_PHP_FILE_TYPES has .engine which is not in the phpcs.xml list.
We could use .php-files-exist in the phpcs job definition for the main pipeline, and if we wanted to backport to main-d7 we could also add .info and .test but that's not a problem.
I have tried, but not been able to replicate this problem, using Core 11.1 and Scheduler 2.2.1.
To be honest, I do not see any way that the label of the title field could affect the validation constraints anyway, but I am willing to try it, and be proved wrong. But until I can replicate the problem and ensure that it is a Scheduler bug it won't get much further. Maybe there is some unintended interaction with another module which is removing your validation constraints?
Thanks for the feedback. The patch will need to be converted into a Merge Request, then we will get Gitlab Pipeline testing.
@wells or @junaidpv can you do this?
Thanks. To verify, I have just re-run the 'next minor' job that I linked to in #10 and it now does exactly as intended. The output is clearer, and we can see that the problem was caused by DRUPALORG_CI_SERVER_URL being empty.
https://git.drupalcode.org/project/scheduler/-/jobs/5459742#L38
I have added links to the documentation sites, as it can be confusing having two outputs here. This is only due to the temporary override in the .gitlab-ci.yml.
I'm going to see if we can find a better way to do this in Gitlab Templates generally, so that there is only one temporary documentation site, and the "live" site would only get updated on merging into the main repo.
Thanks for the above, and also for leaving your edit in strikethrough. That helps us to know from what angle you are approaching this, and I respsect your point of view about BC.
So, given that these flagWords are not in a dictionary, but this work is part of a general advancement of the state of Drupal code, I would still suggest that it should be opt-out, not opt-in. Many of the projects that have their own cspell config are doing it for other reasons, not to avoid using core flagWords.
It's good that we are having this discussion.
This commit has now just been included in default-ref
For info I did a search on how many of these words are currently in the contrib codebase
Search for blacklist returns 2,502
Search for whitelist returns 3,751
Many of these will have to be ignored, not fixed. So we could see an increase in the simple counts. Maybe we can work out some regex which would exclude the "cspell:ignore" lines, so we can then see how these counts change.
Debug and test lines removed, ready for final review.
This is actually a good improvement to check the variables for blank first, it makes it more reliable to fail if anything is empty.
Thanks. I've removed the test variable.
No problem. I used the MR suggestions, to make it easier to see what was original and which new changes needed to be reverted.
Point 1 in PHP -> Quotes does not seem to be rendered properly. I have not done an exhaustive review of all the pages, but just mentioning things as I spot them. But it may be good to get these renames committed soon(ish) so that we don't get rebase errors again if new issue branches are opened.
Thanks for that suggestion, yes I think it is better to detect empty variables and fail directly, rather than try to let the curl fail. I've pushed a change, and set EXIT_CODE and then the same exit block displays all the values. I used -1 so that we do not clash with any actual curl exit codes which are all positive
Here's the test in d7-composer which has the blank value
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/54...
Ready for review.
Thank you for updating default-ref. I think this was a good time to do it.
It seems like there won't be any changes needed to Gitlab Templates right now, but we could leave the issue open for a few days in case there is any problem when other Contrib projects start using 11.2.0-dev
The pipeline failed but it seems that some of the spelling errors are in files that have been renamed. I updated the issue fork (using the gitlab UI when browing the repository) but that has not solved it.
I have aligned the pipeline changes with 'main' so we now run cspell and do not allow it to fail.
I have confimed that the core revert has solved this and the jobs run as before, certainly in Scheduler where I use @group annotation in the test files. Here is the job log from a re-run of the same branch as the failed one in the issue summary.
But ... bear in mind that I am using the "main" branch or a gitlab templates MR which is obviously on 'main' so therefore with the change made in 📌 11.2.x is released - update NEXT_MINOR Active . The 'default-ref' does not yet include this commit, so most contrib testing will still be using '11.x-dev' for next minor, not '11.2.x-dev'. It would be beneficial to update the 'default-ref' soon, so that everyone is on the 11.2.x-dev branch and won't be affected when work progresses on 11.x-dev to re-do the new test discovery work that was reverted.
Tested in the d9-basic downstream branch.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/54...
Ready for review
There could be a few conflicts here, so I suggest we get MR2 and MR4 merged first, then see where we are.
In d10-theme the intended failure works, and we get the message with the updated output.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/54...
For error 1, where $_CURL_TEMPLATES_REPO being empty in the phpcs job even though the value is written to build.env, maybe this is due to the variable hierarchy? It could be that if the variable is defined in the custom .gitlab-ci.yml (or in this case the downstream 'trigger' job) then this takes precedence ov the value in build.env. If that is it, then it's fine.
For error 2, I'm goint to try again without making the variable non-blank, to see if that effects the exit code.
Hi @ghost of drupal past,
If we adopted this change, the Drupal Project would benefit by having less stupid rules.
Can I suggest you write this in a positive way, rather than just saying you want to remove the rule.
Also can you fill in more of the issue summary templates, for example give a link and show the current text of the standard that you want to change.
[mymodule] may be common, it is also a misspelling.
Ah. OK thanks for that background. Yes of course we don't want to add it back.
Now that you've added those custom words to the project dictionary this is RTBC. But also so is MR4. So which ever you merge first, I can rebase and resolve the conflict.
I've removed the temporary 'pages' override, and the unwanted SKIP variables. When this MR is merged (maybe using the 'merge' button this time?) then I can rebase MR2 spelling to remove the conflict.
Back to RTBC.
@jonathan1055, thanks for setting this up to temporarily deploy the pages. That is great!
You're welcome. I note that when I run the pipeline the documentation site is built in the issue fork, which is good
https://coding-standards-3527422-de7e89.pages.drupalcode.org/
But when you run it, as a maintainer, the site is built in the "real" location, thus updating the live pages with each MR push. This is one of the reasons why the 'pages' job is set to only run on commit to the main branch. It would be nicer if all MR pipeline built the documents in the issue fork, then we could always try out changes without altering the live site. I will revert that change before you merge the MR. (NW for that)
I possibly went a bit out of scope adding the navigation. I wanted to see how it all fit together.
Not at all, that's exactly what we need to get the live site usable.
I've made the change, and doing some testing in GTD.
In the d7-composer branch the I set _CURL_TEMPLATES_REPO: '' which did not cause a problem in the composer job, because we re-calculate if it is blank. The build.env file has
_CURL_TEMPLATES_REPO=issue/gitlab_templates-3524073
_CURL_TEMPLATES_REF=3524073-drupalorg-ci-server-url
GITLAB_TEMPLATES_VERSION=3524073-drupalorg-ci-server-url (branch)
which is what we expect. But the subsequent phpcs job fails, the value of _CURL_TEMPLATES_REPO seems to have been lost when the script executes
# REPO and REF need be non-blank to ensure that the curl will give an easily detectable return code.
_CURL_TEMPLATES_REPO=${_CURL_TEMPLATES_REPO:-repo-is-blank}
_CURL_TEMPLATES_REF=${_CURL_TEMPLATES_REF:-ref-is-blank}
and we get the url with https://git.drupalcode.org/repo-is-blank/ at the start. This is problem 1, but more serious is the fact that the curl command still does not give a failed return code even when it fails. See this d7-composer phpcs log. The command is deemed to be a success and we don't get anything in $EXIT_CODE so there is no message when we'd like there to be one.
Any ideas?
jonathan1055 → changed the visibility of the branch main to hidden.
I have added the change to make the job fail red, not amber warning. Nice work on editing to remove more unknown words. The only question I have is that you have changed all mymodule into my_module to remove 'mymodule' from the project dictionary. But I would suggest that "mymodule" is quite a common word used in program documentaion and examples, so I thought it would be better to allow that word. If we don't then future updates are likely to fail on spelling and we'll end up with varying versions of myModule, my_module, and other examples. It's fine if you want to stick to my_module, I'm just raising the discussion :-)
I have the nav locally and it works locally. So, let me know if it can be added here.
I suggest we focus here on the spelling, and do the navigation on 🐛 Fix display of GitLab pages Active where I have already made some comments about it.
That worked, I added a index.md and now the top page is also shown, with no 404
https://coding-standards-3527422-de7e89.pages.drupalcode.org/
Currently, the navigation is being generated on-the-fly by file discovery in the /docs folders. This means that the menu is purely alphabetical, and every menu link is the same as page title, in title case. For exampe we get 'Php' and 'Api documentation examples'. It may be better to add a nav: section in the mkdocs.yml so we can determine the best order of pages and give proper titles. For example:
nav:
- Home: 'index.md'
- PHP: 'php/php.md'
- Introduction: 'docs/php/php-coding-standards.md'
- Namespaces: 'docs/php/namespaces.md'
etc.
I think @quieteone mentioned earlier about navigation, so this change may already be in hand on a local branch.
We can't tell if this MR will fix the real problem, but it looks like it might. When ready to merge, the temporary changes need to be undone, but this is ready for review and feedback.
This may or may not be a help, but the pages have (mostly) been built OK within this issue fork. Here's an example
https://coding-standards-3527422-de7e89.pages.drupalcode.org/composer/pa...
There only problem with the very top level page - it is still 404
https://coding-standards-3527422-de7e89.pages.drupalcode.org/
I am guessing this is because the default file is not set correctly - I will add a docs/index.md and see if that fixes it.
I can work on this, by making the 'pages' job run temporarily in an MR so we can debug and see what is going on.
Would you like this branch to temporarily build the actual documentation site, then we could check the built pages. I can add that here if it would be helpful.
Regarding the comments earlier about there being no automatic commit comment due to the lack of formatting of the commit message, if the Merge Request had been actually merged here (using the "merge" button at the end of the issue thread) then it does take the automatic message from the text box and created a commit comment. That would also have saved the effort of manually closing it afterwards.
jonathan1055 → created an issue.
Ignoring the section of the file that has SQL keywords made a big improvement
CSpell: Files checked: 37, Issues found: 63 in 13 files.
The number of distinct unrecognised/misspelled words is 25
I have also added a project dictionary (using the default .cspell-project-words.txt filename), and we get down to:
CSpell: Files checked: 37, Issues found: 38 in 8 files.
The number of distinct unrecognised/misspelled words is 20
I will stop here, and let you review and give feedback, before doing any more additions or corrections.
Initial cspell results
CSpell: Files checked: 37, Issues found: 105 in 17 files.
The number of distinct unrecognised/misspelled words is 51
The shortest wordlength defaults to 4, so for some made-up ids we can shorten it from 4 to 3 letters. I fixed the 'lorem ipsum' phrases. The words are in a standard dictionary, but the pages were not using the generally agreed spellings, so I fixed that. Plus some actually misspelled words, and UK -> US English variants. Re-run and we have the reduced counts:
CSpell: Files checked: 37, Issues found: 81 in 16 files.
The number of distinct unrecognised/misspelled words is 37
There are various ways to proceed here, we can create a project words file to add the unrocognised words, but its probably better to make the docs use real words if possible. Some words may need to be added to a custom dictionary, but not all of them.
I have pushed an initial commit to check what happens with no customized skip or opt-in settings. The four validation/linting jobs that are run are:
- composer-lint - this checks the overall syntax of files, there is no harm in running it. But it may not be needed and just wasting resource, so could skip it.
- cspell - we want this
- eslint - this checks the formatting of .yml files, so we should keep this
- phpcs - this is probably checking the .md files for standards. It passes, so is OK. But we may decide to skip it
The two validation/linting jobs which do not get run are phpstan (we have no .php files) and stylelint (we have no .css files)
MR2 is ready for review and feedback.
Does this projects issue fork and remote access follow the same defaults as other projects? I was able to create the issue fork above, but having difficulty pushing to it. Does it use the same username and password as other Contrib projects?
I had no problem pushing to the other issue branch. Currently git remote -v shows:
coding_standards-3521924 git@git.drupal.org:issue/coding_standards-3521924.git (fetch)
coding_standards-3521924 git@git.drupal.org:issue/coding_standards-3521924.git (push)
coding_standards-3527481 https://git.drupalcode.org/issue/coding_standards-3527481.git (fetch)
coding_standards-3527481 https://git.drupalcode.org/issue/coding_standards-3527481.git (push)
origin https://git.drupalcode.org/project/coding_standards.git (fetch)
origin https://git.drupalcode.org/project/coding_standards.git (push)
Note that the first issue used git@git.drupal.org whereas the new one just has https://git.drupalcode.org. I followed the same normal procedure, using the commands shown in the issue summary above.
I have opened 📌 Run cspell and other linting/validation jobs Active as the follow-up.
jonathan1055 → created an issue.
OK. I've put this back to fixed. Then please can you close MR1 as it was not merged, and I will open MR2 on a new issue. That just seemed like make-work as we have all the infrastucture (issue branch and MR) right here, that's why I continued to use it. But no worries, I will do that.
MR1 is ready for review and feedback. The four validation/linting jobs that are run are:
- composer-lint - this checks the overall syntax of files, there is no harm in running it. But it may not be needed and just wasting resource, so could skip it.
- cspell - we want this
- eslint - this checks the formatting of .yml files, so we should keep this
- phpcs - this is probably checking the .md files for standards. It passes, so is OK. But we may decide to skip it
The two validation/linting jobs which do not get run are phpstan (we have no .php files) and stylelint (we have no .css files)
Given that MR1 was not actually merged, I have just rebased it and pushed a change to run the cspell job, and not skip the others either, just to see which jobs run. Most of them should not, as we don't have .php files etc. It makes sense to re-use this MR rather than create a new issue and new MR if this one does the work necessary.
@fjgarlin Please could you re-open this issue. Now that we have changed the get-file-via-curl.sh script to use the new $DRUPALORG_CI_SERVER_URL this variable should really be displayed in the log when there is a curl failure. I just tried to trigger manually a Composer job from a pipeline first run a week ago, and the new curl script failed. I think it was because the variable $DRUPALORG_CI_SERVER_URL did not exist in Gitlab Templates then, but when downloading the gitlab_templates/scripts/get-file-via-curl.sh it gets the latest version, which is using the new variable. See this job log
I know it's an edge-case, and probably will not happen often, but it would be better to display all of the variables values used, then it is clearer to see what happened.
Here is the commit
https://git.drupalcode.org/project/coding_standards/-/commit/6e7ecb58dca...
I think the reason there is no automated comment is that the commit message text was not of the required format (starting Issue #nnnnnnn
The next bit is to get GitLab pages generated.
In #14 I talked about making the pages, but in #15 you suggested that should be in a follow-up. Happy to help there, when that issue has been created, or continue here if you want.
Thanks for all your input above. It's a lot to grok all at once. Thanks for marking it a bug, but are you sure it's a bug in Gitlab Templates, as we cannot control the changes to run-tests.sh that caused this shift between 11.1 and 11.2. Shouldn't this be resolved in Core? I need to do more testing and investigation, to see how we can progress from here, for the existing contrib projects using @group to divide up their test jobs.
This core Change Record → explains what was done and how to fix tests. We now need to work out the solution for Gitlab Templates. We might need to add some version-checking logic in the phpunit job, so that we have a seemless transition from 11.1 to 11.2, instead of make it dependent on changes in the 'next minor' variant.
I've made the change to use nage to $_CONFIG_DOCKERHUB_ROOT and I used == null with when: never. The conditional != null did not appear to work as expected above (comment #13) but we know that positive testing for == null does work.
Tested here https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
Ready for review.
A similar problem is happening in Project Browser, where no tests are specfied when running against 11.2.0-dev. The executed run-tests.sh command has the final arguments of --types PHPUnit-Functional --module project_browser
https://git.drupalcode.org/project/project_browser/-/jobs/5393446
Background on this slack thread
I avoided the deprecation job from running by adding the rule that check for current. But I think your solution of checking $_CONFIG_DOCKERHUB_ROOT != null might be simpler to understand, so I'll try that.
We could even make that rule reusable and add it to the .composer-base job as well.
If we stopped the Composer jobs from being added, then we'd also have to add that rule to every other job which "needs:" a composer job. If we did not do that, the pipeline would not be built because the 'needed' job would not exist. But if you think it is worth editting every single job just so that we only show the "pipeline set-up failed" job in this scenario, then we can do that. It would make the problem very clear for anyone, as they would only get that one job.
Testing for && $SKIP_COMPOSER != null did not prevent the deprecation job running, see this test which has an odd sh: : unknown operand at the end of the unwanted deprecation log.
So I have used the same method that we have in the D10+ template, where we use the *opt-in-current-rule. This aligns the templates, and also achieves what we want, namely no confusing deprecation job, just the 'pipeline set-up failed' job.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
This is ready for review and feedback.
We did not have this job at all in the D7 template (sounds like you had already discovered that).
I have used GTD MR17 to erase all variables: and here is a test using default ref as-is. We get a .pre deprecation warning job, but obviously it cannot show anything helpful.
Here is a test using MR364. We get the new "pipeline set-up failed" job. But the additional deprecation job is confusing. The log has nothing, because the image cannot be loaded, due to the variables being blank. So I am going to make that better by using the hard-coded alpine image, like we do with the new checking job.
OK, I understand that. It was a simple change, that's why I opened the issue.
The reason was that Scheduler tests at 'next minor' have so many deprecations from Address (and other modules) that the log length exceeds the Gitlab limit, and is terminated short of the end. I guess I need to find a way to silence these deprecations, otherwise the log obscures any real test failures and other deprecations.
Yes let's do it here. Need to do something else right now, but will follow up tomorrow or later.
Following my thoughts in #8 I have opened
📌
Add core flagWords even when project has a custom .cspell.json
Active
so that even those contrib projects that have their own .cspell.json will be alerted for use of these flagged words.
Thanks for your comments in the MR. I changed the jobname to "Pipeline set-up failed" and here's the test
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
jonathan1055 → created an issue.
It might be a PHPUnit 10 vs 11+ thing.
Both the failed and woking jobs are running PHPUnit 10.5.46
Yes is does seem like 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active is linked with this, as that issue was the reason @catch asked me to test in #28 on #3469828-28: Lower default concurrency → (not to test this specifically, but to see if affected run times)
That commit also has lots of changes to run-tests.sh too.
This is really good that we are now testing 11.2.0-dev so that we find these things early. My tests are using ref 'main' and that is how this suddently showed up now.
Testing in GTD by trying to erase the variables: here in the 'd10-theme' downstream trigger did not work. So I used GTD MR8 directly to test this. It worked fine and we get the warning job
https://git.drupalcode.org/issue/gitlab_templates_downstream-3503337/-/p...
I used three different 'warning' symbols, and the yellow one looks the best I think. Now we need to settle on the job name. It does not necessarily have to include the word 'pre' as that stage name is shown in the pipeline.
I have raised Gitlab Tempates issue 📌 MissingGroupException: Missing group metadata with run-tests.sh in 11.2.0-dev Active . It may end up being a core problem, but need to investigate further.