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

Introduce a centralized library to call the Checkmk API #291

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

godspeed-you
Copy link
Contributor

The code to contact the Checkmk API is redundantly placed all over the modules. This projects aims to centralize this code into a single library to make our lifes more easy. Later changes may be done in one single place instead of changing several dozens of code lines.

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, if the API headers or other static information is changed, this needs to be adapted in an unclear amount of files and code lines.
  • Everytime a new API call to Checkmk is added, bugs or slightly different behaviour may be introduced.

What is the new behavior?

  • The centralized file may be called by every module to have a defined set of functionality returned information.
  • This set may be used to derive a module specific class to address specific requirements.
  • The result of this API call is stored in a defined and clearly structured data set. This allows to automatically provide a clear and reliable set of information after the API call.

Other information

This PR contains a migrated module as an example on how to use this new library. The changes have been reduced to an absolute minimum to minimize potential bugs. So potential further refactorings may be skipped to have this change less complex.

@godspeed-you godspeed-you changed the title Extend rule module integration tests Introduce a centralized library to call the Checkmk API Mar 26, 2023
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.

I really like the way you unified and simplified the API calls!
I have a question regarding the definition of the inheriting class, though. In some modules, there are multiple, different POST or GET calls. How would you define the related
def post(self)
calls for those?

@msekania
Copy link
Contributor

related #176

@godspeed-you
Copy link
Contributor Author

godspeed-you commented Mar 27, 2023

I prepared a second module to use the API library (user) and I had minor problems/changes to the API.py - but I guess, you're referring more to the group modules, where you can post a single or multiple groups. For the users module, it felt very natural, to just use get/post/put/delete. If there is more than one put-call, I would probably use a suffix (e.g. def put_group and def put_groups.

On users (in the pipeline) I worked with the following:

def get(self):
   ... 

[...] 

def module():
    ... 
    user = UserAPI()
    result = user.get()

@msekania
Copy link
Contributor

@godspeed-you,

First of all thanks,

Concerning module, in essence it should at least cover all already implemented usage in different modules, otherwise it will be yet another modules which one has to adjust manually per module and not the central one.

@godspeed-you
Copy link
Contributor Author

Would it be okay to add other modules one after another? It's a lot of work and I would prefer to take my time to avoid to introduce new Bugs.

In the other Side it's very time consuming keeping a feature branch in sync with the main/devel for a longer period.

@lgetwan
Copy link
Contributor

lgetwan commented Mar 28, 2023

I agree to update the modules one by one. And I agree, it's probably easier to have separate feature branches for each module change.
Thanks for updating the user module, as well!

@robin-checkmk robin-checkmk added the enhancement New feature or request label Mar 28, 2023
@msekania
Copy link
Contributor

@godspeed-you,

when structure is fixed and accepted, I will also help with group modules.

plugins/modules/activation.py Outdated Show resolved Hide resolved
The behavior of the module.exit_json should be predictable across the project.
This is now part of the first implementation of a centralized api.

Also the api is now able to exit with module.fail_json. This shortens the code
to run until the exit is executed eventually.
@robin-checkmk robin-checkmk mentioned this pull request Mar 29, 2023
7 tasks
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.

Looks fine!

@robin-checkmk robin-checkmk merged commit 643c405 into devel Mar 29, 2023
@robin-checkmk robin-checkmk deleted the gdspd_you/feature/api-module branch March 29, 2023 11:37
robin-checkmk added a commit that referenced this pull request May 22, 2023
Introduce a centralized library to call the Checkmk API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants