CronWorkflow suspend: incorrect manifest

[DanCardin]

Pre-bug-report checklist

1. This bug can be reproduced using pure Argo YAML

• Yes 👉 Please report a bug to the Argo Workflows GitHub 👈
• No

If yes, it is more likely to be an Argo bug unrelated to Hera. Please double check before submitting an issue to Hera.

2. This bug occurs in Hera when...

• exporting to YAML
• submitting to Argo
• running on Argo with the Hera runner
• other:

Bug report

Describe the bug

CronWorkflow(
    name="foo",
    schedule="* * * * *",
    workflow_template_ref=models.WorkflowTemplateRef(name="bar"),
    suspend=True,
)

Generates:

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  name: foo
spec:
  schedule: 0 */6 * * *
  workflowSpec:
    suspend: true
    workflowTemplateRef:
      name: bar

This does not prevent the workflow from being scheduled, as expected. Instead, it regularly schedules the workflows, but the workflow itself is suspended and stays running forever.

Expected behavior
As far as i can tell from the docs (and from clicking the suspend checkbox inside Workflows), it should generate

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  name: foo
spec:
  schedule: 0 */6 * * *
  suspend: true
  workflowSpec:
    workflowTemplateRef:
      name: bar

Environment

• Hera Version: 5.15.1
• Python Version: 3.10
• Argo Version: 3.5.1

---

As far as I can tell, this stems from https://github.com/argoproj-labs/hera/blob/main/src/hera/workflows/cron_workflow.py#L153 not special casing the suspend field, like it does the schedule field.

It seems like it could be fixed by something alike:

        model_workflow = cast(_ModelWorkflow, super().build())
+       model_workflow.spec.suspend = None
        model_cron_workflow = _ModelCronWorkflow(
            metadata=model_workflow.metadata,
            spec=CronWorkflowSpec(
                schedule=self.schedule,
+               suspend=self.suspend,
                workflow_spec=model_workflow.spec,
            ),
        )

to exactly produce the above yaml. With the caveat, that if you did want the suspend inside workflowSpec, this makes that impossible, for whatever that's worth...

Im happy to supply a PR if this makes sense?

[DanCardin]

for what it's worth, I ran into the relative complexity caused by CronWorkflow subclassing Workflow in #942.

imo, while subclassing Workflow for the DRYness of its fields is convenient, the complexity added downstream ends up being more confusing due to fields like this.

It's perhaps beyond the scope of this specific bug report, but it might be better to construct a mixin that lets them both share the methods that are mutually applicable, while redefining the actual attributes on CronWorkflow and properly assigning the spec path, so that the build method doesn't need to pretend it's a workflow and individually handle the colliding attributes.

[DanCardin]

🥹 there's a whole separate cron_suspend?! gah. Well that probably solves this bug-wise...

I still probably/maybe think something about this issue indicates something maybe ought to change...