Skip to content

Replace uses of strings.Split(N) with strings.Cut()#3936

Merged
thaJeztah merged 14 commits intodocker:masterfrom
thaJeztah:strings_cut
Dec 29, 2022
Merged

Replace uses of strings.Split(N) with strings.Cut()#3936
thaJeztah merged 14 commits intodocker:masterfrom
thaJeztah:strings_cut

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

Similar to moby/moby#44381

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Dec 27, 2022
@thaJeztah thaJeztah added this to the 23.0.0 milestone Dec 27, 2022
@thaJeztah thaJeztah force-pushed the strings_cut branch 5 times, most recently from f636304 to 0bac506 Compare December 27, 2022 22:05
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 27, 2022

Codecov Report

Merging #3936 (0bac506) into master (ed94b6e) will decrease coverage by 0.00%.
The diff coverage is 63.21%.

❗ Current head 0bac506 differs from pull request most recent head c4c7638. Consider uploading reports for the commit c4c7638 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3936      +/-   ##
==========================================
- Coverage   59.21%   59.20%   -0.01%     
==========================================
  Files         287      287              
  Lines       24720    24690      -30     
==========================================
- Hits        14637    14617      -20     
+ Misses       9198     9189       -9     
+ Partials      885      884       -1     

@thaJeztah
Copy link
Copy Markdown
Member Author

@rumpl ptal 🤗

Copy link
Copy Markdown
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

Small typo 😅

return securityOpts, errors.Errorf("Invalid --security-opt: %q", opt)
}
k, v, ok := strings.Cut(opt, "=")
if !ok && k != "no-new-privileged" {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

no-new-privileges -> no-new-privileged? 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh! 💩 🙈 Good catch. I, erm, blame auto-correct, but I likely fat-fingered 😂 .

if !ok && k != "no-new-privileged" {
k, v, ok = strings.Cut(opt, ":")
}
if (!ok || v == "") && k != "no-new-privileged" {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as above

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This validator was not specific to "log options", so renaming it to make this
clearer; also updating values used in the tests to make it clear they're not
"actual" valid values, just for testing, and while updating, also fixed some
unhandled errors in tests.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Copy Markdown
Member Author

@vvoland fixed the typos 😅

(I'd love to get rid of some of that logic in the ParseSecurityOpts; I don't think it should all belong in that function, or even in the CLI code)

Copy link
Copy Markdown
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants