Add known hosts to manual provisioner #16662
Conversation
7 similar comments
|
Thanks @DanielKenichi for your contribution. Much appreciated and definitely looks like a good addition to Juju. May I ask if there is a specific version of Juju you would like to see this available in? At the moment this PR is currently targeting our master branch which will end up in Juju 4.0 release that won't happen for a little while yet. In the meantime I will start having a look at this PR and get you some feedback. Would you also be able to let us know how much more you would be willing to work on this PR if it needs a little bit of extra work to get it across the line? Cheers |
|
Heyo @tlm! Thanks for the feedback! We really appreciate it! There isn't a specific version of juju we would like to see it in, so I think there is no problem if it ends up on Juju 4.0. About working more on it, as long as there is no problem with me being more active on my freetime on weekends, I am willing to work as much as it needs since I think it is a good and fun experience to work with the codebase. My friends @migeyel @SaraO3O and @SnakeZ0 also had a part on this so I will be keeping them in touch with the feedbacks as well ok? |
There was a problem hiding this comment.
Thanks for the work thus far. I have left some comments for your considerations.
I am also thinking some of these ssh functions are becoming long in the tooth for ssh options getting passed to them and wondering if we should introduce an options ssh structure into the mix.
If this is something you don't feel comfortable with designing I am happy to put a commit on this PR with a design.
Cheers
tlm
DanielKenichi
force-pushed
the
add-known-hosts-to-manual-provisioner
branch
from
47bffd0
to
14684a1
Compare
|
/build |
DanielKenichi
force-pushed
the
add-known-hosts-to-manual-provisioner
branch
2 times, most recently
from
c5cc5b8
to
f51e26b
Compare
|
Hi @tlm! would you mind having a look at the changes done after your last review? |
hpidcock
added
decaying
|
/build |
|
@DanielKenichi you have test failures: |
Co-authored-by: Miguel Oliveira <[email protected]> Co-authored-by: Sara <[email protected]> Co-authored-by: PedroBaleeiro <[email protected]>
- Also fix missing arguments issue on init_test.go tests due to new KnownHostsFile parameter
This check ensures that the given file and directory has correct permissions for adding the machine fingerprint to it ssh creates the file if it does not exists and in case of the file not being accessible, it continues silently with the connection without adding any fingerprint to the file But I think that what we want here is to fail early if the knownhosts file is not writeable to inform the user that the given file will not have the machine fingerprint added
DanielKenichi
force-pushed
the
add-known-hosts-to-manual-provisioner
branch
from
f917191
to
756c5af
Compare
|
@hpidcock sorry for taking some time here, but finally fixed the merge conflicts |
Automatically provisioning machines via SSH is interesting for the implementation of models. Juju can accept manual provisioning on its machines. However, manual provisioning of a new machine asks for host confirmation by the user interactively. To make the task of automation easier, it is interesting to provide an option of defining the set of hosts in a specified file.
QA steps
To quickly verify this new option, first build the project with the applied changes. Then, bootstrap a localhost cloud with
juju bootstrap localhost test-controllerand launch a linux container withlxc launch ubuntu:22.04. Add a test model withjuju add-model fooand tryjuju add-machine --known-hosts <path-to-known-hosts-file> ssh:ubuntu@<lxd instance ip>.Verify that the known_hosts file used is not the default located on ~/.ssh, and that the specified file is used instead.
Checklist