Skip to content

[cri/config] : fix range iterator issue in ValidatePluginConfig#4772

Merged
estesp merged 1 commit intocontainerd:masterfrom
gaurav1086:ValidatePluginConfig_fix_range_iterator_issue
Dec 7, 2020
Merged

[cri/config] : fix range iterator issue in ValidatePluginConfig#4772
estesp merged 1 commit intocontainerd:masterfrom
gaurav1086:ValidatePluginConfig_fix_range_iterator_issue

Conversation

@gaurav1086
Copy link
Copy Markdown
Contributor

@gaurav1086 gaurav1086 commented Nov 27, 2020

Go uses the same address variable while iterating in a range, so use a copy when using its address.

Sample code to highlight the issue: https://play.golang.org/p/YSqm1Gu3O0i

@k8s-ci-robot
Copy link
Copy Markdown

Hi @gaurav1086. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Nov 27, 2020

Build succeeded.

@gaurav1086 gaurav1086 force-pushed the ValidatePluginConfig_fix_range_iterator_issue branch from 95fda97 to 8ade062 Compare December 2, 2020 15:18
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 2, 2020

Build succeeded.

@estesp
Copy link
Copy Markdown
Member

estesp commented Dec 2, 2020

Please sign your commit to pass CI validation

@gaurav1086 gaurav1086 force-pushed the ValidatePluginConfig_fix_range_iterator_issue branch from 8ade062 to 60f2fe7 Compare December 2, 2020 16:12
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 2, 2020

Build succeeded.

@gaurav1086
Copy link
Copy Markdown
Contributor Author

@estesp The error appears to be: FAIL - commit subject exceeds 90 characters . Is there a character limit on the subject ?

@estesp
Copy link
Copy Markdown
Member

estesp commented Dec 4, 2020

yes, commit messages should start with a short "subject" line, then a blank line and then any more informative/descriptive text and then sign-off. In your case because that string of text just continues into a full sentence it breaks the 90-char limit check. The title of this PR would be a better commit subject and the current text should be the longer detail. Example:

This is my commit title

This is why I made this commit. Here are some more details on
what is solves.

Signed-off-by: ....

Go uses the same address variable while iterating in a range,
so use a copy when using its address.

Signed-off-by: Gaurav Singh <[email protected]>
@gaurav1086 gaurav1086 force-pushed the ValidatePluginConfig_fix_range_iterator_issue branch from 60f2fe7 to 071a185 Compare December 4, 2020 22:39
@gaurav1086
Copy link
Copy Markdown
Contributor Author

Thanks @estesp . Changes made.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 4, 2020

Build succeeded.

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

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

@estesp estesp merged commit 73a301c into containerd:master Dec 7, 2020
@AkihiroSuda AkihiroSuda added cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch and removed cherry-pick/1.4.x labels Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch needs-ok-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants