-
Notifications
You must be signed in to change notification settings - Fork 21
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
[ClockSync] add support for linux root users #319
base: master
Are you sure you want to change the base?
Conversation
octobot_commons/os_clock_sync.py
Outdated
@@ -39,11 +39,13 @@ def _get_sync_cmd(self): | |||
platform = os_util.get_os() | |||
bot_type = os_util.get_octobot_type() | |||
if bot_type == commons_enums.OctoBotTypes.DOCKER.value: | |||
raise NotImplementedError(bot_type) | |||
return "service ntp stop && ntpd -gq && service ntp start" |
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.
There is no ntp service in the current octobot docker image
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 added it to the dockerfile as well see PR in octobot -> [Docker] add ntp to docker version
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 don't think we should add ntp service in docker based on this thread: https://stackoverflow.com/questions/24551592/how-to-make-sure-dockers-time-syncs-with-that-of-the-host
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.
Oh yeah crap, I removed the docker part, now only the linux root fix is included
4830814
to
0a32c83
Compare
0a32c83
to
82d5fab
Compare
82d5fab
to
730ff92
Compare
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.
@GuillaumeDSM I let your merge it when it's ok for you
edit: removed the docker changes as its not working