Recipe AddCronTrigger creates empty expression

[moderne-meeseeks[bot]]

Sample configuration :

type: specs.openrewrite.org/v1beta/recipe
name: org.yeikel.MyCustomRecipe
displayName: My custom recipe
description: Fix all the things.
recipeList:
  - org.openrewrite.github.AddCronTrigger:
      cron: 5 4 * * *

The input to cron does not seem to matter as the other expressions also create the same problem

  @Test
  fun netflixruntimeHealth() = assertUnchanged(
    moderneAstLink = "https://api.public.moderne.io/worker/ast/file?path=Netflix%2Fruntime-health&origin=github.com&branch=master&changeset=89a897ad7958551f439fe019d5436665e3a510a0&filePath="
  )

---

This issue originally created by @yeikel on: https://github.com/moderneinc/support-public/issues/26

[moderne-meeseeks[bot]]

---

This comment originially written by @ on:

[timtebeek]

I'm wondering if it'd need quotes around the cron expression.. 🤔

[yeikel]

I'm wondering if it'd need quotes around the cron expression.. 🤔

I tried adding quotes but the UI added a single quote returning the same output

type: specs.openrewrite.org/v1beta/recipe
name: org.undefined.MyCustomRecipe
displayName: My custom recipe
description: Fix all the things.
recipeList:
  - org.openrewrite.github.AddCronTrigger:
      cron: '"@daily"'

[yeikel]

I tried importing it using the import YML feature, and while it read it correctly, it still produced the same output

[timtebeek]

Likely fixed in https://github.com/openrewrite/rewrite-github-actions/commit/45db5286d147d72e9552b1815f4c40055ace6119 ; still needs to roll out to public.moderne.io to be sure, but I checked a couple other instances and did not see @Option applied to any final fields before; I suspect that interferes, but does not throw an error. Perhaps it should? 🤔

[knutwannheden]

Candidate for ArchUnit rule?

[timtebeek]

I like the suggestion, but then still we'd need to run those Arch tests for every rewrite module. Perhaps it's easier to catch where we interpret those options, but not finding that yet.

[knutwannheden]

Both would be good to have, as we would still like to catch the error before it happens.

[yeikel]

I don't quite understand why this is the case when unit tests passed

Can we test this discrepancy in any way? From my perspective, it seems to be a unique requirement of the deployed service and the reason I set it to final was because it made sense at the context :/

[timtebeek]

I gave it a quick attempt to test & verify the fix with a unit tests as also seen here, but gave up when that didn't immediately ran on this project with a weird error. Bugged me for a bit, but you can only work on so many things in a day. :)