Remove nailgun entity_mixin import from discovery tests #15164
Conversation
Gauravtalreja1
added
Easy Fix :)
Gauravtalreja1
force-pushed
the
remove-nailgun-import-discovery
branch
2 times, most recently
from
b0d42d0 to
3ded594
Compare
robottelo/config/__init__.py
Outdated
| @@ -90,6 +90,19 @@ def get_url(): | |||
| return urlunsplit((scheme, hostname, '', '', '')) | |||
|
|
|||
|
|
|||
| def default_nailgun_config(): | |||
There was a problem hiding this comment.
Can you please explain the rationale for adding a new function for this instead of passing the admin credentials to user_nailgun_config?
There was a problem hiding this comment.
yes, its indeed possible, but I just wanted to keep it separate from existing user_nailgun_config, as similar to what we had in nailgun. entity_mixin and its using get_credentials func to get creds so we don't need to pass and import settings just for ssh username and passwd in the tests
There was a problem hiding this comment.
I'm not totally convinced that adding a function with very similar functionality to an existing function is preferable to importing settings in a test module, especially since #9936 suggests that user_nailgun_config can be used with both admin and non-admin users. I won't block the PR based on that, though.
However, I do think the name of the function and the docstring should make it clear why the new function exists. I would suggest changing the name of the function to admin_server_config, since it is designed specifically to return a ServerConfig object with the admin credentials. In the docstring, I would remove the reference to :param user: since the function has no parameters, and I would also make it clear in the docstring that the config file will be constructed from the admin credentials.
What do you think about these suggestions?
There was a problem hiding this comment.
@synkd Good point, renamed the func and updated the docstrings
Gauravtalreja1
force-pushed
the
remove-nailgun-import-discovery
branch
from
3ded594 to
884512d
Compare
Signed-off-by: Gaurav Talreja <[email protected]>
Gauravtalreja1
force-pushed
the
remove-nailgun-import-discovery
branch
from
884512d to
73471c0
Compare
Remove nailgun entity_mixin import from discovery test Signed-off-by: Gaurav Talreja <[email protected]> (cherry picked from commit e48b00c)
Remove nailgun entity_mixin import from discovery test Signed-off-by: Gaurav Talreja <[email protected]> (cherry picked from commit e48b00c)
Remove nailgun entity_mixin import from discovery test Signed-off-by: Gaurav Talreja <[email protected]> (cherry picked from commit e48b00c)
…15164) Remove nailgun entity_mixin import from discovery test Signed-off-by: Gaurav Talreja <[email protected]>
Problem Statement
Remove nailgun entity_mixin import from discovery tests, and replace entity_mixins.DEFAULT_SERVER_CONFIG
Solution
Replacing entity_mixins.DEFAULT_SERVER_CONFIG with new default_nailgun_config method, which is same as existing user_nailgun_config method
And, we don't have any automated tests under this module, verifying default_nailgun_config() method in #15163