Fix global config get for port scan. #2794
Conversation
It was not actually testing cylc scan.
(Was inconsequential, however).
|
Note I took out the default settings specially. I think it is important to respect the empty setting, e.g. in the dumper and in logical decision in some cases - where the empty list should not be treated the same way as the default list. (I also suspect that I've done something about default evaluation in #2609.) |
|
(Or maybe not.) |
|
Note we have the "--sparse" option to return explicitly set (i.e. non-default) items. I agree that some default settings should be empty or null. But in this case, if we default to running suite daemons on localhost, is there any reason not to default to scanning localhost? If not, then having to explicitly (and correctly) default to "localhost" at every use of the config item just seems like more chances to make errors, no? |
|
(Not sure why we were both doing work on a Sunday!) The other problem is that we still need to handle the case when users put an empty value to the setting (so overriding the default). This still needs to be handled by the run time logic, so we might as well move the default to where the run time logic is. |
|
(But sorry to have broken the logic!) |
|
No worries. OK, I guess you are saying we should default to "localhost" no matter what - even if the user explicit unsets the hosts list ... and that requires run time defaulting (because a built-in default gets overridden by the user setting). I guess that makes sense, reverting to run time default... |
|
(mind you, shouldn't that same argument apply to the list of ports, which you still set a built-in default for?) |
|
(you win 👍 ... although I think we're not being consistent on this defaulting business.. Maybe #2609 helps, I'm almost on to that review again...) |
|
Yes, the port setting has the same issue - although it is less likely that users will override it with empty settings. (Another problem I have just realised - on dumping out the configuration - integer ranges get expanded to all values, so we should find a way to tuck them back into a |
|
@hjoliver can you tag me as soon as this is at a stage when there is agreement on an approach & you want me to review the implementation (possibly that is now)? It's not always obvious from the context of the comments if this is the case. Then I will review it as a priority. Thanks :) |
Yeah, that's not nice. |
|
@sadielbartholomew - you can go ahead and review this if Matt is happy with the defaulting behaviour now, which I think he is. It's actually a trivial change, it'll take you about 5 mins max 😁 |
|
(Running test in my local environment.) |
|
(Tests pass in my environment.) |
|
@sadielbartholomew please review 2. |
|
Sorry for the delay. I have just run the full test battery locally with |
Fix #2795 (bug on current master, since last release).
glbl_cfg().get()isn'tdict.get().(maybe was not the best name for that function).
Also removes a
cylc scantest that was not actually testing anything anymore.