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

feat(jsonnet): add support for importing all files in a directory #1374

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

partcyborg
Copy link
Contributor

@partcyborg partcyborg commented Mar 4, 2025

Add native function importFiles which reads and evaluates all jsonnet files in a directory.

Returns an object of with key/value pairs of <file name>: <evaluated object>

Directory path is found using a calledFrom option, similar to helmTemplate.

Includes support for:

  • Choosing files by extension using the extension option (default is .libsonnet)
  • Excluding files using the exclude option, which takes an array of file names to skip

Fixes #1375

@partcyborg
Copy link
Contributor Author

partcyborg commented Mar 4, 2025

If accepted, I will add support for this function to jsonnet-libs

Additionally, I noticed that the TestColordiff test is failing locally, but I am not sure if that is related to my local system or caused by something else.

@partcyborg partcyborg force-pushed the import-files branch 2 times, most recently from d0afe89 to a73c5d1 Compare March 5, 2025 00:45
dsotirakis
dsotirakis previously approved these changes Mar 10, 2025
Copy link
Contributor

@dsotirakis dsotirakis left a comment

Choose a reason for hiding this comment

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

@partcyborg 👋
Thanks for your contribution! Would you also be able to add some tests please, for our better understanding and ease of reviewing?

@dsotirakis dsotirakis dismissed their stale review March 10, 2025 09:46

meant to comment

@partcyborg
Copy link
Contributor Author

partcyborg commented Mar 10, 2025

@partcyborg 👋 Thanks for your contribution! Would you also be able to add some tests please, for our better understanding and ease of reviewing?

Hey @dsotirakis, thank you for your reply. I have added a unit test for the importFiles native function

Update: I noticed i was missing a test for the exclude functionality, so i updated the test to ensure that is working too

Add native function `importFiles` which reads and evaluates all jsonnet files in a directory.

Returns an object of with key/value pairs of `<file name>: <evaluated object>`

Directory path is found using a `calledFrom` option, similar to `helmTemplate`.

Includes support for:
 - Choosing files by extension using the `extension` option (default is `.libsonnet`)
 - Excluding files using the `exclude` option, which takes an array of file names to skip
@partcyborg partcyborg force-pushed the import-files branch 5 times, most recently from 4c46530 to 0f7d83d Compare March 12, 2025 19:34
Copy link
Contributor

@zerok zerok left a comment

Choose a reason for hiding this comment

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

Thank you for proposing this 🙂 I've left you a comment also in the issue with an idea for a different approach 🙂

@@ -208,3 +225,41 @@ func TestRegexSubstInvalid(t *testing.T) {
assert.Empty(t, ret)
assert.NotEmpty(t, err)
}

func TestImportFiles(t *testing.T) {
tempDir, err := os.MkdirTemp("", "importFilesTest")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Please use T.TempDir here. With that you don't need to do the os.RemoveAll manually later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced this with a call to T.TempDir()

importDir := filepath.Join(tempDir, importDirName)
err = os.Mkdir(importDir, 0750)
assert.Nil(t, err)
importFiles := []string{"test1.libsonnet", "test2.libsonnet"}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think it would make the test easier to read if the files were explicitly filled with certain content and the result was compared to an "expected" JSON string 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refactored the test to

  • Use static jsonnet content in the imported and excluded files
  • Compare the result in json format with the expected json

Please let me know if this is what you were thinking.

}

// importFiles imports and evaluates all matching jsonnet files in the given relative directory
func importFiles(vm *jsonnet.VM) *jsonnet.NativeFunction {
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: Please add documentation about this new function to https://github.com/grafana/tanka/blob/main/docs/src/content/docs/jsonnet/native.md 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zerok I am happy to do this, but held off because I don't see documents for helmTemplate here either. My original plan was to add jsonnet library code similar to the helm functions in tanka-util and document those.

That being said, I am happy to also add documentation for the raw native function here, but I would like to get some clarity on whether this will be accepted before I do the work to add it.

@Duologic
Copy link
Member

This breaks the hermitic nature of jsonnet, I'd vouch heavily against doing this in Tanka as we'd be creating a parallel ecosystem that is different from upstream.

See upstream discussion here: https://groups.google.com/g/jsonnet/c/JdvlXDAdvq0

Copy link
Member

@Duologic Duologic left a comment

Choose a reason for hiding this comment

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

adding a request changes as I would love to have a more in depth discusssion about this, see comments in #1375

@partcyborg
Copy link
Contributor Author

This breaks the hermitic nature of jsonnet, I'd vouch heavily against doing this in Tanka as we'd be creating a parallel ecosystem that is different from upstream.

See upstream discussion here: https://groups.google.com/g/jsonnet/c/JdvlXDAdvq0

While I see your point, I don't think has the impact you think it does for a few reasons:

  1. The jsonnet files rendered by importFiles are each evaluated in their own jsonnet context, with the result being a json object. This means there is no late binding between the primary VM, nor between individual files. My plan is to make this clear in the documentation and that the primary use case for this is for generating configs for inline environments.
  2. The hermetic nature of jsonnet is already broken in Tanka via the helmTemplate native function, as this loads content from an arbitrary number of files in an arbitrary directory

@Duologic
Copy link
Member

Why not render the JSON beforehand then? Seems like this could be handled by the tooling pipeline rather than the jsonnet code.

The fact that we break hermiticity in another feature does not warrant that we should do it regardless. These features have different tradeoffs.

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.

Add support for importing all jsonnet files in a directory
4 participants