Code review comment for ~lihuiguo/charm-logrotated:bug/1864598

Revision history for this message
Eric Chen (eric-chen) wrote :

It's dangerous for the logic when we want to modify it in the future.
The hidden logic here is.. we only put the cronjob file with one frequency ["hourly", "daily", "weekly", "monthly"]. That is the design limitation in charm-logrotate, not a limitation in linux cronjob. But I don't have good idea to make this design limitation explict in the cleanup_cronjob function right now.

I have other ideas to remove some duplicated code and avoid additional parameter in the cleanup_cronjob.

1. There could be a cronjob filepath generator to be used in install_cronjob and cleanup_cronjob
2. We can use frequency index to generate the path and it's easy to know which one should be reserved in the cleanup_cronjob. We also can change the parameter as an array in the future if need.

/* Pseudo Code */
def cleanup_cronjob(self, reserve=-1):
    '''
    if reserve = -1, delete cronjob file in all frequencies
    else: only reserve the cronjob file in corresponding folder.
    '''

    for i in range(len(self.cronjob_check_paths)):
        if reserve == -1 or i != reserve:
            path = cronjob_path_generator(i)
            os.remove(path)

« Back to merge proposal