Skip to content
New issue

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

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

Already on GitHub? # to your account

Differing date formats in Executions and Webhooks #661

Closed
davidgengenbach opened this issue Dec 13, 2019 · 3 comments
Closed

Differing date formats in Executions and Webhooks #661

davidgengenbach opened this issue Dec 13, 2019 · 3 comments
Labels

Comments

@davidgengenbach
Copy link

Describe the bug
The date format differs between in

To Reproduce

  1. Configure a webhook (with template)
  2. Create a job
  3. Wait for the execution of the job

Expected behavior
The execution timestamps should have the same format as the webhook notifications.

Specifications:

  • OS: Linux david 5.3.14-200.fc30.x86_64 #1 SMP Mon Dec 2 15:57:50 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
  • Version Dkron 2.0.0

Additional context

From the golang time documentation:

The returned string is meant for debugging; for a stable serialized representation, use t.MarshalText, t.MarshalBinary, or t.Format with an explicit format string.

The date format in the job executions are the "right" ones - I would think.

I would propose using the t.MarshalText as suggested by the golang people!

These are the places where the fields are defined/marshalled:
Webhooks: https://github.com/distribworks/dkron/blob/master/dkron/notifier.go#L86
Executions: https://github.com/distribworks/dkron/blob/master/dkron/execution.go#L18

@yvanoers
Copy link
Collaborator

@davidgengenbach I suspect the format in the webhook template is how it is because it is intended to be human readable, whereas the API returns computer readable values. Maybe @Victorcoder can confirm this.

Nevertheless it is better to use t.MarshalText or the likes, as you mentioned. All that needs to be agreed on is the right format, and I'll whip up a PR.

@vcastellm
Copy link
Member

Agree, better to use MarshalText, thanks for the finding @davidgengenbach

@vcastellm
Copy link
Member

Fixed in #665

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants