add support for importing a glob of files with either import or importstr semantics #153

gotwarlost · 2020-09-06T17:56:41Z

This change introduces the ability to import a bag of data or libsonnet files.
It provides the following import syntax:

    import 'glob-import:*.json'
        and
    import 'glob-importstr:*.yaml'

The first form looks for all files that match the wildcard and returns an object keyed by
relative file name with values importing the actual files. i.e. something like:

    {
        'a.json': import 'a.json',
        'b.json': import 'b.json',
    }

The second form is very similar except that it uses 'importstr' for inner imports. The returned
code looks like:

    {
        'a.yaml': importstr 'a.yaml',
        'b.yaml': importstr 'b.yaml',
    }

This allows for loading optional overlay files, user data files and such without having to write out
an import statement in the calling code for each file imported.

This importer is enabled by default in qbec.

codecov-commenter · 2020-09-06T18:00:33Z

Codecov Report

Merging #153 into master will increase coverage by 0.43%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
+ Coverage   73.80%   74.24%   +0.43%     
==========================================
  Files          48       51       +3     
  Lines        4688     4775      +87     
==========================================
+ Hits         3460     3545      +85     
- Misses       1057     1058       +1     
- Partials      171      172       +1

Impacted Files	Coverage Δ
internal/vm/importers/glob.go	`97.14% <97.14%> (ø)`
internal/vm/importers/composite.go	`100.00% <100.00%> (ø)`
internal/vm/importers/file.go	`100.00% <100.00%> (ø)`
internal/vm/vm.go	`97.54% <100.00%> (+0.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a131d7d...9f723f6. Read the comment docs.

gotwarlost · 2020-09-07T14:26:02Z

@sbarzowski - I'm concerned that this code will not work or work differently on Windows. I noticed that the standard file importer uses path.Split(file) instead of filepath.Split(file) so it is always splitting on '/'. I couldn't find any reference doc on how import paths should be constructed (i.e. always Unix style or using the OS separator). Could you clarify this?

gotwarlost · 2020-09-07T17:09:34Z

@kvaps - I think you guys use qbec on windows? If so, could you do me a favor. Check out this branch and see if the unit tests pass on windows.

@harsimranmaan is it possible to run unit tests on windows in CI?

sbarzowski · 2020-09-07T17:42:56Z

@sbarzowski - I'm concerned that this code will not work or work differently on Windows. I noticed that the standard file importer uses path.Split(file) instead of filepath.Split(file) so it is always splitting on '/'. I couldn't find any reference doc on how import paths should be constructed (i.e. always Unix style or using the OS separator). Could you clarify this?

The behavior of Jsonnet programs should not depend on the operating system or environment in general. We don't want paths in the code to be interpreted differently on Unix and Windows, so this is very much by design – the paths are supposed to be always separated by / assuming the convention used by the standard importer. Windows allows both, so it shouldn't be a problem (otherwise the "proper" solution would be to translate the paths in the importer).

Technically, the spec allows the formatter can invent its own completely different convention as long as the basic properties (explained in godoc) are preserved. I would like to have an official format eventually, and it's something I think about quite often recently.

gotwarlost · 2020-09-07T18:37:46Z

The behavior of Jsonnet programs should not depend on the operating system or environment in general. We don't want paths in the code to be interpreted differently on Unix and Windows, so this is very much by design – the paths are supposed to be always separated by / assuming the convention used by the standard importer. Windows allows both, so it shouldn't be a problem (otherwise the "proper" solution would be to translate the paths in the importer).

This is very useful to know. The semantics of filepath.Clean for instance, used in the code, are dependent on the separator. If it is clear that import paths should use /, then I need to translate the / to \ in the globbing code on windows and reverse-translate the import path to be in canonical form during rendering.

Technically, the spec allows the formatter can invent its own completely different convention as long as the basic properties (explained in godoc) are preserved. I would like to have an official format eventually, and it's something I think about quite often recently.

Good to know. I don't want to change current conventions without a good reason. In particular, I think it is important that user-supplied paths be canonical across operating systems so that the jsonnet code can work everywhere.

kvaps · 2020-09-07T20:02:09Z

@kvaps - I think you guys use qbec on windows? If so, could you do me a favor. Check out this branch and see if the unit tests pass on windows.

No, sorry I have no any windows at hand. On archlinux test is failed:

go test  -coverprofile=coverage.txt -covermode=atomic -race ./...
?   	github.com/splunk/qbec	[no test files]
ok  	github.com/splunk/qbec/cmd/changelog-extractor	0.059s	coverage: 66.7% of statements
?   	github.com/splunk/qbec/cmd/gen-qbec-swagger	[no test files]
?   	github.com/splunk/qbec/cmd/jsonnet-qbec	[no test files]
ok  	github.com/splunk/qbec/internal/commands	10.690s	coverage: 86.2% of statements
ok  	github.com/splunk/qbec/internal/diff	0.045s	coverage: 86.2% of statements
ok  	github.com/splunk/qbec/internal/eval	0.530s	coverage: 97.5% of statements
ok  	github.com/splunk/qbec/internal/model	2.661s	coverage: 97.4% of statements
ok  	github.com/splunk/qbec/internal/objsort	0.475s	coverage: 85.1% of statements
ok  	github.com/splunk/qbec/internal/remote	0.402s	coverage: 15.1% of statements
ok  	github.com/splunk/qbec/internal/remote/k8smeta	0.713s	coverage: 85.9% of statements
ok  	github.com/splunk/qbec/internal/rollout	1.665s	coverage: 91.6% of statements
ok  	github.com/splunk/qbec/internal/sio	0.040s	coverage: 100.0% of statements
ok  	github.com/splunk/qbec/internal/types	0.582s	coverage: 96.2% of statements
[helm template] cd /home/kvaps/git/qbec/internal/vm && helm template ./testdata/charts/foobar --name my-name --namespace my-ns --values -
Error: unknown flag: --name
--- FAIL: TestHelmSimpleExpand (0.07s)
    helm_test.go:64: 
        	Error Trace:	helm_test.go:64
        	Error:      	Expected nil, but got: &errors.errorString{s:"RUNTIME ERROR: run helm template command: exit status 1\n\t./consumer.jsonnet:(4:1)-(15:2)\t$\n\tDuring evaluation\t\n"}
        	Test:       	TestHelmSimpleExpand
[helm template] cd /tmp && helm template ./testdata/charts/foobar --name my-name --namespace my-ns --values -
Error: unknown flag: --name
[WARN] helm template command failed, you may need to set the 'thisFile' option to make relative chart paths work
FAIL
coverage: 96.7% of statements
FAIL	github.com/splunk/qbec/internal/vm	0.209s
ok  	github.com/splunk/qbec/internal/vm/importers	0.058s	coverage: 98.3% of statements
FAIL
make: *** [Makefile:37: test] Error 1

helm version
version.BuildInfo{Version:"v3.3", GitCommit:"", GitTreeState:"", GoVersion:"go1.15"}

kvaps · 2020-09-07T20:06:16Z

Ah, after this patch helm test is pass with both helm2 and helm3 binaries:

diff --git a/internal/vm/helm_test.go b/internal/vm/helm_test.go
index a45e84d..bc912d6 100644
--- a/internal/vm/helm_test.go
+++ b/internal/vm/helm_test.go
@@ -54,7 +54,7 @@ expandHelmTemplate(
     },
     {
         namespace: 'my-ns',
-        name: 'my-name',
+        nameTemplate: 'my-name',
         thisFile: std.thisFile,
                verbose: true,
     }

gotwarlost · 2020-09-09T17:56:58Z

@kvaps thanks for reporting your findings. We need to do something soon about helm support based on this and other issues. We'll probably go in the direction of only supporting helm3 etc. Thoughts?

kvaps · 2020-09-09T20:01:53Z

I'm fine with just supporting helm3, but I would be very and very glad if qbec could handle helm hooks #149 as well:
https://helm.sh/docs/topics/charts_hooks/#the-available-hooks

It might be done like aliases to qbec directives, eg. argocd is doing this:

see:

many charts are not working fine without the hooks.

Thank you!

…tstr semantics This change introduces the ability to import a bag of data or libsonnet files. It provides the following import syntax: import 'glob-import:*.json' and import 'glob-importstr:*.yaml' The first form looks for all files that match the wildcard and returns an object keyed by relative file name with values importing the actual files. i.e. something like: { 'a.json': import 'a.json', 'b.json': import 'b.json', } The second form is very similar except that it uses 'importstr' for inner imports. The returned code looks like: { 'a.yaml': importstr 'a.yaml', 'b.yaml': importstr 'b.yaml', } This allows for loading optional overlay files, user data files and such without having to write out an import statement in the calling code for each file imported. This importer is enabled by default in qbec.

harsimranmaan mentioned this pull request Sep 9, 2020

helm3 upgrade would be handled when we plan for a backwards-breaking release #157

Merged

gotwarlost force-pushed the alt-importers branch 2 times, most recently from efba837 to e97b00d Compare September 9, 2020 21:04

gotwarlost force-pushed the alt-importers branch from e97b00d to 9f723f6 Compare September 9, 2020 21:31

gotwarlost changed the title ~~Simpler implementation of glob importer~~ add support for importing a glob of files with either import or importstr semantics Sep 9, 2020

gotwarlost mentioned this pull request Sep 9, 2020

glob importer for review #152

Closed

gotwarlost merged commit bc0d072 into master Sep 9, 2020

gotwarlost deleted the alt-importers branch September 9, 2020 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for importing a glob of files with either import or importstr semantics #153

add support for importing a glob of files with either import or importstr semantics #153

gotwarlost commented Sep 6, 2020 •

edited

Loading

codecov-commenter commented Sep 6, 2020 •

edited

Loading

gotwarlost commented Sep 7, 2020

gotwarlost commented Sep 7, 2020

sbarzowski commented Sep 7, 2020

gotwarlost commented Sep 7, 2020 •

edited

Loading

kvaps commented Sep 7, 2020

kvaps commented Sep 7, 2020 •

edited

Loading

gotwarlost commented Sep 9, 2020

kvaps commented Sep 9, 2020

add support for importing a glob of files with either import or importstr semantics #153

add support for importing a glob of files with either import or importstr semantics #153

Conversation

gotwarlost commented Sep 6, 2020 • edited Loading

codecov-commenter commented Sep 6, 2020 • edited Loading

Codecov Report

gotwarlost commented Sep 7, 2020

gotwarlost commented Sep 7, 2020

sbarzowski commented Sep 7, 2020

gotwarlost commented Sep 7, 2020 • edited Loading

kvaps commented Sep 7, 2020

kvaps commented Sep 7, 2020 • edited Loading

gotwarlost commented Sep 9, 2020

kvaps commented Sep 9, 2020

gotwarlost commented Sep 6, 2020 •

edited

Loading

codecov-commenter commented Sep 6, 2020 •

edited

Loading

gotwarlost commented Sep 7, 2020 •

edited

Loading

kvaps commented Sep 7, 2020 •

edited

Loading