Add Jpaths for jsonnet #2679 #2678
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #2678 +/- ##
==========================================
- Coverage 38.13% 38.11% -0.02%
==========================================
Files 156 156
Lines 17297 17311 +14
Branches 229 203 -26
==========================================
+ Hits 6596 6598 +2
- Misses 9882 9898 +16
+ Partials 819 815 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for your contribution, @anarcher ! Overall, it looks good and useful to me, and I can see the use-case.
Please have a look at my code comments regarding jumping out of appPath (needs to be addressed) and the documentation (minor).
reposerver/repository/repository.go
Outdated
| @@ -486,8 +486,12 @@ func findManifests(appPath string, env *v1alpha1.Env, directory v1alpha1.Applica | |||
| objs = append(objs, &obj) | |||
| } else if strings.HasSuffix(f.Name(), ".jsonnet") { | |||
| vm := makeJsonnetVm(directory.Jsonnet, env) | |||
| jpaths := []string{appPath} | |||
| for _, p := range directory.Jsonnet.JPaths { | |||
| jpaths = append(jpaths, filepath.Join(appPath, p)) | |||
There was a problem hiding this comment.
We should really make sure that we don't jump out of appPath here, i.e. when p is ../../foo/bar or similar. Paths should only be added if they are within appPath after calling filepath.Join()
There was a problem hiding this comment.
my usecase like below:
├── environments
│ └── default
│ ├── app.yaml
│ ├── main.jsonnet
│ └── spec.json
├── jsonnetfile.json
├── jsonnetfile.lock.json
├── lib
└── vendor
├── k.libsonnet
├── k8s.libsonnet
└── ksonnet-util
├── jaeger.libsonnet
└── kausal.libsonnet
If appPath is environments/default or something else like environments/XXX, jpaths can be that ../../vendor and ../../lib . So I need to access appRoot (e.g.: gitRoot or local file path of spec.source.repoURL).
IMHO, Paths shoule be there inside appRoot. Is it right? Do you have a good idea about it? :->
| vm.Importer(&jsonnet.FileImporter{ | ||
| JPaths: []string{appPath}, | ||
| JPaths: jpaths, |
There was a problem hiding this comment.
It should be documented somewhere that appPath will be an implicit member of jpath, even if not explicitly specified by --jsonnet-jpath
But, This failure is not reproduced with my desktop ㅠ |
Checklist:
Closes #2679