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

Feature azure rm adapplication info diff #1560

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

TheNotoriousRMM
Copy link

SUMMARY

azure_rm_adapplication_info add option "app_diff" #1559

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
COMPONENT NAME

azure_rm_adapplication_info.py
test targets > azure_rm_adapplication > tasks: main

ADDITIONAL INFORMATION
DOCUMENTATION = '''
options:
    app_diff:
        description:
            - The application Name or the app id.
        type: list
        elements: dict

Copy link
Collaborator

@Fred-sun Fred-sun left a comment

Choose a reason for hiding this comment

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

@TheNotorioursRMM Please take a look, Small change request! Thanks!


- name: Assert the difference
ansible.builtin.assert:
that: diff_output.app_diff[0].app_display_name == "{{ display_name }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong indentation: expected 6 but found 8

@@ -94,4 +104,4 @@
- name: Assert there is no application
ansible.builtin.assert:
that:
- output.applications | length == 0
- output.applications | length == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

No new line character at the end of file

@@ -149,7 +229,8 @@ def __init__(self):
app_id=dict(type='str'),
object_id=dict(type='str'),
identifier_uri=dict(type='str'),
app_display_name=dict(type='str')
app_display_name=dict(type='str'),
app_diff=dict(type='list', elements='dict')
Copy link
Collaborator

Choose a reason for hiding this comment

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

If defined this way, it will be easier to understand!

Suggested change
app_diff=dict(type='list', elements='dict')
app_diff=dict(type='list',
elements='dict',
options=dict(
app_id=dict(type='str'),
app_display_name=dict(type='str')
)
)

Copy link
Author

Choose a reason for hiding this comment

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

If I have the configuration of my adapps in a YAML or JSON file, I don't want to edit it additionally. I want to provide it directly to the plugin, not just the "app_display_name" or the "app_id". However, if I provide this as an option, I have to filter the YAML or JSON for these two options, which means another Ansible task in the playbook.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But your code only handles these two fields and no other fields?

if app.get("app_id") == diff.get("app_id") or app.get("app_display_name") == diff.get("app_display_name"):
found = True
break
if not found:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a definition error? Add to 'app_diff' if it should be True! Thank you!

Suggested change
if not found:
if found:

Copy link
Author

Choose a reason for hiding this comment

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

No, that is correct. It compares the provided list with the production Azure data, and if something is not present in the production environment, the status "absent" is added. This way, you can immediately reuse the return to delete the adapps that do not belong in the production environment using "plugins/modules/azure_rm_adapplication.py".
This allows you to first add adapps with "plugins/modules/azure_rm_adapplication.py" and then use "plugins/modules/azure_rm_adapplication_info.py" (option diff) to identify the adapps that should not be in the live environment and subsequently delete them with "plugins/modules/azure_rm_adapplication.py".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, please fix the following error, and make a detailed definition of the parameter definition. Thank you!

@Fred-sun Fred-sun added question Further information is requested medium_priority Medium priority work in In trying to solve, or in working with contributors labels May 16, 2024
@Fred-sun
Copy link
Collaborator

kindly ping!

….yml

Fix: No new line character at the end of file
@Fred-sun
Copy link
Collaborator

@TheNotoriousRMM Please help solve the conflicts I will review and push forward the merger as soon as possible. Thank you!

Copy link
Collaborator

@Fred-sun Fred-sun left a comment

Choose a reason for hiding this comment

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

small change request!

- name: get ad app diff ---- by display name
azure_rm_adapplication_info:
app_diff:
- app_display_name: "{{ display_name }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong indentation: expected 6 but found 8

azure_rm_adapplication_info:
app_diff:
- app_display_name: "{{ display_name }}"
- app_id: "{{ app_id }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong indentation: expected 6 but found 8

if app.get("app_id") == diff.get("app_id") or app.get("app_display_name") == diff.get("app_display_name"):
found = True
break
if not found:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, please fix the following error, and make a detailed definition of the parameter definition. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority question Further information is requested work in In trying to solve, or in working with contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants