azure - schema - period modes cron regex

[jomalsan]

Adds regex expressions to the schemas for azure-periodic and container-periodic that check the validity of the cron expression. Note: The two regexs are different because the interpreter used by the two modes for the cron expressions are different. Included comment linking to the interpreters

Closes #4552

[stefangordon]

100 char line max, will fail lint.

[jomalsan]

Yes that is what I am working on addressing now. One of the unit tests for function periodic was misformatted and was failing as well

Changes are coming

[jomalsan]

@stefangordon This should be fixed now

[stefangordon]

You might look at some of the validate tests we have for most resources, they are very simple and just verify schema.

e.g.

    def test_aks_schema_validate(self):
        with self.sign_out_patch():
            p = self.load_policy({
                'name': 'test-azure-aks',
                'resource': 'azure.aks'
            }, validate=True)
            self.assertTrue(p)

Not a bad idea to toss some of those in to make sure the regex seems to be working right.

You can also test the negative by loading the policy inside a with like this:

        with self.assertRaises(ValidationError):
            self.load_policy(data=p, validate=True)

[logachev]

I think we can simplify it by using regex for months, days of the week from container.
Yes, functions allow you to use whatever, but we can keep some consistency here.

[stefangordon]

nit: You don't have to do the +=, they will be auto-concatenated.

https://docs.python.org/3/reference/lexical_analysis.html#string-literal-concatenation

regex = 'line1'
        'line2'
        'line3' 

is valid

[jomalsan]

@stefangordon that method of string concatenation was not working. It does not throw a compile error, but the resulting regex matched with all of the invalid cases. To replicate the style I put everything in a list and then join the list when I need the regex. This passes all of the unit tests for it

[jomalsan]

I think we can simplify it by using regex for months, days of the week from container.
Yes, functions allow you to use whatever, but we can keep some consistency here.

@logachev The current version is working for the functionality of the function timer cron expression. Are you thinking that we should simplify it for maintainability?

[logachev]

Yes.. c7n is not primary functions provisioning tool, so there is no need to support everything

[stefangordon]

While I agree with the sentiment, I think in this case we are already committed to exposing the azure trigger functionality here, I think we even link to it in the docs somewhere. It would be an unnecessary breaking change to block certain syntax.

[jomalsan]

We do link to the Azure Functions cron expression documentation in the Function Host Options doc. If we change the implementation here, then we need to change the documentation as well

[logachev]

good point, lets keep it as-is in that case

[kapilt]

a better link might be to apscheduler trigger parser code

[kapilt]

impressive regex, reminds me of the jwz quote https://web.archive.org/web/20140424160443/http://regex.info:80/blog/2006-09-15/247

[stefangordon]

Azure triggers don't allow * * * * * * ?

[jomalsan]

Azure triggers don't allow * * * * * * ?

I don't see anywhere where they specifically say it is not allowed, but none of their examples have * in the first slot (seconds). I figured blocking a function (that takes 2-3 seconds to run) from running every second seemed ok. Also if you want the every second functionality "0-59 * * * * *" does the same thing

[stefangordon]

Azure triggers don't allow * * * * * * ?

I don't see anywhere where they specifically say it is not allowed, but none of their examples have * in the first slot (seconds). I figured blocking a function (that takes 2-3 seconds to run) from running every second seemed ok. Also if you want the every second functionality "0-59 * * * * *" does the same thing

Agree it would almost certainly crash Functions so fine with blocking it.

[erwelch]

I think this is great! My only thought is that perhaps if we actually do the check in the validator method instead of the schema we might be able to give the user a better error message. If it didn't fit in the schema I can say personally with 100% confidence I would not be able to tell from that regex that I had an extra * or something haha.

[stefangordon]

I think this is great! My only thought is that perhaps if we actually do the check in the validator method instead of the schema we might be able to give the user a better error message. If it didn't fit in the schema I can say personally with 100% confidence I would not be able to tell from that regex that I had an extra * or something haha.

Yeah I had the same thought this error is going to be nasty, but I guess its 10000x better than getting no validation error and having functions just broken. I was actually seeing there might be mechanisms in jsonschema to customize the validation error now, so this might be the best of both worlds. (might require adding code to the validation error handler to print out the custom message)

Lets not block the PR on it as its a net-improvement, but we can research that.