Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cron-workflow): fix active and conditions nullity #25

Closed
wants to merge 1 commit into from

Conversation

mobin102
Copy link
Contributor

@mobin102 mobin102 commented Dec 6, 2021

Cron workflow example can not be executed properly due to ValueError: Invalid value for active, must not be None. I have added V1LocalObjectReference to active and conditions empty list of V1alpha1CronWorkflowStatus.

@flaviuvadan
Copy link
Collaborator

flaviuvadan commented Dec 8, 2021

Thank you so much for contributing again @mobin102 🚀 Is the ValueError thrown right by the cron_workflow_service.create(...) or the initialization phase of the CronWorkflow object? Asking because, internally at @dynotx, we have faced something similar with cron workflows. The submit implementation we have is:

def submit(self, namespace: str = 'default') -> None:
    try:
        self.service.submit(self.workflow, namespace)
    except ValueError as e:
        # the deserialization protocol of the V1alpha1CronWorkflow object creates a V1alpha1CronWorkflowStatus
        # that does not set the `active`, which cannot be `None`. Deserialization happens after this receives
        # the HTTP response back and it raises a ValueError. This captures that specific error message and simply
        # returns as it's safe for now. Otherwise, it raises the same exception to propagate it to the client
        if str(e) == 'Invalid value for `active`, must not be `None`':
            return
        else:
            raise e

So, I am not sure whether the active field of the workflow initialization object is required when it's created by the __init__. It might actually be required when it's created by the service create method! A similar issue was reported here. The OpenAPI SDK generators does not add the right objects to the cron workflow create response, in this case. Thoughts?

@flaviuvadan
Copy link
Collaborator

flaviuvadan commented Dec 8, 2021

@mobin102 I have a PR in Argo Workflows to (hack) fix the autogenerated SDK: argoproj/argo-workflows#7363 I think that will help solve the issue without us modifying Hera. We're going to be dependent on migrating to argo-workflows==6.0.0, which is #7, but that should not be that big of a change 🙂

@flaviuvadan
Copy link
Collaborator

@mobin102 check out this line: https://github.com/argoproj-labs/hera-workflows/pull/38/files#diff-40b2cae22ec120b51dde27b367365d8ae1aa08f425cfd186da5c1712b31e5cefR60 That's the one that makes the SDK not check the return type validity (it is a valid response, it just doesn't have all the expected fields but that's fine).

@flaviuvadan
Copy link
Collaborator

@mobin102 I just merged #38! I am going to close this for now. Thank you for looking into this issue in the first place! I hope the new releases fixes the problems you noticed 🙂 New issues are always welcome!

@flaviuvadan flaviuvadan closed this Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants