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

allow time formatting constants in rego time.format and time.parse_ns #6005

Merged

Conversation

tjons
Copy link
Contributor

@tjons tjons commented Jun 12, 2023

Why the changes in this PR are needed?

Resolves #5945

What are the changes in this PR?

Add support for Go standard library format constants when calling time.format. If the third parameter is passed to time.format and is a valid constant name for a timestamp format in the Go standard library, it will be used to format the timestamp. Otherwise, the string will be treated as a format string and may error.

Signed-off-by: Tyler Schade <tyler.schade@solo.io>
@netlify
Copy link

netlify bot commented Jun 12, 2023

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 7b2f7cd
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/6488635e111bb000089f7f45
😎 Deploy Preview https://deploy-preview-6005--openpolicyagent.netlify.app/docs/edge/policy-reference
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

anderseknert
anderseknert previously approved these changes Jun 12, 2023
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Nice! Thanks. Skimming the docs on the date/time built-ins just now and it strikes me that we probably want this capability for time.parse_ns too, or what do you think?

Doesn't have to be included in this PR, but I suppose it's simple enough to do.

@ashutosh-narkar
Copy link
Member

@tjons thanks for the contribution. Can you run make build locally and check-in the generated builtin_metadata.json file. This should resolve this failed check.

@tjons
Copy link
Contributor Author

tjons commented Jun 12, 2023

@ashutosh-narkar yep, I can do that right now.

@anderseknert - I agree, was thinking of that when I did the PR. Since it's not part of the original issue, do you want me to create a new issue for it or just handle it in this one? Happy to contribute that as well.

Signed-off-by: Tyler Schade <tyler.schade@solo.io>
@anderseknert
Copy link
Member

It's small enough that adding it here seems fine. I'll update the issue to reflect that 👍

Signed-off-by: Tyler Schade <tyler.schade@solo.io>
@anderseknert anderseknert changed the title allow time formatting constants in rego time.format allow time formatting constants in rego time.format and time.parse_ns Jun 13, 2023
@anderseknert
Copy link
Member

anderseknert commented Jun 13, 2023

Ah, just noticed there's a section on timestamp parsing here: https://www.openpolicyagent.org/docs/latest/policy-reference/#timestamp-parsing

Perhaps we could add something about this there? Either a list of the constants available, or a link to somewhere else where they are explained.

Sorry for the scope creep @tjons 😅

Signed-off-by: Tyler Schade <tyler.schade@solo.io>
@tjons
Copy link
Contributor Author

tjons commented Jun 13, 2023

All good @anderseknert. Done - let me know what you think.

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

It's putting a bit more burden on non-golang-based implementations (wasm, IR-based things), but that's OK, I think. Thanks for contributing 👏

@anderseknert anderseknert merged commit 4f5882d into open-policy-agent:main Jun 13, 2023
27 checks passed
@tjons
Copy link
Contributor Author

tjons commented Jun 13, 2023

@srenatus can you expound on that to help me understand? From skimming the WASM docs it looks like the time.format and time.parse_ns built-ins are not supported - what burden is it adding here and how can we correct?

@srenatus
Copy link
Contributor

Any builtins that don't have a native wasm implementation (read: another implementation written in in C) will be unavailable to Wasm users, unless

  1. they use OPA -- which fills all non-natively-implemented builtins with its own ones
  2. their SDK provides the builtin

Now, for (2.), and SDKs that are not based on Golang, the builtin implementation will have to figure out what golang does, and replicate it. The only way to avoid that would be to write a native implementation; but then that work would go into writing that.

@tjons
Copy link
Contributor Author

tjons commented Jun 13, 2023

@srenatus makes sense, thank you. hard to avoid here since we are replicating the golang feature but good to note.

@srenatus
Copy link
Contributor

@tjons exactly. OTOH, it's not as bad as golang fmt.Sprintf showing through in Rego's sprintf 😄

superff pushed a commit to superff/opa that referenced this pull request Jun 18, 2023
…open-policy-agent#6005)

Allow time formatting constants in `time.format` and `time.parse_ns`

Signed-off-by: Tyler Schade <tyler.schade@solo.io>
Signed-off-by: Fred Feng <superffeng@gmail.com>
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.

Allow for predefined "constants" in time formatting built-ins
4 participants