-
Notifications
You must be signed in to change notification settings - Fork 23
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
ref: Migrate profile_info
to use common info module
#558
ref: Migrate profile_info
to use common info module
#558
Conversation
attribute_names = [v.name for v in self.attributes] | ||
attribute_names = ( | ||
[[v.name for v in self.attributes]] | ||
if len(self.attributes) > 0 | ||
else None | ||
) | ||
|
||
super().__init__( | ||
module_arg_spec=self.module_arg_spec, | ||
required_one_of=[attribute_names], | ||
mutually_exclusive=[attribute_names], | ||
required_one_of=attribute_names, | ||
mutually_exclusive=attribute_names, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation brought an edge case that when the module didn't require any attribute, we still send an [[]]
value as required_one_of
. It caused a bug to fail the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration and manual tests working properly. Nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM works locally.
📝 Description
Migrate
profile_info
to use common info module. Also make some necessary change to the base info module because profile_info doesn't require any attribute or parameter to call.✔️ How to Test
make TEST_ARGS="-v profile_info" test
Manual test: