-
Notifications
You must be signed in to change notification settings - Fork 57
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
Implement module "site" for configuring distributed monitoring #654
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we discuss the module name?
The UI calls it "Distributed monitoring", but the REST API "Site Management".
My issue here is, that the latter sounds a lot like what our server
role does.
We also follow the UI in other places IIRC, as the REST API made some interesting naming choices.
I am also happy, if we find a one-word name, or at least something shorter than distributed_monitoring
or site_management
.
Note to myself: After this is figured out, update README.
I also disliked the module name tbo, so I changed it to distmon, which is short and recognizable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting the overall feeling, that it might make sense, to abstract the module more from the REST API. The structure of the module could probably be flatter than that of the API. That is not to say, that this is not a good first version, we just might want to keep it in mind for a later review.
…it conflicts a bit with the 'server' role.
Currently, the integration tests are not correct. I have to refine them. But from my point of view, the module itself is ready for some first manual tests. |
Looks like the integration tests runs into this problem: actions/runner-images#6680 |
I'll investigate, but I doubt, it is a timeout issue, we have/had tests, which run longer. It might be resource exhaustion. |
It was indeed resource exhaustion. I reduced the number of remote sites to be created. Now github is happy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with the changes. Pending a code review, we are good to merge.
….general into feature/module-distributed
I tested the following without issues against 2.2.0p33cee:
Will discuss with my colleague as soon as he is back from vacation but I would say its working good. |
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
There was no module for managing distributed monitoring sites.
What is the new behavior?
Now there is.
Other information
Thanks to the usage of IDs in the site management REST API, it was possible to make this module idempotent. If you run into problems, please let us know.