search

icatproject / topcat

GUI to work with multiple ICAT and IDS servers.
Other
1 stars 9 forks source link

Settings for 'mail.*' in "topcat-setup.properties" required even if 'mail.enable = false' is in "topcat.properties" #389

Open bmcann opened 5 years ago

bmcann commented 5 years ago

Hi Folks,

I ran into a problem with the TopCat installation whilst deploying a stand-alone instance of the ICAT/IDS/TOPCAT stack. When I set mail.enable = false in topcat.properties the installation still requires some or all of the following to be defined in topcat-setup.properties:

Without these settings, the deployment of TopCat fails. I would expect that the deployment process would not require these settings to be present if mail.enable = false were set.

Best wishes, Bruno

brianritchie1312 commented 5 years ago

The setup script always calls actions.createMailResource() with these properties even if mail.enable is set to false, so they must appear in topcat.properties (it might be OK to leave them blank). On the face of it, it would be simple to change the setup script to skip this if mail.enable is not true; but we would need to make sure that Topcat itself doesn't require them to be set. It's not immediately obvious, as (for example) Topcat makes no direct reference to "mail.host"; but it may still assume that createMailResource() has been called (I don't know what it does). I would want to be sure of this before making the call to createMailResource conditional.

A simple "fix" would be to change the installation notes to make it clear that the properties must appear even if mail.enable is set to false!

brianritchie1312 commented 5 years ago

Having looked in more detail, I think that this is more complicated than it seems. The mail.host etc. properties are defined in topcat-setup.properties, and are intended to configure the environment under which topcat is installed. topcat.properties contains properties used to configure topcat itself. So topcat-setup.properties is processed before topcat.properties, and is in some sense more static. The properties mail.host etc. are required to create a Java mail resource; at the point where this happens, the setup script doesn't know anything about the mail.enable flag in topcat.properties. (I see no reason why it couldn't, but nonetheless it doesn't.) So we can't test mail.enable to determine whether or not to try to run actions.createMailResource().

We could change the setup script so that it only calls actions.createMailResource() when (all) the mail.xxx properties are defined in topcat-setup; but strictly, if we don't call it, then we should check that mail.enable is not true in topcat.properties, and I see no way to do this in the setup script.

That said, I've experimented in a test VM with not doing createMailResource() when mail.host is not defined, and it seems to work OK. So I'm minded to make this change, and to update the installation notes to make it clear that mail.host etc. must be setup properly if mail.enable is set to true.

bmcann commented 5 years ago

Hi Brian,

Thanks for your work on this. My main motivation for opening this issue is that it presents a gotcha in setup procedure that we all seem to know but is not written down anywhere, so it is best to document these things. I'm updating the documentation now so as to avoid this.

It isn't something that needs an urgent fix, it is more of a nice to have. I just thought there is value in opening an issue on unexpected behaviour encountered by a new-comer to the project.

Best wishes, Bruno

brianritchie1312 commented 5 years ago

I believe I've found a problem with this, as a side-effect of trying to develop tests for the StatusCheck class in an embedded container (using Arquillian). It appears that StatusCheck requires the mail resource "mail/topcat" to exist, even if mail.enable is false. If the resource does not exist, the container will fail to create the singleton instance of StatusCheck, and so the scheduled method to check and update download statuses will not run.

I think that this hasn't come to light before only because in all systems where the mail resource has not been created, the download types have been restricted to http/https and a single-tier IDS; in these cases, StatusCheck is not really used, so its absence is not noticed.

I am reopening this issue; but to be honest, I am not sure what to do. Strictly, I should remove the setup code that allows the creation of the mail resource to be optional, and insist that mail.* properties are always defined in topcat-setup.properties, even if mail is not used. However, this might break some of the current automated builds. Instead, I will probably just add a comment / warning to the installation notes.