Skip to content

Conversation

@inqrphl
Copy link
Contributor

@inqrphl inqrphl commented Nov 18, 2025

Also changed the name for RequestHandler, previously it was only using the binding address. Append that to the module name instead of directly replacing. The binding address is still accessible as a variable.

The only reason it would be like this is you want to parse the name back to the binding address somewhere else, but I did not see anywhere this happened. But I did not search it thoroughly.

- if the module is of type RequestHandler, the name was replaced with the binding address. Append that binding string instead of replacing.
@inqrphl
Copy link
Contributor Author

inqrphl commented Dec 4, 2025

This was cancelled right @sni ? No need to quit the program if one module cant start

I would like to still add the change in pkg/snclient/snclient.go though, helps a lot when debugging:

https://github.com/ConSol-Monitoring/snclient/pull/288/files#diff-55f2db129bd3e8af4e4515190fa5bd3fcc156b1f1ee2d955ae5adf72520d8eecL594

Copy link
Contributor

@sni sni left a comment

Choose a reason for hiding this comment

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

Besides the failing test case. I am not totally sure how to handle this. There are 2 situations:

  • initial start: failed modules should prevent startup
  • reload: reloading the client with a changed configuration should probably not stop the running program in case of errors.

if listener, ok := mod.(RequestHandler); ok {
name = listener.BindString()
log.Debugf("bind: %s", name)
// If it can be converted into a Requesthandler, the name changes. Why? Is it later used to parse the port or hostname?
Copy link
Contributor

Choose a reason for hiding this comment

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

please ask question in the PR or directly, not in the code.

@inqrphl inqrphl closed this Dec 18, 2025
@sni sni reopened this Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants