fixed password endline bug #5500
Conversation
pkg/configs/db/db.go
Outdated
| @@ -77,7 +78,8 @@ func New(cfg Config) (DB, error) { | |||
| if err != nil { | |||
| return nil, fmt.Errorf("Could not read database password file: %v", err) | |||
| } | |||
| u.User = url.UserPassword(u.User.Username(), string(passwordBytes)) | |||
| password := strings.TrimSpace(string(passwordBytes)) | |||
| u.User = url.UserPassword(u.User.Username(), password) | |||
There was a problem hiding this comment.
Looks good, but it's not complete. Ideally for every change we modify or add a tests.
you can extract a function that returns url u and error as needed and test that function.
|
@friedrichg Please let me know if this is what you asked for. If correct, I'll add a few more test cases. |
pkg/configs/db/dbtest/unit.go
Outdated
| @@ -4,6 +4,9 @@ | |||
| package dbtest | |||
There was a problem hiding this comment.
Don't modify this file, this is part of integration tests
pkg/configs/db/dbtest/unit.go
Outdated
| @@ -26,3 +29,48 @@ func Setup(t *testing.T) db.DB { | |||
| func Cleanup(t *testing.T, database db.DB) { | |||
| require.NoError(t, database.Close()) | |||
| } | |||
|
|
|||
| func TestUserPasswordFromPasswordFile(t *testing.T) { | |||
There was a problem hiding this comment.
create a file called db_test.go next to db.go and move this test there.
pkg/configs/db/dbtest/unit.go
Outdated
| defer tempFile.Close() | ||
| defer os.Remove(tempFile.Name()) |
There was a problem hiding this comment.
defer functions have a specific order.
In this case we are removing and then closing. We want the opposite.
https://go.dev/tour/flowcontrol/13#:~:text=Deferred%20function%20calls%20are%20pushed,in%2Dfirst%2Dout%20order.
pkg/configs/db/db.go
Outdated
| return nil, fmt.Errorf("--database.password-file requires username in --database.uri") | ||
| } | ||
| passwordBytes, err := os.ReadFile(cfg.PasswordFile) | ||
| updatedUrl, err := SetUserPassword(u, cfg.PasswordFile) |
There was a problem hiding this comment.
| updatedUrl, err := SetUserPassword(u, cfg.PasswordFile) | |
| updatedURL, err := SetUserPassword(u, cfg.PasswordFile) |
So it passes linting
moki1202
force-pushed
the
bugFix/5444
branch
from
01e23b6 to
2f15833
Compare
Signed-off-by: Shashank <[email protected]>
Signed-off-by: Shashank <[email protected]>
Signed-off-by: Shashank <[email protected]>
|
@friedrichg should I add few more testcases here? please let me know if this PR needs further tweaks. |
|
@moki1202 the lint has to pass and the changelog entry is missing |
Which issue(s) this PR fixes:
Fixes #5444
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]