-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
0581cd0
to
c9d076a
Compare
dc75663
to
9aac2a3
Compare
e673da8
to
47d67ee
Compare
"os" | ||
|
||
log "github.com/sirupsen/logrus" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
database/psqldb.go
Outdated
db.DBName = credential.DatabaseName | ||
} | ||
|
||
func (db *PSQLDataBase) Initalize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A common Go pattern is to use Must<DoSomething>
for method/function names to imply that something must occur and if an error happens the app will crash. We may want to do that here. Right now, getCredentials
does nothing if it finds errors. I think it should probably return an err
and we should log.Fatal if an err exists
database/psqldb.go
Outdated
|
||
func (db *PSQLDataBase) Connect() (string, error) { | ||
psqlInfo := fmt.Sprintf("host=%s user=%s "+ | ||
"password=%s dbname=%s sslmode=disable", //pragma: allowlist secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we not able to use ssl today?
2966787
to
1d15505
Compare
5977b38
to
3d5311c
Compare
database/psqldb_dao.go
Outdated
return 0, fmt.Errorf("could not create user") | ||
} | ||
|
||
log.Infof("Sucessfully created user with name %s and email %s", name, email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.Infof("Sucessfully created user with name %s and email %s", name, email) | |
log.Infof("Sucessfully created user with id %d", userId) |
We should avoid logging potentially sensitive user information. Either of these fields could be considered PII
database/psqldb_dao.go
Outdated
return fmt.Errorf("update user failed") | ||
} | ||
|
||
log.Infof("User %s updated successfully", user.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.Infof("User %s updated successfully", user.Name) | |
log.Infof("User %d updated successfully", user.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me! I'm sure there may be changes as we integrate the DB work, but great start 👍
3d5311c
to
cd386b4
Compare
The change is the work for PXP-8095 which will set up a db interface layer to interact with a psql db that mariner will talk to. unit tests have been added and no breaking changes introduced.