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

Add support for configuring service templates #117

Merged

Conversation

pdorschner
Copy link
Contributor

@pdorschner pdorschner commented Jan 2, 2023

Hi,

I created an Issue for this PR. Everything is explained there. More over, this PR will fail, because it needs a new release for the go-icinga2-client. For this, I created an PR also.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update the README.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Link this PR to related issues.

Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

This PR (and vshn/go-icinga2-client#10) don't adhere to the boundary between Signalilo and go-icinga2-client, and move a number of Signalilo-specific implementation details into the generic Icinga2 client library.

Please make sure that the library remains generic, and handle all the Signalilo-specific configurations in Signalilo itself.

webhook/icinga.go Outdated Show resolved Hide resolved
webhook/icinga.go Outdated Show resolved Hide resolved
webhook/icinga.go Show resolved Hide resolved
gc/gc.go Outdated Show resolved Hide resolved
@dragoangel
Copy link
Contributor

dragoangel commented Feb 2, 2023

I now have issues exactly with this default generic-service template in Icinga2Web with Director and really need a function to use any other service template, but not this one, would be cool to see it working. Maybe it can be used - but I don't get clue how to do it. Icinga API don't see generic-service till I not create it via Director, and at same time it will result in duplicated service template when Satelites try reuse it. @simu you not faced such cases?

Update:
For now I get temporary workaround, changed one hardcode to another one.

  1. I cloned vshn/go-icinga2-client repo to the root of Signalilo repo on my PC
  2. Added to Signalilo Dockerfile: COPY go-icinga2-client /src/go-icinga2-client after WORKDIR command
  3. Added to the end of go.mode: replace "github.com/vshn/go-icinga2-client" v0.0.16 => "/src/go-icinga2-client"
  4. Replaced in go-icinga2-client/icinga2/service.go text from generic-service to go-icinga2-service
  5. Build on image and pushed it to my registry
  6. Created needed service template via Director

I tried to change the code to get this PR in nicer condition locally, but ATM my go knowledge is not so good to get these things done 😓

P.s. also funky stuff going on with heartbeat as Director can't create functions, only strings 🙈...

@pdorschner
Copy link
Contributor Author

I've reworked the go-icinga2-client PR, so it will be generic.

@pdorschner
Copy link
Contributor Author

Hi,
is there any chance that this PR will be merged?

Best Regards

Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

Implementation looks ok now. Please make sure the PR doesn't break the static service variables feature, and rebase it on the latest master branch.

webhook/icinga.go Outdated Show resolved Hide resolved
@pdorschner
Copy link
Contributor Author

All issues has been fixed.
The problem with the go.mod can be fixed after merging this PR go-icinga2-client

@pdorschner
Copy link
Contributor Author

Is there a chance this PR getting merged? Can I support you with something?

Best Regards

@simu
Copy link
Member

simu commented Jun 21, 2023

Hi, sorry for the late reply.

This looks good now, I've verified the changes locally and everything works now (side-note: I didn't test the configurable service templates part).

I've now merged and tagged the go-icinga2-client changes as v0.0.17. Unfortunately, since I don't have write access to your fork, I can't fix the conflicts on this PR myself. Can you please rebase the branch on the latest master branch and make sure that the go.sum file is updated to reference github.com/vshn/go-icinga2-client instead of github.com/pdorschner/go-icinga2-client.

Edit: nevermind, I just didn't have enough coffee yet. I'll merge and release this when the tests pass.

@simu simu force-pushed the feature/configurable_icinga2_service_templates branch from 315b975 to 72c90e9 Compare June 21, 2023 06:46
@simu simu merged commit 8f98363 into vshn:master Jun 21, 2023
@simu simu added the enhancement New feature or request label Jun 21, 2023
@simu simu changed the title Implement configurable service templates Add support for configuring service templates Jun 21, 2023
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.

3 participants