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

Fixed Cron.ValidUntil using incorrect ISO8601 parsing format #187

Merged

Conversation

venera-program
Copy link
Contributor

What this PR does / why we need it:
The parsing method currently used for validating Cron.ValidUntil tries to convert the given value to ISO8601 Duration when it should be parsing for Datetime.

You can see this in the 0.8 spec's example definition found here: https://github.com/serverlessworkflow/specification/blob/0.8.x/specification.md#cron-definition

validUntil | Specific date and time (ISO 8601 format) when the cron expression is no longer valid

Special notes for reviewers:
If there is a preference for a different ISO8601 timestamp parser library over github.com/relvacode/iso8601 I am welcome to any suggestions.

Signed-off-by: Venera <31911811+venera-program@users.noreply.github.com>
@spolti
Copy link
Member

spolti commented Sep 12, 2023

Hi @venera-program Thanks for submitting this PR.
Isn't the time.Parse() enough?
It can handle RFC-3339 / ISO-8601 date-time as well.

@venera-program
Copy link
Contributor Author

venera-program commented Sep 12, 2023

@spolti How would we determine the correct layout to use in time.Parse(layout, value string)? Serverless spec does not define a specific format other than "ISO 8601 format" for this field. Additionally, there is no field for specifying a layout for ValidUntil; therefore we should probably aim to support as many layout variations as we can allow. Using time.Parse does not sufficiently fill that role.

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition! Many thanks! Sorry for my late review, I didn't notice it until yesterday. 🤦

If you need a patch release, let us know please.

@ricardozanini ricardozanini merged commit 5a23a17 into serverlessworkflow:main Sep 13, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants