- Issue created by @jurgenhaas
- Merge request !237Issue #3459888: Job Upgrade Status: Variable MODULE_NAME should be overridable... β (Merged) created by jurgenhaas
- Status changed to Needs review
2 months ago 11:29am 8 July 2024 - πΊπΈUnited States cmlara
Some modules may have multiple submodules.
Should the upgrade status job instead of using the $MODULE_NAME variable search for each info.yml and test each module and submodule by name?
Should this be a hidden variable ? (Will the majority of users running a custom job from the gitlab pipeline need to change the variable?)
- π©πͺGermany jurgenhaas Gottmadingen
Some modules may have multiple submodules.
That's a great point. However, the situation might be more complex. Some submodules are optional, i.e. they can only be enabled if a certain dependency is available, where that is deliberately not a hard dependency. In such cases, the test can't just enable all submodules. I think, this should be addressed in a separate issue and carefully thought through.
Should this be a hidden variable?
Good question. I'd be OK to make this hidden if that's what we want to go for.
- πΊπΈUnited States cmlara
I think, this should be addressed in a separate issue and carefully thought through.
I could see some argument for that in that the module name code is wrong in general for even referring to the root module without the upgrade status job.
That said why make this overridable when we could use code to find the name of the root module from the info.yml? (Basically implementing half of the suggestion above, obtain module name from code without recursion)
. Some submodules are optional, i.e. they can only be enabled if a certain dependency is available, where that is deliberately not a hard dependency.
Those dependencies should already be present as require-dev. That doesnβt make it a hard requirement for deployment just testing. Though it does run risk of the job failing now. Fair that could be deferred to a follow-up issue.
- π©πͺGermany jurgenhaas Gottmadingen
we could use code to find the name of the root module from the info.yml
Fair enough, I'll give that a try.
- π©πͺGermany jurgenhaas Gottmadingen
I've updated the MR. The module name is now determined from the info.yml file name. It also verifies that there is exactly 1 info.yml file available, and aborts otherwise.
- πͺπΈSpain fjgarlin
Thanks for the back and forth. I'm back from holiday and still catching up on things.
I haven't read the whole conversation up there, but wanted to clarify this in case it's not clear yet.
Should the upgrade status job instead of using the $MODULE_NAME variable search for each info.yml and test each module and submodule by name?
We were testing this when creating the new job. The upgrade status tool checks all submodules.
I will review further tomorrow and try to read all the back and forth.
- πͺπΈSpain fjgarlin
From a quick glance into the MR, I think the most valuable part is to check if we are on a theme, and if we are then run
vendor/bin/drush --root=$_WEB_ROOT -y ten $MODULE_NAME
I'm not sure the rest of the logic in the script section above is needed.
- Status changed to Needs work
about 2 months ago 10:51am 18 July 2024 - πͺπΈSpain fjgarlin
I added some feedback to the MR. I think we can simplify it.
- Status changed to Needs review
about 2 months ago 10:10am 24 July 2024 - π©πͺGermany jurgenhaas Gottmadingen
Just returned from vacation and addressed all the great feedback in the MR.
- Status changed to Needs work
about 2 months ago 4:43pm 24 July 2024 - πͺπΈSpain fjgarlin
Thanks for the updates and the extra comments. I added a few more questions/suggestions to the MR. I feel like we should tackle the calculation of MODULE_NAME at its root if it's not correct for this job (which will make it not correct for other jobs that use it).
- Status changed to Needs review
about 2 months ago 5:03pm 24 July 2024 - π©πͺGermany jurgenhaas Gottmadingen
Great additional questions, thanks @fjgarlin. I've added comments in the code and also responded to the threads in the MR.
- Status changed to Needs work
about 1 month ago 10:16am 25 July 2024 - πͺπΈSpain fjgarlin
Thanks for the extra comments, it's clearer now why the code was like that and what each line meant. I wanted to make sure I understood what was going on before giving additional feedback.
Two main things:
- We can simplify the info files checks: No need to double check that info file exist, one check is enough. And no need to check if there are multiple info files in the first level. I don't think that this job should worry about it at all.
- We can improve/move the MODULE_NAME calculations in a follow-up issue. I don't want this to be a blocker as we'd also need to test nightwatch jobs. So let's create a follow up and make sure we link it in the code. - Status changed to Needs review
about 1 month ago 11:11am 25 July 2024 - π©πͺGermany jurgenhaas Gottmadingen
I've removed the extra checks as suggested. However, let me emphasize this related to
No need to double check that info file exist, one check is enough.
This will now fail for projects with no info.yml file in the root but only for sub-modules. This is because this job is not yet able to deal with sub-modules. It hasn't been before this issue and still doesn't.
Other than that, I've also created the follow-up issue #3463740: Improve MODULE_NAME calculation β
- πͺπΈSpain fjgarlin
Thanks for the changes. Re submodules, #3463740: Improve MODULE_NAME calculation β should solve the issue when there is no info file in the root folder.
Code-wise, I think it looks cleaner and it's a good reference for the follow-up issue too. Right now, I don't have any more suggestions (other than the small @todo line that I added). So it looks good.
It'd be great now to test it on different projects as none of the downstream pipelines have that job enabled. We'd need to test:
- Project where gitlab project name/folder name and module name is the same (most cases)
- Project where gitlab project name/folder name is different from the actual module name.A before and after for each of those cases would be awesome. Reminder on testing MRs: https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/
- π©πͺGermany jurgenhaas Gottmadingen
Thanks @fjgarlin, I've also marked all threads in the MR as resolved.
Regarding tests:
- The ECA module uses the upgrade_status test, an example can be found at https://git.drupalcode.org/project/eca/-/pipelines/233596. There is no test with the extra logic yet, since this is covered by the test where the module name gets determined by the info.yml file as well.
- For modules with a different name from the project, I don't have one on d.o but I have a private one:
- The failing pipeline (https://gitlab.lakedrops.com/apbs/components/entities/-/pipelines/1275495) uses the default templates, and it fails because drush wants to enable the
entities
module. - Then I tested with the MR (https://gitlab.lakedrops.com/apbs/components/entities/-/pipelines/1275500) and there it works as expected. Drush can enable the
apbs_entities
module.
- The failing pipeline (https://gitlab.lakedrops.com/apbs/components/entities/-/pipelines/1275495) uses the default templates, and it fails because drush wants to enable the
- Status changed to RTBC
about 1 month ago 4:31pm 25 July 2024 - πͺπΈSpain fjgarlin
The code looks good, the follow-up is created and the testing in #18 shows the before and after.
Also, this is isolated to just this job.We seem to have around 80 projects using the job: https://git.drupalcode.org/search?group_id=2&repository_ref=main&scope=b... (at least on the default branch).
- π©πͺGermany jurgenhaas Gottmadingen
@fjgarlin are we merging this before the next release?
-
fjgarlin β
committed d700bfbb on main authored by
jurgenhaas β
Issue #3459888 by jurgenhaas, fjgarlin, cmlara: Job Upgrade Status:...
-
fjgarlin β
committed d700bfbb on main authored by
jurgenhaas β
- Status changed to Fixed
about 1 month ago 8:44am 5 August 2024 - πͺπΈSpain fjgarlin
Merged, thanks for the back and forth, the changes and the follow-up issue.
Fixed. Automatically closed - issue fixed for 2 weeks with no activity.