search

cloudbase / cloudbase-init

Cross-platform instance initialization
http://openstack.org
Apache License 2.0
421 stars 149 forks source link

SetUserSSHPublicKeysPlugin should get the admin username from the service instead CONF.username #22

Closed rgl closed 4 years ago

rgl commented 4 years ago

Shouldn't the following code (or even all usages of CONF.username):

https://github.com/cloudbase/cloudbase-init/blob/f9c15d221404b8f9842d247e37cfa14a070d9ee8/cloudbaseinit/plugins/common/sshpublickeys.py#L37

Be changed to:

service.get_admin_username() or CONF.username

Or:

shared_data.get(constants.SHARED_DATA_USERNAME,
                                    CONF.username)

Or normalize all usages to the last snippet (which is used by ConfigWinRMCertificateAuthPlugin)?

ader1990 commented 4 years ago

@rgl , I agree. shpublickeys and other plugins should use this logic: username = service.get_admin_username() or CONF.username.

ConfigWinRMCertificateAuthPlugin is a little special as it requires the user password to enable the certificate auth, but the plugin should still use the initial logic to get the username.

ader1990 commented 4 years ago

@rgl can you reply with a +1 on this patch: https://review.opendev.org/#/c/696603 ? Thank you.

rgl commented 4 years ago

@ader1990, @ociuhandu, sorry, I forgot to +1. But I agree with it.