Skip to content

integration: TestUpdateContainerResources_MemoryLimit: remove TODO comment#7367

Merged
kzys merged 1 commit intocontainerd:mainfrom
thaJeztah:cleanup_todo_comment
Sep 6, 2022
Merged

integration: TestUpdateContainerResources_MemoryLimit: remove TODO comment#7367
kzys merged 1 commit intocontainerd:mainfrom
thaJeztah:cleanup_todo_comment

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

This comment was added in 09a0c94 (#5163) when the Windows integration tests were enabled. The PR (microsoft/hcsshim#931) was merged, and part of hcsshim v0.9.0, and support for resource limits on Windows was added in 2bc77b8 (#5778), so it looks like this comment is no longer current.

…mment

This comment was added in 09a0c94 when the
Windows integration tests were enabled. The PR (microsoft/hcsshim#931) was
merged, and part of hcsshim v0.9.0, and support for resource limits on Windows
was added in 2bc77b8, so it looks like this
comment is no longer current.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

/cc @claudiubelu @katiewasnothere @dcantah ptal; I stumbled upon this comment while looking for uses of hccsshim, and I think it was outdated, but perhaps there was still something left to do

Copy link
Copy Markdown
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

Not sure if there was more to do for this comment specifically (other than adding a similar test) but the functionality is there #5187 so LGTM. I'll leave the final say to @katiewasnothere and @claudiubelu though. Maybe we should make an issue to add a similar set of tests for windows to track this better.

It looks like there is a test already that should be at least using the functionality to my surprise

err = runtimeService.UpdateContainerResources(cnTruncIndex, nil, &runtimeapi.WindowsContainerResources{

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@kzys kzys merged commit d99e62d into containerd:main Sep 6, 2022
@thaJeztah thaJeztah deleted the cleanup_todo_comment branch September 6, 2022 15:46
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.

4 participants