Use YAML instead of JSON for worker config #299
Conversation
|
@tylerharter Modified the config.go, commands.go, and init.py files to use yaml instead of json. Just to clarify - is it necessary to change json to yaml in commands.go and init.py ? Thank you! |
|
@tylerharter Installed yaml for the CI tests |
There was a problem hiding this comment.
Hi Anushka, It seems that the the CI test is failing because the config.yaml is not created and populated correctly. I believe there is one more change need to be done in https://github.com/open-lambda/open-lambda/blob/main/src/worker/helpers.go#L147. It is still initializing config.json but trying to read config.yaml.
| @@ -292,40 +292,31 @@ func checkConf(cfg *Config) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // SandboxConfJson marshals the Sandbox_config of the Config into a JSON string. | |||
| func SandboxConfJson() string { | |||
There was a problem hiding this comment.
Just a quick check — are we confident that this function isn’t currently used or won’t be needed in the near future? Want to make sure we’re not removing something that might be useful down the line.
There was a problem hiding this comment.
Hi, @Mandukhai-Alimaa. I did a grep and it appears that this function is not being used anywhere else in the codebase. Should I delete it ?
There was a problem hiding this comment.
Thanks for checking! Yes, go ahead and delete it
|
@Mandukhai-Alimaa Hi Mandy. I fixed the line in helpers.go to initialize config.yaml instead of config.json, but the CI tests are still failing (i'm getting this same error : FileNotFoundError: [Errno 2] No such file or directory: 'test-dir/config.yaml'). I tried searching for all instances of config.json and replacing it with config.yaml, but that still didn't work. not sure what i'm missing, so any help would be appreciated! |
|
Hi @anushkaShekar, the debugging approach I’d recommend here is to trace the config file’s flow from start to finish. Identify where the file is being created, how it’s being written (and to which directory), and then track all the places where it’s being read. Simply searching and replacing all instances of config.json with config.yaml might not catch everything, especially since IDEs can miss references depending on how they’re used. Following the file’s lineage explicitly will not only help you find the bug more reliably, but also give you a better understanding of the codebase, which will be helpful for future PRs. It’s also useful to take a close look at the error trace. Here’s the traceback from the failure you encountered: File "/home/runner/work/open-lambda/open-lambda/./scripts/test.py", line 369, in This shows exactly where the failure occurs — test-dir/config.yaml is missing. I recommend looking at the files and line numbers mentioned here to better understand how the config file is being handled. Let me know if you have any questions! |
67c390d to
ffbd400
Compare
|
Hi, @Mandukhai-Alimaa! I was able to get all the tests to pass, and just committed my changes. Please lmk if I need to make any additional changes at your convenience. |
There was a problem hiding this comment.
Nice work on this! Just a small thing — it looks like the default-ol and min-image directories were committed by mistake. These shouldn’t be part of the PR, so please remove them. Thanks!
Also, just double-checking — was the intent to update all three configs (worker, boss, and zygote tree) to YAML? I’m asking since @tylerharter mentioned:
"Good progress! Please make sure you're only changing the config with JSON => YAML. We're not trying to replace JSON everywhere, and it seems some code got swept up in that."
If the change to all three configs is indeed intended, it looks like the updated CI test still expects boss.json. You can go ahead and update the test to look for boss.yaml instead.
No description provided.