Remove invalid config_install_interval - #8769
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
| self.assertIn("ERROR: Incorrect definition of general.config_install_interval: {}. " | ||
| "Removing it from conan.conf to avoid possible loop error.".format(internal), | ||
| self.client.out) | ||
| self.client.run('install .') |
There was a problem hiding this comment.
Once removed, no error should occur, as we have default conanfile.txt to be installed.
| elif unit == 'h': | ||
| return datetime.timedelta(hours=float(value)) | ||
| else: | ||
| elif unit == 'd': |
There was a problem hiding this comment.
It may be a breaking change. Default was days, now we no longer accept implicit values. I don't think it's a good idea keeping implicit support, we would need read docs to understand what's the behavior.
There was a problem hiding this comment.
In the docs:
The config_install_interval variable starts a time scheduler which runs conan config install according the time interval configured. It only accepts the follow time intervals: minutes, hours and days.
It is lacking some details. It should also have been labeled as experimental, this could be a problem, I'd probably keep the default with a big warning.
memsharded
left a comment
There was a problem hiding this comment.
Looking good, just some concern about breaking, it is indeed likely that this will break, so probably keep old default "days".
| elif unit == 'h': | ||
| return datetime.timedelta(hours=float(value)) | ||
| else: | ||
| elif unit == 'd': |
There was a problem hiding this comment.
In the docs:
The config_install_interval variable starts a time scheduler which runs conan config install according the time interval configured. It only accepts the follow time intervals: minutes, hours and days.
It is lacking some details. It should also have been labeled as experimental, this could be a problem, I'd probably keep the default with a big warning.
Checking again, it was an impossible condition. At line 34 has a regex which only accepts values with units. I just reverted |
| elif unit == 'd': | ||
| return datetime.timedelta(days=float(value)) | ||
| elif unit == 'w': | ||
| return datetime.timedelta(weeks=float(value)) |
There was a problem hiding this comment.
there is no else branch in order unit isn't any of s/m/h/d/w
There was a problem hiding this comment.
Not really necessary if the previous regex guarantees unit is always defined. But yes, probably the dictionary is a bit neater.
You are right, good! |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Hello there.
I've added new units for
secondsandweeks. Now when loadingconan.conf, ifconfig_install_intervalis invalid, it will be removed.I did another change that may break users, but I think necessary. Before this PR, if
config_install_intervalvalue had no value (e.g. config_install_interval=1), it would be considered days, implicitly. Now, an unit is mandatory (explicit is better than implicit).I need to update docs, because those units are not clear there, but first I would like some review.
fixes #8713
Changelog: Fix:
config_install_intervalno longer enter in loop when invalid.Docs: conan-io/docs#2067
developbranch, documenting this one.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.