-
Notifications
You must be signed in to change notification settings - Fork 151
Add support for Context Profiles and Service Groups #535
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
base: master
Are you sure you want to change the base?
Conversation
Add support to create/remove Context Profiles and Service Groups Signed-off-by: William Stoneman <[email protected]>
Updated get_custom_attributes_from_display_name function for profiles with no attributes
|
|
||
| EXAMPLES = ''' | ||
| - name: Create a new Security Service | ||
| nsxt_security_services: |
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.
It seems like you used another module as a template. There are quite a few places that need to be updated to reflect the unique attrs of this module. For example, this should show nsxt_context_profiles.
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.
Options and example updated.
|
|
||
|
|
||
| def main(): | ||
| argument_spec = vmware_argument_spec() |
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.
Since this is an NSX Policy resource, you can use the template for nsxt_policy*.py modules. It removes a lot of boilerplate. Refer https://github.com/vmware/ansible-for-nsxt/blob/master/plugins/modules/nsxt_policy_ip_block.py or https://github.com/vmware/ansible-for-nsxt/blob/master/plugins/modules/nsxt_policy_segment.py
If you don't want to go that way, that's fine too
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.
Based on timeframes for my project, we will utilize this approach. I will look into changing it for a future release.
| if module.check_mode: | ||
| module.exit_json(changed=True, debug_out=str(json.dumps(context_params)), id='12345') | ||
|
|
||
| #print("?????????????????????????") |
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.
remove
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.
Left over testing. Removed.
| manager_url = 'https://{}/policy/api/v1'.format(mgr_hostname) | ||
|
|
||
| # err_msg = 'NSX v9.0.0 and above do not support MP resources in nsxt_ip_blocks.py. Please use nsxt_policy_ip_block.py module.' | ||
| # validate_nsx_mp_support(module, manager_url, mgr_username, mgr_password, validate_certs, err_msg) |
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.
we can remove this, ruight?
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.
Removed.
| context_params['display_name']) | ||
| if existing_services is None: | ||
| return False | ||
| if existing_services['attributes'] != context_params['attributes']: |
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.
are you certain this op is fine? what if list size is not the same or it's a nested dict?
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.
This has been tested. If the list of attributes differ it will "update" the profile.
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.
same comment as in other module: the user may want to update other attrs
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.
I took another look at it and updated the code to support display_name and description support. However, to allow this to work the end user needs to supply the ID. Otherwise there is no way to reference the object if they decide to update the display_name.
| } | ||
|
|
||
| try: | ||
| id = context_params['display_name'].replace(" ", "_") |
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.
afaik, id and display_name can be different. Other modules support that.
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.
It can be difference, but if the display name follows proper naming standards (no spaces and use of "_") then the ID will be the same. This approach just simplifies the deployment and usage of the objects.
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.
I think you don't have to specify the ID if display_name is present. If that's true, let server handle the behavior. Otherwise, you might end up creating a different behavior than the expectation
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.
even worse, what if the user updates display_name? You'll end up using wrong ID and it could fail?
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.
When creating the object you need to specify the ID in the URI. Having them the same simplifies the automation when calling the object in a different task.
If the end user updates the display_name later then it wont update the ID, this is definitely an issue with all nsxt objects.
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.
I took another look at it and updated the code to support display_name and description support. However, to allow this to work the end user needs to supply the ID. Otherwise there is no way to reference the object if they decide to update the display_name.
Updated COntext Profile based on comments made on PR
|
|
||
| EXAMPLES = ''' | ||
| - name: Create a new Context Profile | ||
| nsxt_security_services: |
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.
nsxt_context_profile
| } | ||
|
|
||
| try: | ||
| id = context_params['display_name'].replace(" ", "_") |
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.
I think you don't have to specify the ID if display_name is present. If that's true, let server handle the behavior. Otherwise, you might end up creating a different behavior than the expectation
| def get_services_from_display_name(module, manager_url, mgr_username, mgr_password, validate_certs, display_name): | ||
| services = get_services(module, manager_url, mgr_username, mgr_password, validate_certs) | ||
| for service in services['results']: | ||
| if service.__contains__('display_name') and service['display_name'] == display_name: |
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.
consider
if 'display_name' in service and ...
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.
I took another look at it and updated the code to support display_name and description support. However, to allow this to work the end user needs to supply the ID. Otherwise there is no way to reference the object if they decide to update the display_name.
| service_params['display_name']) | ||
| if existing_services is None: | ||
| return False | ||
| if existing_services['service_entries'] != service_params['service_entries']: |
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.
what if the user wants to update anything else like description or display name?
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.
I took another look at it and updated the code to support display_name and description support. However, to allow this to work the end user needs to supply the ID. Otherwise there is no way to reference the object if they decide to update the display_name.
| context_params['display_name']) | ||
| if existing_services is None: | ||
| return False | ||
| if existing_services['attributes'] != context_params['attributes']: |
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.
same comment as in other module: the user may want to update other attrs
| } | ||
|
|
||
| try: | ||
| id = context_params['display_name'].replace(" ", "_") |
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.
even worse, what if the user updates display_name? You'll end up using wrong ID and it could fail?
Fixed update support to allow for display name and description update
|
I took another look at it and updated the code to support display_name and description support. However, to allow this to work the end user needs to supply the ID. Otherwise there is no way to reference the object if they decide to update the display_name. |
Add support to create/remove Context Profiles and Service Groups
Signed-off-by: William Stoneman [email protected]