Job Upgrade Status: Variable MODULE_NAME should be overridable and the drush command should also work for themes

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?)

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.

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.

we could use code to find the name of the root module from the info.yml

Fair enough, I'll give that a try.

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.

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.

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.

I added some feedback to the MR. I think we can simplify it.

Just returned from vacation and addressed all the great feedback in the MR.

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).

Great additional questions, thanks @fjgarlin. I've added comments in the code and also responded to the threads in the MR.

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.

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 →

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/

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 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).

@fjgarlin are we merging this before the next release?

Yes, I am doing so now. Sorry for the delay.

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.