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

Update version.rs segfault and WithAutoRefresh rename #39

Merged
merged 3 commits into from
Nov 2, 2024

Conversation

zvonkok
Copy link
Contributor

@zvonkok zvonkok commented Oct 30, 2024

Initialize Cache with auto_refresh true according to CDI go-impl
Rename WithAutoRefresh and adjust to CdiOption.
Updated version.rs to not panic

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok zvonkok force-pushed the rename-with-auto-refresh branch from 1d2f780 to 3a01b8c Compare October 30, 2024 23:56
@zvonkok zvonkok requested a review from Apokleos October 31, 2024 00:35
@zvonkok
Copy link
Contributor Author

zvonkok commented Oct 31, 2024

@ChengyuZhu6 @Apokleos PTAL

…t we cannot use Option is a reserved keyword

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok zvonkok force-pushed the rename-with-auto-refresh branch from 5b973eb to 2090a19 Compare October 31, 2024 00:38
@zvonkok
Copy link
Contributor Author

zvonkok commented Oct 31, 2024

Fixes: #40

Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

Thx @zvonkok
Just one comment left to discuss introducing Cache::new(auto_refresh:bool) {}

src/version.rs Outdated Show resolved Hide resolved
src/cache.rs Outdated Show resolved Hide resolved
@zvonkok zvonkok force-pushed the rename-with-auto-refresh branch from 9c88984 to 68d7e6b Compare October 31, 2024 16:52
@zvonkok zvonkok changed the title Initialize Cache with auto_refresh true according to CDI go-impl Update version.rs segfault Oct 31, 2024
@zvonkok zvonkok changed the title Update version.rs segfault Update version.rs segfault and WithAutoRefresh rename Oct 31, 2024
Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok zvonkok force-pushed the rename-with-auto-refresh branch from f8a2bd8 to d325c31 Compare October 31, 2024 19:05
@zvonkok
Copy link
Contributor Author

zvonkok commented Oct 31, 2024

@Apokleos Also fixed a bug with edits, what we missed is that we might only have dev.edits and if spec.edits is None it will skip adding the dev.edits here is an example

{
            "cdiVersion": "0.6.0",
            "kind": "kata.com/gpu",
            "devices": [
                {
                "name": "0",
                "annotations": {
                    "whatever": "false",
                    "whenever": "true"
                },
                "containerEdits": {
                    "deviceNodes": [
                    {
                        "path": "/dev/zero"
                    }
                    ]
                }
                }
            ]
        }

Copy link
Contributor

@ChengyuZhu6 ChengyuZhu6 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Apokleos Apokleos left a comment

Choose a reason for hiding this comment

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

I think we should first fix related issues and make it merged. LGTM!

@Apokleos Apokleos merged commit fba5677 into cncf-tags:main Nov 2, 2024
2 of 3 checks passed
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.

3 participants