New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#7297 fix config install when scheduled #7311
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
conans/client/command.py
Outdated
@@ -2045,7 +2045,7 @@ def run(self, *args): | |||
raise ConanException("Unknown command %s" % str(exc)) | |||
|
|||
if is_config_install_scheduled(self._conan) and \ | |||
(command != "config" or (command == "config" and args[0] != "install")): | |||
(command != "config" or (command == "config" and args[0][1] != "install")): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args now is a tuple with a list ([]) that's why config install triggered the scheduler ...
self.assertIn("Repo cloned!", self.client.out) | ||
shutil.rmtree(self.folder) | ||
with patch("conans.client.command.is_config_install_scheduled", return_value=True): | ||
self.client.run("config --help", assert_error=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please test that you can actually conan config install --remove
the offending config, and then after that next scheduled call will succeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good test case
Signed-off-by: Uilian Ries <uilianries@gmail.com>
now = datetime.now(gettz()) | ||
return now > sched | ||
else: | ||
api.out.warn("Skipping scheduled config install, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that if it is empty, it would keep warning this all the time, at every invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reformulated the idea. It will warn the user, but only when scheduled, not for every new command.
now = datetime.now(gettz()) | ||
return now > sched | ||
if _load_configs(config_install_file): | ||
timestamp = os.path.getmtime(config_install_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should happen always, what if there is one broken one not removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scheduler only checks if the task is scheduled and if a config is there, doesn't matter if it's broken or not. The method configuration_install
(at same file) is the judge, it will consume the configs file.
I believe the scheduler should be stupid, only run a task if available and the interval passed. Otherwise, it will need to "steal" the configuration_install
action too.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
conans/client/command.py
Outdated
@@ -2044,8 +2044,8 @@ def run(self, *args): | |||
self._print_similar(command) | |||
raise ConanException("Unknown command %s" % str(exc)) | |||
|
|||
if is_config_install_scheduled(self._conan) and \ | |||
(command != "config" or (command == "config" and args[0] != "install")): | |||
if (command != "config" or (command == "config" and args[0][1] != "install")) and \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it complete ignores the scheduler if running config install
@@ -258,10 +262,25 @@ def configuration_install(app, uri, verify_ssl, config_type=None, | |||
_save_configs(configs_file, configs) | |||
|
|||
|
|||
def _is_scheduled_intervals(file, interval): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved to here for better simplification, and be able to mock in functional tests.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@memsharded I reformulated this PR 30 minutes ago, I think it looks better now, including the logic. |
There was a bug which considered
config install
as command which was able to trigger a new config command when the scheduler was ready.This PR won't prevent the error when the remote git has been removed and it's listed as config remote, the scheduler will continue to fail. However, you will be able to run
conan config install -r 0
, or anyconan config install xxx
without problem now.Changelog: Fix: Conan config install does not trigger scheduled config command.
Docs: Omit
Fixes: #7297
/cc @michaelmaguire @memsharded
develop
branch, 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.