Skip to content

refactor: move updateContainerStoppedLabel to containerutil#1890

Merged
Zheaoli merged 1 commit into
containerd:mainfrom
davidhsingyuchen:move-updateContainerStoppedLabel
Jan 20, 2023
Merged

refactor: move updateContainerStoppedLabel to containerutil#1890
Zheaoli merged 1 commit into
containerd:mainfrom
davidhsingyuchen:move-updateContainerStoppedLabel

Conversation

@davidhsingyuchen
Copy link
Copy Markdown
Contributor

@davidhsingyuchen davidhsingyuchen commented Jan 20, 2023

PR is a blocker of refactoring container stop and container start as part of #1680.

Thought process:

  1. I removed Container from the function name to avoid stuttering with the package name (i.e., containerutil). Also, one of the parameters is container containerd.Container, so the semantics should be already clear.
  2. I noticed that stopped bool is a bit misleading as it seems to say whether the container is stopped, but the point is to convey whether the container is explicitly stopped so that containerd can decide if the container should be restarted. As a result, I renamed the parameter and the function accordingly.

Please let me know if I should do this in a follow-up PR instead, or it just doesn't make sense and shouldn't be done at all, thanks!

Signed-off-by: Hsing-Yu (David) Chen davidhsingyuchen@gmail.com

Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>
@davidhsingyuchen davidhsingyuchen force-pushed the move-updateContainerStoppedLabel branch from 1560e93 to eec294d Compare January 20, 2023 03:39
@davidhsingyuchen davidhsingyuchen marked this pull request as ready for review January 20, 2023 03:54
Copy link
Copy Markdown
Member

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

LGTM

@Zheaoli Zheaoli merged commit 76d9745 into containerd:main Jan 20, 2023
@Laitr0n
Copy link
Copy Markdown
Contributor

Laitr0n commented Jan 20, 2023

We're doing some works that are the same. I refactored the container start in PR #1864, but I discovered that the CL is enormous, which made it difficult for reviewers to review. So,I am breaking it down into multiple PRs such as #1882, #1891 and so on. Once one PR is merged, I will proceed with the next one.

@davidhsingyuchen
Copy link
Copy Markdown
Contributor Author

davidhsingyuchen commented Jan 20, 2023

We're doing some works that are the same.

Oh I see, I checked the open PRs yesterday, and it seems that #1864 was just reopened today. I will stop creating PRs for those items.

Once one PR is merged, I will proceed with the next one.

Maybe they can be done in parallel as the refactoring blocks not only container start but also other commands. I think the point is to break the giant PR into smaller/independent ones instead of doing them serially.

@Laitr0n
Copy link
Copy Markdown
Contributor

Laitr0n commented Jan 21, 2023

SGTM.
I will open more pr that has no dependent with another at the same time.

@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jan 22, 2023
@davidhsingyuchen davidhsingyuchen deleted the move-updateContainerStoppedLabel branch January 23, 2023 07:33
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