Remove image spec fork that was added to support ArgsEscaped non-OCI docker field#44
Conversation
Does this refer to the fact that once opencontainers/image-spec#892 gets in, we can remove this change? |
I was referring more to your WIP changes that I am looking into as your changes seem to handle all cases and I was wondering if we just wanted to have the upstream fix in our fork too and that's why I mentioned it. But after giving this a second thought, it should not be a problem to leave these changes as is since we longer have dependency on image-spec fork. |
I think even with my WIP changes, we still need a way to determine if ArgsEscaped is set on the image config, so we will still need this change (or ArgsEscaped added to image-spec). IIRC this same change was part of my WIP. |
I updated the description. Since I am in the process of adding regression tests to your WIP code changes, I'll check them in as part of that PR. |
helsaawy
left a comment
helsaawy
left a comment
There was a problem hiding this comment.
If I am understanding correctly, the blob data in the content store is set by docker, so we don't need to set that when pulling an image, right?
kiashok
force-pushed
the
removeImageSpecFork
branch
from
1cca14d to
6baf401
Compare
That is right. We are only trying to read argsEscaped from the blob data we got in |
3 participants
Root Cause:
Image-spec fork was added to our CRI branch to support the non-OCI docker image-spec field called 'ArgsEscaped'. ArgsEscaped is used by docker to determine how the image command should be interpreted. If ArgsEscaped is set, then the first element of the command array will be used as-is.
We would like to get rid of the dependency on the image-fork spec while we work on a cleaner fix that can be pushed upstream as well.
Fix:
Instead of relying on the image-fork spec and adding the ArgsEscaped field to our image spec, we try to read the ArgsEscaped value by making an extra json.Unmarshal call during getImage()
Testing
Manually tested the changes to make sure that the fix does not regress any behavior that the original fix was made for. I will be adding regression tests to the upcoming PR that has the complete upstream fix.