Skip to content
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

Remove nailgun entity_mixin import from discovery tests #15164

Merged

Conversation

Gauravtalreja1
Copy link
Collaborator

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

@Gauravtalreja1 Gauravtalreja1 added Easy Fix :) Easiest Fix to review and quick merge request. CherryPick PR needs CherryPick to previous branches AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 labels May 23, 2024
@Gauravtalreja1 Gauravtalreja1 self-assigned this May 23, 2024
@Gauravtalreja1 Gauravtalreja1 requested review from a team as code owners May 23, 2024 17:53
@Gauravtalreja1 Gauravtalreja1 force-pushed the remove-nailgun-import-discovery branch 2 times, most recently from b0d42d0 to 3ded594 Compare May 23, 2024 18:13
@@ -90,6 +90,19 @@ def get_url():
return urlunsplit((scheme, hostname, '', '', ''))


def default_nailgun_config():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain the rationale for adding a new function for this instead of passing the admin credentials to user_nailgun_config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@synkd Good point, renamed the func and updated the docstrings

@Gauravtalreja1 Gauravtalreja1 force-pushed the remove-nailgun-import-discovery branch from 3ded594 to 884512d Compare May 30, 2024 16:29
Signed-off-by: Gaurav Talreja <gtalreja@redhat.com>
@Gauravtalreja1 Gauravtalreja1 force-pushed the remove-nailgun-import-discovery branch from 884512d to 73471c0 Compare May 30, 2024 16:31
@synkd synkd merged commit e48b00c into SatelliteQE:master May 30, 2024
8 checks passed
github-actions bot pushed a commit that referenced this pull request May 30, 2024
Remove nailgun entity_mixin import from discovery test

Signed-off-by: Gaurav Talreja <gtalreja@redhat.com>
(cherry picked from commit e48b00c)
github-actions bot pushed a commit that referenced this pull request May 30, 2024
Remove nailgun entity_mixin import from discovery test

Signed-off-by: Gaurav Talreja <gtalreja@redhat.com>
(cherry picked from commit e48b00c)
github-actions bot pushed a commit that referenced this pull request May 30, 2024
Remove nailgun entity_mixin import from discovery test

Signed-off-by: Gaurav Talreja <gtalreja@redhat.com>
(cherry picked from commit e48b00c)
@Gauravtalreja1 Gauravtalreja1 deleted the remove-nailgun-import-discovery branch May 30, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches Easy Fix :) Easiest Fix to review and quick merge request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants