security: move logic in security/password.go to security/password
#80332
Conversation
|
This change is |
ZhouXing19
force-pushed
the
move-password
branch
from
5c829f3 to
44ac5a2
Compare
There was a problem hiding this comment.
Thanks for the hard work 💯
you also need to move password_test.go. see my comment below.
No release note needed here.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ZhouXing19)
pkg/security/password_test.go, line 1 at r1 (raw file):
// Copyright 2022 The Cockroach Authors.
This test file should be moved to the new package.
Use package password_test at the top to ensure the dependencies are separate.
ZhouXing19
force-pushed
the
move-password
branch
3 times, most recently
from
40619cf to
259ef73
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/security/password_test.go, line 1 at r1 (raw file):
Previously, knz (kena) wrote…
This test file should be moved to the new package.
Use
package password_testat the top to ensure the dependencies are separate.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
pkg/security/password/password_test.go, line 11 at r2 (raw file):
// licenses/APL.txt. package password
Are you sure package password_test is not needed? By placing the test file inside the same package, you bring in the settings/cluster dependency to package password.
ZhouXing19
force-pushed
the
move-password
branch
from
259ef73 to
f695a2c
Compare
ZhouXing19
changed the title
security/password.go to security/passwordsecurity/password.go to security/password
ZhouXing19
force-pushed
the
move-password
branch
from
f695a2c to
5eb3c9f
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, knz (kena) wrote…
?
Removed.
pkg/security/password/password_test.go line 11 at r2 (raw file):
Previously, knz (kena) wrote…
Are you sure
package password_testis not needed? By placing the test file inside the same package, you bring in the settings/cluster dependency to packagepassword.
Added the password_test package.
There was a problem hiding this comment.
lgtm!
the failure is from linting:
pkg/security/password/password.go:94:6: type name will be used as password.PasswordHash by other packages, and that stutters; consider calling this Hash
--- FAIL: TestLint/TestGolint (58.64s)
I'm not sure I agree with the litner's suggestion. we could add an exception here, which it looks like others have done too:
cockroach/pkg/testutils/lint/lint_test.go
Line 1668 in c676689
Reviewable status:
ZhouXing19
force-pushed
the
move-password
branch
from
5eb3c9f to
5f728b8
Compare
There was a problem hiding this comment.
Thanks! Added this case in lint_test.go.
Reviewable status:
There was a problem hiding this comment.
Reviewed 8 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:
|
Thanks! bors r+ |
|
bors r- |
|
Canceled. |
This commit is to host the password logic in a separate package, `security/password/`. Release note: None
ZhouXing19
force-pushed
the
move-password
branch
from
5f728b8 to
8994c8e
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed: |
|
I reran the acceptance test and it passed. Looks like it failed a flaky test. bors r+ |
|
Build succeeded: |
This commit is to host the password logic in a separate package,
security/password/.Release note: None