-
-
Notifications
You must be signed in to change notification settings - Fork 42
Fix issues with echobot invite link in cmdeploy #741
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
base: main
Are you sure you want to change the base?
Conversation
hpk42
left a comment
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.
lgtm, although it'd probably be good to revisit all the places where the split local/remote execution takes place, and see if the if-branching can be avoided.
The entire stack is setup to support 'localhost' as a value for ssh_host, returning LocalExec when that is the case, but this last step made a new explicit SSHExec connection to config.mail_domain. This changes that to simply use the subprocess module when ssh_host is local. This also fixes the issue where the connection was made to 'config.mail_domain' instead of the supplied ssh_host value, ensuring that remains consistent. Additionally, the entire process will be skipped if --dry-run is used with cmdeploy, allowing a dry-run to complete without error.
|
I originally tried this but LocalExec and SSHExec don't seem to have the same semantics, with LocalExec only having a 'logged()' method, so I left it alone. I wouldn't mind adding the required methods to LocalExec so it could just be used in place of SSHExec and avoiding the crappy if/else branching, if that sounds ok |
| echobot_cmd = "cat /var/lib/echobot/invite-link.txt" | ||
| if ssh_host in ["localhost", "@local", "@docker"]: | ||
| result = ( | ||
| subprocess.check_output(echobot_cmd, shell=True) | ||
| .decode() | ||
| .strip() | ||
| ) | ||
| print(result) | ||
| else: | ||
| echo_sshexec = get_sshexec(ssh_host, verbose=args.verbose) | ||
| print( | ||
| echo_sshexec( | ||
| call=remote.rshell.shell, | ||
| kwargs=dict(command=echobot_cmd), | ||
| ) |
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.
| echobot_cmd = "cat /var/lib/echobot/invite-link.txt" | |
| if ssh_host in ["localhost", "@local", "@docker"]: | |
| result = ( | |
| subprocess.check_output(echobot_cmd, shell=True) | |
| .decode() | |
| .strip() | |
| ) | |
| print(result) | |
| else: | |
| echo_sshexec = get_sshexec(ssh_host, verbose=args.verbose) | |
| print( | |
| echo_sshexec( | |
| call=remote.rshell.shell, | |
| kwargs=dict(command=echobot_cmd), | |
| ) | |
| invite_path = "/var/lib/echobot/invite-link.txt" | |
| if ssh_host in ["localhost", "@local", "@docker"]: | |
| with open(invite_path) as f: | |
| print(f.readline()) | |
| else: | |
| echo_sshexec = get_sshexec(ssh_host, verbose=args.verbose) | |
| print( | |
| echo_sshexec( | |
| call=remote.rshell.shell, | |
| kwargs=dict(command=f"cat {invite_path}"), | |
| ) |
Thanks for the fix, just stumbled upon it ourselves. We don't even have to open a subprocess if we're on the same machine :) let's just do it in python.
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 is actually too fast now and requires a time.sleep (5 seconds works for me) before the file exists. (Somewhat related to #641).
| echobot_cmd = "cat /var/lib/echobot/invite-link.txt" | |
| if ssh_host in ["localhost", "@local", "@docker"]: | |
| result = ( | |
| subprocess.check_output(echobot_cmd, shell=True) | |
| .decode() | |
| .strip() | |
| ) | |
| print(result) | |
| else: | |
| echo_sshexec = get_sshexec(ssh_host, verbose=args.verbose) | |
| print( | |
| echo_sshexec( | |
| call=remote.rshell.shell, | |
| kwargs=dict(command=echobot_cmd), | |
| ) | |
| invite_path = Path("/var/lib/echobot/invite-link.txt") | |
| if ssh_host in ["localhost", "@local", "@docker"]: | |
| sleep(5) # wait a bit for echobot to settle down | |
| with invite_path.open() as f: | |
| print(f.readline()) | |
| else: | |
| sshexec = SSHExec(args.config.mail_domain, verbose=args.verbose) | |
| print( | |
| localexec( | |
| call=remote.rshell.shell, | |
| kwargs=dict(command=f"cat {invite_path}"), | |
| ) | |
| ) |
(still needs the from time import sleep at the top)
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.
sleep(5) is a questionable hack to avoid. Why is it that "cmdeploy run" finishes but echobot is not running? Is it possible to write a final "deployer" that waits e.g. for echobot to be up? Not sure if that's easily possible with pyinfra (or a systemctl command).
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.
maybe "systemctl is-active --wait echobot.service" or something similar could ensure that "cmdeploy run" returns only after echobot runs?
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.
systemctl is-active --wait echobot.service will only wait if state is initializing or starting, and we have either failed or active with active not necessarily meaning that configuration worked :/ I'll try something with Requires=: #746
Maybe we should move this question out of this PR.
The entire stack is setup to support
localhostas a value for ssh_host, returningLocalExecwhen that is the case, but this last step made a new explicitSSHExecconnection toconfig.mail_domain.This changes that to simply use the
subprocessmodule whenssh_hostis local.This also fixes the issue where the connection was made to
config.mail_domaininstead of the suppliedssh_hostvalue, ensuring that remains consistent.Additionally, the entire process will be skipped if
--dry-runis used with cmdeploy, allowing a dry-run to complete without error.Absolutely looking for feedback, as I mostly fixed this to get a deployment running in my environment, if more is needed, or a different approach is preferable, I'd be glad to rework this.