Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not panic if windows.layerFolder is null #127

Merged
merged 1 commit into from
Apr 30, 2023

Conversation

jsturtevant
Copy link
Contributor

If windows layerFolders in null string then loading of the runtime spec fails. This happens when containerd serializes Windows{} portion of the spec due to the way golang serializes lists. Although the runtime spec says this is a required field, containerd 1.6 and 1.7 do not fill this field resulting in a null value when serialized to disk. See opencontainers/runtime-spec#1185 for discussion of this field in the runtime spec.

fixes #126

@jsturtevant jsturtevant force-pushed the windows-layer-folders branch from fb26b7f to 9d966e3 Compare April 13, 2023 02:29
Copy link
Contributor

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please take a look @Furisto @utam0k

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #127 (b8ca149) into main (a46cd8b) will decrease coverage by 0.64%.
The diff coverage is 14.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   24.39%   23.75%   -0.64%     
==========================================
  Files          21       22       +1     
  Lines        1644     1692      +48     
  Branches      816      839      +23     
==========================================
+ Hits          401      402       +1     
- Misses        546      584      +38     
- Partials      697      706       +9     

@saschagrunert
Copy link
Contributor

saschagrunert commented Apr 13, 2023

Clippy is not happy with the change, though, but it's unrelated.

@jsturtevant
Copy link
Contributor Author

Clippy is not happy with the change, though, but it's unrelated.

pushed up a fix on 92420be

@jsturtevant
Copy link
Contributor Author

clippy --fix didn't format the fix properly 😆

@jsturtevant jsturtevant force-pushed the windows-layer-folders branch from 92420be to 0ae6d4e Compare April 14, 2023 20:50
@jsturtevant
Copy link
Contributor Author

clippy --fix didn't format the fix properly 😆

ran cargo fmt and check for diffs

If windows layerFolders in null string then loading of the runtime spec fails. This happens when containerd serializes Windows{} portion of the spec due to the way golang serializes lists. Although the runtime spec says this is a required field, containerd 1.6 and 1.7 do not fill this field resulting in a null value when serialized to disk. See opencontainers/runtime-spec#1185 for discusion of this field in the runtime spec.

Signed-off-by: James Sturtevant <[email protected]>
@jsturtevant jsturtevant force-pushed the windows-layer-folders branch from 0ae6d4e to b8ca149 Compare April 27, 2023 19:35
@jsturtevant
Copy link
Contributor Author

I dropped the linter commit since #129 merged

@Furisto @utam0k @saschagrunert PTAL

@utam0k utam0k merged commit 2608fc6 into youki-dev:main Apr 30, 2023
@utam0k
Copy link
Member

utam0k commented Apr 30, 2023

@jsturtevant Thanks 🙏

@jsturtevant jsturtevant mentioned this pull request May 1, 2023
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When windows layerFolder is null parsing of the runtime spec fails
4 participants