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

Fix module folder for future checkmk releases #404

Closed

Conversation

msekania
Copy link
Contributor

@msekania msekania commented Aug 7, 2023

Fixes issue #389

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?

Currently all three options attributes, remove_attributes, and update_attributes can be used simultaneously, which is no longer supported by API for Check MK >= v2.2.0p7

Issue Number: #389

What is the new behavior?

  • issue a warning message if more then one option from attributes, remove_attributes, and update_attributes is used simultaneously for Check MK < v2.2.0p7
  • fail with error message if more then one option from attributes, remove_attributes, and update_attributes is used simultaneously for Check MK >= v2.2.0p7

Other information

@github-actions github-actions bot added role:server This affects the server role module:folder This affects the folder module labels Aug 7, 2023
@robin-checkmk
Copy link
Member

@msekania thanks for tackling this!
Is your branch up-to-date with our devel? Looks like there are some unrelated changes in here.

@robin-checkmk robin-checkmk self-assigned this Aug 7, 2023
@msekania
Copy link
Contributor Author

msekania commented Aug 7, 2023

Is your branch up-to-date with our devel? Looks like there are some unrelated changes in here.

apparently not, changes should be only in folder.py

@robin-checkmk
Copy link
Member

I'll take a look at the failing tests and the weirdness with the additional changes.

@msekania
Copy link
Contributor Author

msekania commented Aug 7, 2023

@robin-checkmk

thanks!

@robin-checkmk
Copy link
Member

robin-checkmk commented Aug 7, 2023

I understand the issue with the integration tests now but the solution is not trivial.
I pushed a hotfix, but that is not final yet.
@msekania the current results indicate, that there is an actual issue with the module. Can you take a look?

@msekania
Copy link
Contributor Author

msekania commented Aug 8, 2023

So the current test findings should be valid, and if you pull the devel branch into yours, the integration tests should be fixed properly.

@robin-checkmk, not sure whether it got better, even more files are now altered

@robin-checkmk
Copy link
Member

@msekania did you pull the devel branch, or did you rebase or something different? It looks indeed weird, but we can figure this out for sure. If you can check the integration test findings, we should get through this.

@msekania
Copy link
Contributor Author

msekania commented Aug 8, 2023

@robin-checkmk,

I pulled devel branch, and merged it in the current, which required rebase.

@msekania
Copy link
Contributor Author

msekania commented Aug 8, 2023

@robin-checkmk,

alternative solution, when tests go through successfully, I can create a new pull request branching out from the actual devel branch and moving only folder.py and close this PR.

just let me know what do you think.

@robin-checkmk
Copy link
Member

robin-checkmk commented Aug 8, 2023

@msekania I am uncertain what exactly happened, but I think your idea of creating a new and clean PR is the best approach. Go ahead and create it, I will merge tomorrow then and release.
Thanks!

P.S.: The failing molecule test is irrelevant.

@msekania msekania closed this Aug 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2023
@msekania msekania deleted the fix-folder-for-future-checkmk-releases branch August 8, 2023 18:36
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 role:server This affects the server role
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants