fix: ensure image oci config is included when building actor oci spec#301
Conversation
Signed-off-by: JM Huibonhoa <jm.huibonhoa@solo.io>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| } | ||
| } | ||
|
|
||
| add("PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin") |
There was a problem hiding this comment.
Should we drop this now?
There was a problem hiding this comment.
yeah realistically speaking the path will be supplied in the image env.
| } | ||
|
|
||
| add("PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin") | ||
| add(templateEnv...) |
There was a problem hiding this comment.
The precedence here seems flipped, templates envs should override image envs.
| } | ||
|
|
||
| var imageEnv []string | ||
| if cfg, cfgErr := img.ConfigFile(); cfgErr != nil { |
There was a problem hiding this comment.
Isn't the config manifest mandatory? Why continue and not returning and error and hard-failing?
There was a problem hiding this comment.
its currently only consumed on this code path which is why I added a warn log and opted not to hard fail. However, I think there is merit in your suggestion since if the resulting image needs the env vars it would be broken anyways particularly given that I've made the other change to remove the default path.
Signed-off-by: JM Huibonhoa <jm.huibonhoa@solo.io>
J.M. Huibonhoa (jmhbh)
changed the title
Signed-off-by: JM Huibonhoa <jm.huibonhoa@solo.io>
|
Julian Gutierrez Oschmann (@juli4n) thanks for your initial review, do you mind taking another pass? |
2 participants
Summary
buildActorOCISpecfunction.Testing
Fixes #300