Crontab rewrite

[taranlu-houzz]

I made this PR in order to deal with some of the various issues that I have run into when using the crontab operation (recently #533).

Currently, this patch resolves all issues that I have run into personally (I managed to track down the cause of #533 to be an issue with the way sed_replace works at least in my specific edge case). I feel like sed is pretty tricky to work with when it comes to catching all of the possible edge cases...

@Fizzadar I was hoping that I could get some feedback to see if this is something that you would consider integrating into pyinfra.

Advantages:

• Crontab fact is more robust, and the entirety of the crontab is gathered, in order, so that any part could be modified (I feel like it could be nice to add a normalize_whitespace param that would turn all whitespace chunks into a single blank line, for example).
• Crontab operation add/remove/update logic is a bit more straightforward and clear in my opinion.
• sed issues with edge cases are avoided.

Remaining issues:

• Need to format code to match pyinfra style spec (single quotes mainly).
• Need to get all tests passing (the fact tests were simple to update, but I am a little confused by the generated operation tests).
• I'm not entirely clear on how interporlate_variables is supposed to work. I assumed it means that you could have something like $USER in the command argument, and it would resolve that before actually writing to the crontab, but that did not seem to work when I tested on current.
• Need to add appropriate error for invalid arguments (e.g. special_time param). Not sure how that is handled elsewhere in pyinfra.

[Fizzadar]

This is a huge improvement - thank you so much for taking the time to rewrite this!

The only change required is the fact class name to avoid breaking existing implementations, once that's swapped out (and tests/linting pass) I think this is good to go 👍

[Fizzadar]

This is a much cleaner approach - I always regretted the command -> details dict choice here!

One problem here is that this is a breaking change - could we make this a new class (maybe just Crons? Or CronEntries?) and leave the old one as-is, I can then add a deprecation notice when used pending removal in v2.

[taranlu-houzz]

Ah, yeah, I can make that change. Will get to that when I have some more time to work on this PR.

[taranlu-houzz]

@Fizzadar Ah, good to hear! I may not be able to get to this for a little while, but I do plan to continue on this when I have time (will be out on leave for about a month).

In the meantime, these are a couple of questions that I have:

• Could you help me to understand how your operation tests are generated? I found the json that generates the tests to be a little confusing.
• How do operations typically handle errors for invalid parameters? I have a check in place for the special_time param, but I wasn't quite sure what the standard process is elsewhere in pyinfra.
• I was also hoping to get some clarification on how the interpolate_variables param is supposed to function. It didn't seem to do what I was expecting...

I have a few more questions, but I'll get to those when I get back to this PR to tidy it up and get it ready for merging.

[Fizzadar]

Hey @taranlu-houzz! No worries at all hugely appreciate you taking the time to work on this; answers below:

* Could you help me to understand how your operation tests are generated? I found the json that generates the tests to be a little confusing.

The JSON files should roughly define, for a given operation, the inputs (args/kwargs) and the commands output as a result. There's also ways to define facts/filesystem objects/etc which have been tacked on over time. I need to do a proper write up of the format of these because I'm sure this will help many contributors - it's certainly quite confusing at the moment (#656).

* How do operations typically handle errors for invalid parameters? I have a check in place for the `special_time` param, but I wasn't quite sure what the standard process is elsewhere in `pyinfra`.

There's an OperationError class that works nicely for this; example @ https://github.com/Fizzadar/pyinfra/blob/27f4732ae712f3ae158fbb00c98e5319e3f9eefd/pyinfra/operations/mysql.py#L271-L275.

* I was also hoping to get some clarification on how the `interpolate_variables` param is supposed to function. It didn't seem to do what I was expecting...

The idea here is that it will double-quote any user provided strings in the output shell - that should then enable environment variable substitution in those strings.

I have a few more questions, but I'll get to those when I get back to this PR to tidy it up and get it ready for merging.

👍

[taranlu-houzz]

@Fizzadar I'm back from leave and will hopefully be able to get this wrapped up soon. I do have a further question about the interpolate_variables param: Could I get a couple of example cases showing the difference this setting makes?

In some basic test using the below deploy, it doesn't seem to make any difference (I think I am probably still just misunderstanding what it does):

#!/usr/bin/env python
# -*- coding: utf-8 -*-


"""Test crontab interpolate_variables param."""


####################################################################################################
# Imports
####################################################################################################


# ==Site-Packages==
from pyinfra.operations import server


####################################################################################################
# Main
####################################################################################################


server.crontab(
    name=f"Are my variables interpolated yet?",
    command='echo "$USER" "toot" "oop"',
    special_time="@reboot",
    interpolate_variables=True,
)

I did run several variations with --debug set, so I can see that it can change the sed command that gets run, but the resulting crontab doesn't seem to be different.

[Fizzadar]

@taranlu-houzz the variables will only render if not quoted at all, for example:

from pyinfra.operations import files


files.line(
    path='somefile',
    line='this is $USER data',
    interpolate_variables=True,
)

files.line(
    path='somefile',
    line='this is $USER data',
)

Results in a file:

this is nick data
this is $USER data

This has uncovered an unrelated bug (fixed by adaf051).