N°7506 - Add method to check if a specific module is installed in iTop

[Hipska]

I wasn't sure to which class it could be added. Might as well be a static method on Utils or even somewhere else.

[Molkobain]

Discussed with Denis, as-is the PR doesn't suit us for the following reasons:

• Just looking for a module in that class / table can return true even though the module has been uninstalled. This table is an history, not the current state of the instance.
• Ideally, it would look for the most recent "datamodel %" entry, than look for the module at the exact same datetime.

[Hipska]

Oh, that's right indeed. I'll see how it could be retrieved the best then...

[Molkobain]

It seems that the ModuleInstallation class is already ordered by desc. installation datetime (see here), so you could:

• Make a first call to core/get, with a limit to 1 for SELECT ModuleInstallation WHERE name = "datamodel", which will return the last compilation datetime.
  • Mind to make a cache for that datetime info, so you don't have to make the call again if you are checking more than 1 module.
• Then make a second call to core/get, also with a limit to 1 for SELECT ModuleInstallation WHERE name = "<YOUR_MODULE_CODE>" AND installed = "<DATETIME_FROM_PREVIOUS_CALL>"

[Hipska]

Yeah, also had something like this in mind. Please see 8b8849d.

[Molkobain]

Can you add a unit test that check:

• That a common module is present (eg. itop-structure as tests run on an iTop 3.2, or itop-config)
• That a fake module is not present (eg. fake-module)

[Hipska]

I lost a full day trying to get the tests working, so I added what I got and should work, but the old version of phpunit does not seem to do what I'm expecting it to do somehow..

[Molkobain]

Thank you very much for coding the extra mile with the unit test, it means a lot.

[Molkobain]

Just discussing the unit test with @odain-cbd before merging

[odain-cbd]

Dear Thomas,

I just cloned your fork and indeed there issues with PHPUNIT somehow. we will upgrade this lib.

[odain-cbd]

please see my proposal

[Hipska]

I don't feel like this method fits well into the Utils class, but I have moved and fixed the test.

[Molkobain]

Discussing this with Stephen, wouldn't it be better if we could pass an optional "min version" for the module?
It could be helpful to adapt what we send when there is a DM change in a module version.

[odain-cbd]

"I don't feel like this method fits well into the Utils class, but I have moved and fixed the test."

I agree with you. maybe it should be located somewhere else than Utils. I just proposed it to let you validate this feature. now that is tested you may easily move it in a proper place. maybe a new class?

[Hipska]

Discussing this with Stephen, wouldn't it be better if we could pass an optional "min version" for the module? It could be helpful to adapt what we send when there is a DM change in a module version.

@Molkobain I'm not sure if this should also be in the same method.. What do you propose as its prototype then? An optional argument or the method returning the latest version number (as it is now putting into the logs)?

I don't think the versions are that relevant for the collectors. The one I know that has module checks, only checks if the classes are available: example. For my own use-case I wouldn't mind the version as well. Do you have other examples?

I agree with you. maybe it should be located somewhere else than Utils.

@odain-cbd I still feel it fits well in RestClient similar to GetFullSynchroDataSource also is located there. Maybe it could be a method of Orchestrator? Similar to AddRequirements and CheckRequirements, but those are related to PHP modules.

[Molkobain]

For instance, if you collect data about racks or servers, the "width" field was not added ar first in the molkobain-datacenter-view module, it came later on. So if you want to collect this, you need to know which version of the module is present.

Regarding the signature, I'd say something like the following, but totally open to other suggestions:

public static function CheckModuleInstallation(string $sName, ?string $sMinVersion = null, bool $bRequired = false, RestClient $oClient = null): bool

If $sMinVersion is null, then don't check the version at all. Otherwise, ensure that it is >= (You can use version_compare() to compare 2 string version numberings)

[Hipska]

I was more asking for current existing collectors that actually check for a version or have a need for it. Your example is also a valid one.

I'm more in favour of this method returning the installed version, so the version can be checked elsewhere.

[Molkobain]

Discussed in DM with Hipska, prototype will remain the same, but $sName will allow to pass the complete module identifer as well (eg. "some-module" or "some-module/1.2.3")