Separate the GenerateRandomAlphaOnlyString function from stringutils#35313
Separate the GenerateRandomAlphaOnlyString function from stringutils#35313thaJeztah merged 1 commit intomoby:masterfrom
Conversation
ChiuWang
force-pushed
the
RandomAlpha
branch
3 times, most recently
from
14014ca to
a0f0dcc
Compare
integration-cli/cli/cli.go
Outdated
There was a problem hiding this comment.
Perhaps we should move it somewhere inside docker/internal/test/, together with the existing unit tests; that marks the package as "internal" only
There was a problem hiding this comment.
Oh, looks like we have a "testutil" directory there already; https://github.com/moby/moby/blob/master/internal/testutil/
There was a problem hiding this comment.
You can create a file there, and move the GenerateRandomAlphaOnlyString() function there, including the associated tests from https://github.com/moby/moby/blob/master/pkg/stringutils/stringutils_test.go
As I noticed in #35034 (review), GenerateRandomASCIIString() is not used anywhere, so that can be removed from the pkg/stringutils/ as well (I suggest doing that all in a single PR/commit so that it's easier to track where things went)
There was a problem hiding this comment.
Yes. As I comment it above in the tasklist, it will be done next. And would plz check PR func Ellipsis() at docker/cli
There was a problem hiding this comment.
Oh, missed that comment; perfect!
Let me review that PR, I see you updated 👍
|
Please sign your commits following these rules: $ git clone -b "RandomAlpha" [email protected]:charrywanganthony/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
GordonTheTurtle
added
dco/no
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
second commit needs to be signed, otherwise LGTM
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
@charrywanganthony can you remove the GenerateRandomAlphaOnlyString() and tests from the original package? I'd prefer to have that done in the same pull request.
Also, can you squash your commits?
GordonTheTurtle
removed
the
dco/no
Signed-off-by: chaowang <[email protected]>
|
Modified. squash commits and remove function from original package. |
5 participants
Signed-off-by: Chao Wang [email protected]
- What I did
GenerateRandomAlphaOnlyString() in pkg/stringutils only used in test(integration-cli), so we move it to the integration-cli/cli. And it's a part of improving stringutls.
- How I did it
According to @thaJeztah 's suggestion at /moby/moby/pull/35034(closed)
Move
Ellipsis()to Docker/cli repository, and rewrite to fix multilanguage display bug. docker/cli/pull/621Separate the
GenerateRandomAlphaOnlyString()function from stringutils.We should copy
Inslice()to those parts that use it, possibly renaming it for each purpose. ( Used in a couple of tests, and also in the seccomp and oci_linux parts )stringutils can be removed safely.
Truncate() & GenerateRandomASCIIString() & ShellQuoteArguments() & InSlice()