-
Notifications
You must be signed in to change notification settings - Fork 288
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
Clean up handling of connection file and info #863
Conversation
It turned out that trying to push |
f78dd2d
to
3a173de
Compare
ping @kevin-bates for another review |
Hi @blink1073 - thanks for looking into this. Given the caveats you ran into regarding Is this PR serving as a small step forward to where we ultimately want to be? I suppose since this would be considered an API change, and we're nearing 8.0, this is the time to do this. One thing we might be able to do to avoid breaking existing (non-LocalProvisioner) provisioners is to rework the
There's still risk introduced by item 2, but the hope would be either those provisioners didn't implement What are your thoughts on the PR and the above? If you'd still like to move things forward, I'm okay with that, but I think, at a minimum, we need item 3 since "other" provisioners need to convey their connection info and they'd need to add the equivalent of item 2 somewhere. Frankly, if 8.0 is the impetus here, I kinda like letting |
@kevin-bates and I had a chat about this today. We agreed that it should be up to the provisioner whether it even needs to write a connection file, so we should keep the "forcing" logic on the manager to write the file for other consumers. However, we should also add a check in |
KernelProvisioners
are now explicitly responsible for the handling of the connection info and file, but the properties and methods of theConnectionFileMixin
continue to live on theKernelManager
since they are needed when a provisioner is not available.