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 underscore to js evaluation to allow for use of get() #83

Merged
merged 6 commits into from
Mar 12, 2024

Conversation

macebake
Copy link
Contributor

@macebake macebake commented Mar 8, 2024

This will help with nested fields that could potentially be null

@LHeggy
Copy link
Contributor

LHeggy commented Mar 11, 2024

On Performance

after pulling up vm creation to an init function it is way way faster in every sense:

1.03s user 1.03s system 188% cpu 1.094 total

(tested on 1.5k entries)

On shared state

I reckon overwriting the subject each operation will protect us from any serious issues and further effort is not warranted because:

  • Checked all 1.5k entries made and they match with entries in the database, so nothing is getting garbled by reusing the VM (at least in this simple use case)
  • Someone went down the rabbit hole of making this safe a few years ago and it looks like they gave up. Although it seems like it was an academic concern even in their use case
  • I can't imagine what a user would have to be doing with their jsonnet file to really start leaving nasty state in our otto VM. If that happens in the next few months I feel like we can tell them not to get clever with their jsonnet files and be a normal egg.

@macebake
Copy link
Contributor Author

Something isn't quite right here - when I test locally on the backstage samples, only catalog types defined without a filter are synced. Filters are checked in output/collect.go

@LHeggy
Copy link
Contributor

LHeggy commented Mar 11, 2024

Something isn't quite right here - when I test locally on the backstage samples, only catalog types defined without a filter are synced. Filters are checked in output/collect.go

tweaked the jsonnet file, should be working now

aliases: [
'$.metadata.name',
'_.get($.badKey, "name", "default alias")',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

did you mean to commit $badKey?

also, in these cases, we're sure that accessing nested field as normal still works? i assume so but wanted to check. We probably don't need all the examples of _.get() in the sample docs IMO (lmk what you think). Hopefully it's clear from expressions.md that you can use both methods

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that was just testing out aliases. I've replaced it with something less confusing

@macebake
Copy link
Contributor Author

This looks good and all works for me! You can merge and cut a minor release. Also it's probably worth following up w Hank from Netflix (you can find this on the original thread)

@LHeggy LHeggy merged commit 6a4f5e7 into master Mar 12, 2024
1 check passed
@LHeggy LHeggy deleted the add-underscore branch March 12, 2024 12:07
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.

None yet

3 participants