search

slp / krun

krun - run programs from your system in a microVM
MIT License
25 stars 7 forks source link

Clean up some nits and fix some minor bugs #28

Closed teohhanhui closed 4 weeks ago

teohhanhui commented 4 weeks ago

Done. And removed unnecessary cloning. (lock_or_connect now takes and gives back ownership in the case of LockAcquired)

Hmm... Actually lock_or_connect does not adequately describe what the function actually does... Should it be launch_or_lock?

EDIT: Renamed

slp commented 4 weeks ago

Done. And removed unnecessary cloning. (lock_or_connect now takes and gives back ownership in the case of LockAcquired)

Hmm... Actually lock_or_connect does not adequately describe what the function actually does... Should it be launch_or_lock?

EDIT: Renamed

Could you please rename it to lock_or_launch? I think it reflects better that (except it KRUN_SERVER_PORT env is present) it'll first try to lock and only launch if it can't acquire the lock.

slp commented 4 weeks ago

LGTM. I also tested this and didn't find any issues. I've just suggested a very minor change, but otherwise I'm fine merging it.

teohhanhui commented 4 weeks ago

Could you please rename it to lock_or_launch? I think it reflects better that (except it KRUN_SERVER_PORT env is present) it'll first try to lock and only launch if it can't acquire the lock.

I get what you mean, but... This seems more natural to me: we first attempt to launch using an existing krun instance, and only continuing if the launch did not happen. That also fits with the usual pattern of the "right" (Err) case giving back ownership. Except we're not using Err here for anyhow reasons...