Telegraf Charm

Code review comment for ~sajoupa/charm-telegraf:expose_ip_ranges

  1. Git
  2. lp:~sajoupa/charm-telegraf
  3. expose_ip_ranges
  4. Merge into master
Revision history for this message
Paul Goins (vultaire) wrote :

Thanks for the MR.

This seems like a fairly large MR if the intent is purely to address bug #1890102. I wonder if the code can be reduced here? I haven't read the whole MR yet, but I've read 1/3rd to 1/2, and it seems like maybe it's addressing something else as well? Could you explain?

Additionally, as axino noted on the above bug, this can be addressed, albeit more verbose, via the extra_options config value. Is this insufficient? And if it is, could we focus the change on adding the config and updating the template to consume that config value?

To be clear, I'm not trying to brush off these changes - but I am not understanding why all these changes are needed based upon the bug this is connected to. If there are other bugs to be fixed, it'd be better to fix those via additional MRs, in my opinion.

Best Regards,
Paul Goins

review: Needs Information

« Back to merge proposal