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

Add support of the time module in Starlark Processor #9004

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Add support of the time module in Starlark Processor #9004

merged 2 commits into from
Mar 23, 2021

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented Mar 17, 2021

Required for all PRs:

  • Associated README.md updated.
  • Has appropriate unit tests.

fixes #9000

Motivation

The upstream project starlark-go has added a new module called time to support dates and durations, thus the goal of this feature request is to add the support of this new module in the Starlark Processor.

Modifications

  • Upgrade the version of go.starlark.net
  • Import the module time of go.starlark.net in the Starlark Processor
  • Add a test showing how to parse a date and a duration and how to create a timestamp in seconds
  • Refer the tests into the README.md

Result

In Starlark script, we can now import the module with load('time.star', 'time'), and then have access to all the corresponding building functions and constants.

NB: Beware the built-in function from_timestamp only supports timestamps in seconds.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 17, 2021
Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

@srebhan
Copy link
Member

srebhan commented Mar 18, 2021

Hey @essobedo, thanks for all your effort regarding starlark! You are my hero!
Regarding this PR I think it tries to achieve the same as #7742 and also suffers from the same problems regarding the update of protobuf. How should we proceed? Should I close #7742 in favor of this PR?

@essobedo
Copy link
Contributor Author

@srebhan Hi, if you don't mind closing #7742 in favor of this PR, it would be great

@essobedo
Copy link
Contributor Author

@srebhan about the build failure on make tidy, I don't really get it, it doesn't fail on my side. Any idea how to fix it?

@srebhan
Copy link
Member

srebhan commented Mar 18, 2021

Ok closed. Please note, that your build errors/dependency hell is resolved with #8937 being merged (tested locally).

@sspaink
Copy link
Contributor

sspaink commented Mar 18, 2021

@essobedo are you using the Go version 1.16+ locally when running `make tidy'? If not could you update Go to the latest version and run it again and push the changes.

@essobedo
Copy link
Contributor Author

@essobedo are you using the Go version 1.16+ locally when running `make tidy'? If not could you update Go to the latest version and run it again and push the changes.

@sspaink No, I'm using 1.15.8, let me upgrade it and try again, thank you for your help

@essobedo
Copy link
Contributor Author

@sspaink it seems to work now, many thanks

@sspaink
Copy link
Contributor

sspaink commented Mar 18, 2021

The failing of the job 'share-artifacts' is unrelated to this PR, will be fixed soon but will require a re-base to get the new config. Sorry about that.
Ok should be resolved now, will require a re-base to get a check mark but it isn't required. sorry for the confusion.

Great work on the PR!

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

@ssoroka ssoroka merged commit f267f34 into influxdata:master Mar 23, 2021
jblesener pushed a commit to jblesener/telegraf that referenced this pull request Apr 18, 2021
@sjwang90
Copy link
Contributor

@essobedo Do you think you could add time.star and math.star information to the Starlark readme libraries section?

@essobedo essobedo deleted the 9000/add-time-module branch April 21, 2021 07:18
@essobedo
Copy link
Contributor Author

@sjwang90 yes of course, I will propose a PR asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support of the time module in Starlark Processor
5 participants