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

Extend the functionality of the folder module #539

Merged

Conversation

msekania
Copy link
Contributor

Extends the functionality of the folder module.
See also #533 .

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #533

What is the new behavior?

  • One can supply a) a list o strings or b) dict as a romote_attributes parameter.
  • a) reproduces the functionality of the REST API, and was already implemented
  • b) removes the specified attribute if the given folder had such an attribute.

Other information

@msekania msekania requested a review from lgetwan as a code owner January 27, 2024 20:38
@github-actions github-actions bot added the module:folder This affects the folder module label Jan 27, 2024
Copy link
Contributor

@lgetwan lgetwan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for improving the functionality of this module! It looks like you invested quite a bit of Hirnschmalz. :-)
Unfortunately, the diff shown here is not the diff between feature/migrate_folder_module and feature/extend_folder_module_functionality, but agains the devel branch. So I did my own diff on the commandl ine. ;-)
I added some comments, but nothing severe.

plugins/modules/folder.py Show resolved Hide resolved
plugins/modules/folder.py Show resolved Hide resolved
plugins/modules/folder.py Outdated Show resolved Hide resolved
plugins/modules/folder.py Show resolved Hide resolved
plugins/modules/folder.py Show resolved Hide resolved
@msekania
Copy link
Contributor Author

msekania commented Feb 9, 2024

Thanks a lot for improving the functionality of this module! It looks like you invested quite a bit of Hirnschmalz. :-) Unfortunately, the diff shown here is not the diff between feature/migrate_folder_module and feature/extend_folder_module_functionality, but agains the devel branch. So I did my own diff on the commandl ine. ;-) I added some comments, but nothing severe.

@lgetwan
I do not know what went wrong here, but header says

msekania wants to merge 10 commits into Checkmk:feature/migrate_folder_module from msekania:feature/extend_folder_module_functionality

and Resolve conflicts shows also something different than branch changes 🤷‍♂️

@robin-checkmk robin-checkmk changed the base branch from feature/migrate_folder_module to devel February 9, 2024 13:20
@robin-checkmk robin-checkmk added enhancement New feature or request release:4.3.0 Affects the mentioned release. labels Feb 9, 2024
@lgetwan
Copy link
Contributor

lgetwan commented Feb 9, 2024

For some reason, the integration test that checks idempotency, fails in the 2nd step of these two:

- name: "{{ outer_item.version }} - {{ outer_item.edition | upper }} - Update folder attributes without specifying title (name)."
  folder:
    server_url: "{{ checkmk_var_server_url }}"
    site: "{{ outer_item.site }}"
    automation_user: "{{ checkmk_var_automation_user }}"
    automation_secret: "{{ checkmk_var_automation_secret }}"
    path: "{{ checkmk_folder_attr_test.path }}"
    state: "present"
    update_attributes:
      tag_criticality: "test"
      tag_networking: "wan"
  delegate_to: localhost
  run_once: true  # noqa run-once[task]

- name: "{{ outer_item.version }} - {{ outer_item.edition | upper }} - Check the idempotency of Update attributes."
  folder:
    server_url: "{{ checkmk_var_server_url }}"
    site: "{{ outer_item.site }}"
    automation_user: "{{ checkmk_var_automation_user }}"
    automation_secret: "{{ checkmk_var_automation_secret }}"
    path: "{{ checkmk_folder_attr_test.path }}"
    update_attributes:
      tag_criticality: "test"
    state: "present"
  delegate_to: localhost
  run_once: true  # noqa run-once[task]
  register: test_output
  failed_when: test_output.changed | bool

The forst task returns:

"msg": "200 - Folder modified. Changed: update attributes: {\"tag_criticality\": \"test\", \"tag_networking\": \"wan\"}"

The second one returns:

 "msg": "200 - Folder modified. Changed: attributes"

@lgetwan lgetwan self-requested a review February 9, 2024 15:20
@msekania
Copy link
Contributor Author

msekania commented Feb 9, 2024

might be related to the fact that attribute is {}.

I have to check it.

@msekania
Copy link
Contributor Author

msekania commented Feb 9, 2024

For some reason, the integration test that checks idempotency, fails in the 2nd step of these two:

I found the issues, line 307,
In essence I have introduced it when I "simplified" if statement.
Sorry

@lgetwan
Copy link
Contributor

lgetwan commented Feb 13, 2024

Thanks for fixing this, Michael!
Then, the PR is ready to be merged. :-)

@robin-checkmk robin-checkmk mentioned this pull request Feb 13, 2024
7 tasks
@robin-checkmk robin-checkmk merged commit b646c9c into Checkmk:devel Feb 13, 2024
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2024
@msekania msekania deleted the feature/extend_folder_module_functionality branch February 14, 2024 15:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request module:folder This affects the folder module release:4.3.0 Affects the mentioned release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Inconsistent behaviour of Ansible modules (update vs remove + compared to API)
3 participants