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

Rewrite folder module and migrate to new collection API. #536

Closed
wants to merge 8 commits into from

Conversation

lgetwan
Copy link
Contributor

@lgetwan lgetwan commented Jan 25, 2024

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?

The folder module was still a bit oldschool spaghetti code.

What is the new behavior?

The folder module now uses the new API

@github-actions github-actions bot added the module:folder This affects the folder module label Jan 25, 2024
plugins/modules/folder.py Outdated Show resolved Hide resolved
@lgetwan lgetwan requested a review from Max-checkmk January 25, 2024 14:37
@msekania
Copy link
Contributor

@lgetwan,

First of all, the restructured module looks great.

As for #533, I would say that it is more of a feature request.
I think I have implementation almost ready. If there is no other implementation or plans (like keeping it tight to REST API behavior), I can make PR with changes to this PR or wait until this PR is accepted and make my PR after that.

@robin-checkmk
Copy link
Member

@msekania I think it makes sense for you to open your PR based on this branch. That way, we can either merge your changes into this branch, or we merge this PR, which will make your PR automatically move to the devel branch. Either way will it make things easier, when we can see the actual code.

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

@robin-checkmk,

I made a PR, see #539 , but there are some merge issues.
Could you please have a look.
Thanks

@robin-checkmk
Copy link
Member

@msekania thanks for the PR! I think @lgetwan has to take a look at the conflict. Are you sure, you based your changes on the top of this branch?

@msekania
Copy link
Contributor

msekania commented Jan 27, 2024

@robin-checkmk, yes, let's wait for @lgetwan.

I sync and pulled this branch before branching out to mine.
Conflicts also look "interesting". Should not be an issue.

@lgetwan
Copy link
Contributor Author

lgetwan commented Jan 29, 2024

Thanks for creating this pull request, @msekania!
This week, I'm quite busy, also in the first half of next week. I will try to resolve the merge conflicts until end of next week.

@lgetwan
Copy link
Contributor Author

lgetwan commented Feb 14, 2024

I will close this PR, as its complete contents were already merged into #539, where they have been improved further.

@lgetwan lgetwan closed this Feb 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2024
@robin-checkmk robin-checkmk deleted the feature/migrate_folder_module branch February 14, 2024 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

3 participants