Code review comment for ~mertkirpici/charm-hw-health:lp/1928403

> Hi Facundo thanks for the review and thoughts.
>
> > Thoughts?
> This is a very good catch! Thanks for bringing this up.
> As rare as this scenario might be I think if we could avoid it, we should. I
> am thinking around two solutions:
>
> 1 - Introducing a `force=False` parameter in the Ipmi::install_cronjob()
> signature . That will be checked alongside the date_filter variable that
> you're pointing out. Only the action code will set it to True when calling,
> i.e. Ipmi().install_cronjob(..., force=True). I believe this should be enough
> to fix this however it smells like bad design to me.
>
> 2 - Delegating the max record id getting code to the action code and storing
> _that_ in the unit data rather than the date. This will render everything
> static(nothing getting figured out during charm runtime after the action call
> completes). This is somewhat a bigger change(unit data key, purpose changing,
> etc.) however I think it is the proper way to implement this change.
>
> I am inclined to push an update towards option 2. Is there anything I am
> missing here?

I'd really prefer telling you to go for option 1. as it's simpler, and I wouldn't invest time in implementing the "right" option for a charm like this one which has a death sentence in the wake of the COS.

*But*, I believe option 1 doesn't cut, as it breaks in other ways. If a config-changed is triggered, then add_exclude_display_range_to_cron_args() won't be executed (if I understood correctly) and hence the filter won't be rendered to the cron file. This effectively unacks all SEL entries.

Option 2. is, like you say, the way to go. We only care about the record ID up to which we're ignoring, and the action code is the only one which should mess around with it.