Also, if LocalModules
is not extending DrupalDotOrgSourceBase
, why do we even need to split DrupalDotOrgJsonApi
into two classes?
Thanks for the extra set of eyes and testing. Merged.
I see great progress on this. Right now there is still a lot of code duplication between src/Plugin/ProjectBrowserSource/DrupalDotOrgJsonApi.php
and src/Plugin/DrupalDotOrgSourceBase.php
.
I assume that we are moving a big chunk to DrupalDotOrgSourceBase
and that everything is still WIP, so I'll hold of until I see it's marked as "Needs review".
fjgarlin → created an issue.
Merged.
Hi @cedrxc, thanks for the interest in working on it but I had already started and I'm just finishing. You are welcome to see the commits in the MR. I just made the most and decided to include GitLab CI in this issue, as it will also check the composer dependencies.
fjgarlin → created an issue.
Tested the actual sending with drupalpod
Merged. Thanks.
Rebasing d7-basic
on the fork should trigger the behavior we are after, but for whatever reason there is a conflict and cannot be done via the UI.
Merged!
That drush dl date
issue where it offers options is one the reasons why D7-api is failing: https://git.drupalcode.org/project/api/-/jobs/4315997
I think it might be due to D7EOL actions on the modules I guess (no stable releases).
In any case, yes, not related to this issue.
--
Thanks for adding all the test cases in the issue description.
Reviewed by @kbentham. Merged!
fjgarlin → created an issue.
We can create the block plugin here, like these two: https://git.drupalcode.org/project/drupalorg/-/tree/1.0.x/src/Plugin/Blo...
Then we'll worry about how to place it.
Right now we are in an interim situation where the content of the new site is managed by a small group of people, so probably the link should go to this issue queue and then we can triage and redirect from here.
Oh, I saw the commit fixing it in the other issue but I thought that it was part of that MR only, I didn't realize it came from here. I'll merge the other one soon anyway, and if I get a RTBC on this one 🐛 Tags contained in manual rebase of issue fork triggers pipelines Active , I can even make a new release.
I think the downstream API test failures are unrelated to this change
Yes, probably due to 🐛 PHP function list download no longer exists Active . I know that seeing those red lights is not great but I still think that they are good projects to test things in. I need to find a gap and try to fix those tests for D7 (which is a new one) and D10.
Created follow-up.
fjgarlin → created an issue.
Suggestions to try to remove some more /custom/
around the code:
Calculate the destination path as: $DRUPAL_PROJECT_FOLDER/../
# These links are created in the folder above modules/custom/$CI_PROJECT_NAME and will be used in addition to the project's own .eslintrc.json.
- ln -s $CI_PROJECT_DIR/$_WEB_ROOT/core/.eslintrc.passing.json $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/.eslintrc.json
- ln -s $CI_PROJECT_DIR/$_WEB_ROOT/core/.eslintrc.jquery.json $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/.eslintrc.jquery.json
Calculate the relative path to pass to run-test.sh. Not sure how to do it in bash but it'll be something like (untested)directory=${DRUPAL_PROJECT_FOLDER//"$CI_PROJECT_DIR/$_WEB_ROOT/"/}
:
# if _PHPUNIT_TESTGROUPS is --all then add --directory modules/custom/$CI_PROJECT_NAME
# Otherwise add $_PHPUNIT_TESTGROUPS (without the --group)
WHAT_TO_RUN=$([[ "$_PHPUNIT_TESTGROUPS" == "" ]] && echo "" || ([[ "$_PHPUNIT_TESTGROUPS" == "--all" ]] && echo "--directory modules/custom/$CI_PROJECT_NAME" || echo "$_PHPUNIT_TESTGROUPS"))
I'm thinking more an more to leave these "leftovers" for a follow-up issue, so I'm actually setting this to RTBC.
We can change the ones that we are sure about here and now, and for the others, we can either leave them as they are or create follow-ups.
As we are touching a good number of jobs, I triggered all downstream pipelines here: https://git.drupalcode.org/issue/gitlab_templates-3505585/-/pipelines/42...
I will try to review more in-depth tomorrow as I'm logging off earlier today.
After triggering the update and reindex, the data seems to be ok: https://www.drupal.org/jsonapi/index/project_modules?filter[machine_name... →
I created ✨ Track changes on migrations or update more regularly Active so we can take care of these things in the endpoint.
Also, we still seem to have some references to the "old" composed path (look in the code for /custom
. It'd be great to see if we can simplify those with this variable, if possible.
The MR looks really good. I made really minor suggestions to the documentation page to make it more explicit that they are just examples and what the purpose of the given examples might be for.
Happy with DRUPAL_PROJECT_FOLDER
. And yeah, +1 to mentioning it somewhere in the documentation.
fjgarlin → created an issue.
We are not running migrations with --update
. I’d expect that Drupal migrations catch the update at the source and propagate nodes that have been updated. But maybe this is not happening.
Migrations run once per hour, but just drush migrate:import drupalorg_migrate_project_module
The migration for projects is this one.
Due to our setup, we cannot use high water marks, but maybe we can use track_changes
or run with --update
once a day only. I just triggered a migration with --update
manually in any case right now.
Since this is "the path of the project inside the Drupal site", maybe we can call it DRUPAL_PROJECT_FOLDER
or DRUPAL_PROJECT_DIR
. I know that we could add "drupal" to many variables (and I'm actually not a big fan of naming drupal-things with "drupal"), but in this case I think it makes a lot of sense.
PROJECT_FOLDER
is kind of generic and could easily be confused with being the same as CI_PROJECT_DIR
. The MR looks good, so I think it's just deciding on the name.
There is a single field_active_installs_total
field, with just one value, usage can be tracked in
https://www.drupal.org/project/usage/webform →
.
The cron job runs weekly and grabs data from the previous week parsed. Right now we can see in https://www.drupal.org/project/usage/webform → how the last week parsed was February 2nd, so we're grabbing the data from the week prior to that, as the data needs to be be synced from D7 to D10. This is a temporary situation while usage data is being managed by D7 drupal.org
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.
Merged. Thanks!
fjgarlin → made their first commit to this issue’s fork.
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.
Finally done.
https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na... →
it went from +360K to +220K for field_active_installs_total
.
This should have already worked but the values are not updated, at least for the project linked in the issue description.
I added some extra debug so I can check logs to make sure that everything is running as expected: https://git.drupalcode.org/project/drupalorg/-/commit/7dcb07197b2268a795...
I'll continue monitoring it.
But just saw that you already transferred the commit!! Great!
Ouch! I had the MR ready, was just waiting for the pipeline to finish.
I think it was just to replicate what was done in here: https://git.drupalcode.org/project/drupalorg/-/blob/e31465608d1380345834...
ie:
https://www.drupal.org/project/ckeditor →
From the API endpoint is just a taxonomy term value, and then it’s really up to PB what to do with it. I think it can be cleaned up, and if we want to implement it, it can be done from scratch taking into account what is it we really want.
Currently, PB and filters only care about: active status and maintained status. In the early days, we were checking each possible value, and marking/showing some as "negative", but that changed with the introduction of other Plugins and it makes no sense anymore.
Should we want to show the descriptions of the values for the taxonomies, we could always do it, without the need to show a warning triangle.
Merged.
fjgarlin → changed the visibility of the branch main to hidden.
fjgarlin → created an issue.
Enlace al cartel.
I commented on the other issue, I still think that this should be done as author (person that created the node) is not the same as maintainers.
As mentioned in the issue description
The person that created a module might or might not be a maintainer of the module, and they might even cancel their account and the module would still exists.
Yeah, I think both issues can be valid.
Author, as it is (which is creator of the project) shouldn't be relevant.
What we want is maintainers, and this issue is the follow-up that
✨
Remove author from Projects
Active
was referring to.
So, I'd still remove author, clean up code, and then start afresh with maintainers. Note that this part of the API is NOT ready on d.o yet, so it won't be possible to do this yet.
Edito la descripción con tareas.
Fecha tentativa para hacer esto: 17 febrero 2025 @ 17:00 CET / 10:00 CST
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.
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.
Thanks for the clean up. It looks good. RTBC.
Thanks for that additional test. Merged!
there's a facility to write these plugins in a separate contrib module right now
100% - creating a new "source" or "plugin" via a contrib module is extremely easy.
Re #30. Correct, nothing is done so far and we'd need to add that functionality to the code in order to keep them.
Filters and category filtering might prove tricky unless you bring up all the information locally and then do all the filtering based on that.
I'm not sure that filtering would be needed if the module is already in the codebase. If we need/want some, I'd just do what the "core" plugin is doing, which is checking the "package" property for the categories.
Everything looks good and works as expected, thanks for the additional testing.
Let's clean up and prepare everything for commit. Setting to "Needs work" for the clean up. We also need to write a bit of docs on this but that can be done here or in the documentation issue.
Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3503966/-/pipelines/41...
The changes look good. RTBC.
I'd say go ahead and add those lines and test.
Great!
It'd be cool to try and see what happens with the following syntax:
workflow:
rules:
- !reference [ .default-workflow ]
And then try:
- A merge request ===> should trigger
- A commit in the default branch ===> should not trigger on fork (it would in the canonical project)
- A pipeline triggered via the web UI ===> should trigger
The code looks good. I triggered the downstream pipelines https://git.drupalcode.org/issue/gitlab_templates-3504083/-/pipelines/41... and it looks good too.
I'd put it RTBC but we're waiting for some testing on other external projects and also testing the usage of the reference
.
Thanks for that, I'm more interested in knowing if the actual emails were being sent. Also, can you try with a mix of valid and invalid emails and see what happens.
The code looks good.
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.
Sounds like a good idea. We'll need to test it well to figure out the syntax but definitely doable.
Thanks so much for the core issue and MR. Hopefully it's a quick one and it will go through, but if it takes longer we can always set the env variable for the nightwatch job in this part of the code: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes...
I'd rather have that fixed in core, so leaving as postponed and the above would be a plan B.
This is great, thanks so much!
fjgarlin → made their first commit to this issue’s fork.
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.
It seems that drupalInstall
sets that variable here https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/...
Not sure why it's not detected.
I'm happy with (a) as it'd clearly show there is a conflict with the codebase and the setup and that it needs to be fixed one way or another. Having a "web" folder means something for a project, but we can't know what that is, so we can just inform about it and offer a workaround.
There is also the argument for (b), but I think that doing so could just hide potential issues, which (a) would force to fix (either change the value of the variable for CI or rename that folder in the code).
Merged. From now on all new code will have lintin checks.
For phpstan, a baseline was created with the current issues. These can be done as follow ups as they'd need extra manual testing.
That's a great find!!
Something that we could do in the templates is to check early in the "composer" step if _WEB_ROOT
folder already exists in the codebase (before we create it) and then throw a warning either:
- Asking them to change that value and exit with an error.
- Informing them (via job output) that the templates will use another value, then change it and persist it via build.env
fjgarlin → changed the visibility of the branch 3503990-fix-gitlab-ci to hidden.
fjgarlin → created an issue.
Rebasing / updating fork is going wrong in all branches. Closing this issue altogether and will open a new one.
fjgarlin → changed the visibility of the branch 1.0.x to hidden.
100% agree with this change.
Thanks for investigating in the other issue. Once you get to the bottom of things we may or may not need to alter things in our scripts, but regardless we can include a section or page in the documentation to talk about "distributions", or even "themes", as the templates are optimised for "projects". In this new section we can do some recommendations like "include their own phpcs.xml file with the right ignore paths", etc.
Thanks for confirming it works on this version.
We will investigate further on this, for which we might open an issue in your project to investigate and be able to trigger pipelines.
Merged. Thanks!
I just triggered them myself here https://git.drupalcode.org/issue/gitlab_templates-3503426/-/pipelines/41....
The code looks good. RTBC.
Noting that this is a distribution, not a module.
PHPStan seems to be complaining about symlinks:
RecursiveDirectoryIterator::__construct(/builds/project/droopler/web/module
s/custom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/w
eb/modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/d
roopler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules
/custom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/we
b/modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/dr
oopler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/
custom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/web
/modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/dro
opler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/c
ustom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/web/
modules/custom/droopler/web/modules/custom/droopler/web/modules/custom/droo
pler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/cu
stom/droopler/web/modules/custom/droopler/web/modules/custom/droopler/web/m
odules/custom/droopler/web/modules/custom/droopler/web/modules/custom/droop
ler/web/modules/custom/droopler/web/modules/custom/droopler/web/modules/con
trib/search_api/tests/modules/search_api_test_excerpt_field/config/install)
: Failed to open directory: Too many levels of symbolic links
And PHPCS seems to be analysing core code, which is somehow placed inside the "module" custom folder: https://git.drupalcode.org/project/droopler/-/jobs/4201053 (see how "comment" module is being looked at)
Not sure how yet, but I think 🐛 PHPCS error in contributed module caused by core recipe.README.txt Active might be a better candidate for the issue that introduced the issue.
The related task only affected projects without composer.json
file.
I don't see how the related issue could cause this to be honest. This problem seems to be different.
You can easily test that if you change the version of the templates to use (https://project.pages.drupalcode.org/gitlab_templates/info/templates-ver...) in your .gitlab-ci.yml
file.
Right now, we are on 1.7.1
. You could change it to 1.6.14
and see if the issue happens.
So you'd need to change from ref: $_GITLAB_TEMPLATES_REF
to ref: 1.6.14
. You might need to set this too:
variables:
_CURL_TEMPLATES_REF: 1.6.14
I think it looks cool to just have the minimal readme on main
, and then each module just have the module's code itself. I don't think we need per-branch readme at all. Thanks for that.
Happy to get that daily task from GTD. Maybe we can set up the file in main
to trigger the 4 branches automatically (downstream style), that way we only need to set up one daily task.
My suggestion: Downstream: Project DrupalVersion [suffix]
Example:
- Downstream: API D7
- Downstream: API D10+
- Downstream: GTD D7 Basic
- Downstream: GTD D7 Composer
...
Or maybe we want to shorten Downstream
with D
, or Test
, or even an emoji →
. The idea of the same prefix is that they are all next to one another.
Maybe we can use this issue to unify somehow the names of the downstream pipelines for the way they are presented in the UI
I believe that the order of the jobs presented there is alphabetical
The changes look great and this could already even be RTBC for those two branches, but as we are creating more branches, I know that this will need extra commits.
It's a great start!
Feedback:
- "main": it doesn't probably need a ".gitlab-ci.yml" file, unless we want to trigger the branch pipelines from there (maybe a daily scheduled task). I'd make this branch the default branch, it shouldn't interfere with running pipelines as/when needed on the other branches.
- Then each branch's readme file should be about that branch/project only and explain briefly the case/s that it covers.
- Each branch's "gitlab-ci.yml" file might have specific overrides for when to run the pipeline if the default setup won't trigger then on commit.
Great to see the two projects connecting and proving that we can control multiple cases from this project.
it is useful to have one of the active branches as default to that a pipeline is run whenever a push is made.
We can tweak/override the rules
on any branch so the CI runs on push for that branch.