feat(cmd): add db's user list #1019

curzolapierre · 2023-11-13T15:50:02Z

Add a changelog entry in the section "To Be Released" of CHANGELOG.md
Update go-scalingo once it has been released (feat(database): add user management routes go-scalingo#365)

List

$ scalingo database-list-users --help
NAME:
   scalingo database-list-users - Print database's users

USAGE:
   scalingo database-list-users command [command options] [arguments...]

DESCRIPTION:
   List the users of a database

   Only available on [postgresql elasticsearch influxdb]

   Example
     $ scalingo --app myapp --addon addon-uuid database-list-users

OPTIONS:
   --app value, -a value  Name of the current app (default: "<name>")
   --addon value          ID of the current addon (default: "<addon_id>")
   --region value         Name of the region to use
   --help, -h             show help

Delete

$ scalingo database-delete-user --help
NAME:
   scalingo database-delete-user - Delete a database's user

USAGE:
   scalingo database-delete-user command [command options] user

DESCRIPTION:
   Delete the given user of a database

   Only available on [postgresql elasticsearch influxdb]

   Example
     $ scalingo --app myapp --addon addon-uuid database-delete-user my_user

OPTIONS:
   --app value, -a value  Name of the current app (default: "<name>")
   --addon value          ID of the current addon (default: "<addon_id>")
   --region value         Name of the region to use
   --help, -h             show help

Create

$ scalingo database-create-user --help
NAME:
   scalingo database-create-user - Create new database user

USAGE:
   scalingo database-create-user command [command options] user

DESCRIPTION:
   Create new database user

   Only available on [postgresql elasticsearch influxdb]

   Examples
     $ scalingo --app myapp --addon addon-uuid database-create-user my_user
     $ scalingo --app myapp --addon addon-uuid database-create-user my_user --read-only

OPTIONS:
   --app value, -a value  Name of the current app (default: "<name>")
   --addon value          ID of the current addon (default: "<addon_id>")
   --read-only            Create a user with read-only rights (default: false)
   --region value         Name of the region to use
   --help, -h             show help

Fixes #1018

yohann-bacha

nice job! some small issues, but nothing too serious

yohann-bacha · 2023-11-16T09:07:57Z

cmd/databases.go

+		}.Render(),
+
+		Action: func(c *cli.Context) error {
+			if c.Args().Len() != 1 {


issue: in this case, an error should be returned. the exit code 0 means the operation was done successfully, which is not the case here

Well the logic we implement in our CLI is more an error meaning CLI itself has failed to do something.
Here we succeed to print the help command because the number of argument is not expected.
It may be debatable, but for the sake of consistency with the rest of CLI, I'll leave it like that.

wtf you're in fact right, but that is weird.. i really thought that there were a client that pointed this behaviour, and that we should handle it differently. this should return an exit code 1, but you are right : consistency is more important

cmd/databases.go

db/users/utils.go

db/users/create.go

yohann-bacha · 2023-11-16T09:19:40Z

db/users/utils.go

+	scErrors "github.com/Scalingo/go-utils/errors/v2"
+)
+
+var ErrDatabaseNotSupportUserManagement = errors.New("Error: DBMS does not support user management")


issue: not a fan of making a constant of an error, as we never do this anywhere in our code. we often have a same base error such as "user cannot be fetched", but we do not factorize them

as we never do this anywhere in our code

Not true, just in this repo I can see some, in addition, we usually do it for two main reason:

use the constant (preferably a typed) to compare an error

avoid duplication

In this case it's the latter we're interested in

just in this repo I can see some

not really sure about that one as I never saw one in my peregrinations in all our go code. in fact, i like the idea that we centralize our errors, and then identify them more easily. though, i see this more as a consistency issue. if we do this here, we should do it every where errors are duplicated.

this is, though, not blocking

yohann-bacha · 2023-11-16T09:21:00Z

db/users/utils.go

+		return false, scErrors.Wrap(ctx, err, "get the addon to check user management support")
+	}
+
+	for _, supportedAddon := range supportedAddons {


praise: i like the simple use of plurals and singular in a foreach

yohann-bacha · 2023-11-16T09:23:35Z

db/users/create.go

+		return nil
+	}
+
+	isPasswordGenerated := false


issue (blocking?): the password generation logic is made backend sided. do we want to handle it on the client side?

I agree, the reason of this check here was to return error has early has possible. This means if the username is incorrect there is no need to continue the process (i.e. password, confirmation)
But WDYT?

well that could have remove some complexity client sided, but this isn't really an issue and is not blocking, do as your heart wishes

yohann-bacha · 2023-11-16T09:24:56Z

db/users/create.go

+func askForPassword(ctx context.Context) (string, string, error) {
+	fmt.Printf("Password: (Will be generated if left empty) ")
+
+	bytePassword, err := term.ReadPassword(int(os.Stdin.Fd()))


issue: no need to specify the type of the variable via its name

Suggested change

bytePassword, err := term.ReadPassword(int(os.Stdin.Fd()))

password, err := term.ReadPassword(int(os.Stdin.Fd()))

yohann-bacha · 2023-11-16T09:25:28Z

db/users/create.go

+	}
+
+	fmt.Printf("\nPassword Confirmation: ")
+	byteConfirmedPassword, err := term.ReadPassword(int(os.Stdin.Fd()))


issue: same, no need for specifying the type in the variable name

yohann-bacha

well the last nitpicks are not enough to block my approval, so LGTM! nice job

notion-workspace · 2024-01-24T10:11:59Z

Add CLI feature about database user management

curzolapierre self-assigned this Nov 13, 2023

curzolapierre force-pushed the feat/1018/db_user_management branch from d5dd9d1 to bfcb8bd Compare November 13, 2023 15:51

curzolapierre marked this pull request as ready for review November 14, 2023 11:20

curzolapierre force-pushed the feat/1018/db_user_management branch from 3aa85ce to c6a9c35 Compare November 14, 2023 15:01

curzolapierre requested a review from yohann-bacha November 14, 2023 15:04

yohann-bacha suggested changes Nov 16, 2023

View reviewed changes

curzolapierre force-pushed the feat/1018/db_user_management branch from c6a9c35 to c1d2739 Compare November 16, 2023 15:05

curzolapierre requested a review from yohann-bacha November 16, 2023 15:52

yohann-bacha approved these changes Nov 17, 2023

View reviewed changes

curzolapierre added 2 commits November 17, 2023 15:01

feat(cmd): add db's user list

9ac1db2

feat(cmd): add delete user from database

8729a10

curzolapierre force-pushed the feat/1018/db_user_management branch from 3ea9edd to d0941f5 Compare November 17, 2023 14:02

curzolapierre added 3 commits November 17, 2023 15:28

feat(cmd): add database user creation

b394a6e

chore: update deps

bc7ea20

fix(cmd): add list of databases to user managements cmd description

52f6e47

curzolapierre force-pushed the feat/1018/db_user_management branch from d0941f5 to 52f6e47 Compare November 17, 2023 14:29

curzolapierre merged commit 9b80d09 into master Nov 17, 2023
5 checks passed

curzolapierre deleted the feat/1018/db_user_management branch November 17, 2023 14:47

curzolapierre mentioned this pull request Jan 24, 2024

[Database Users] Create aliases to be more consitent with naming #1030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cmd): add db's user list #1019

feat(cmd): add db's user list #1019

curzolapierre commented Nov 13, 2023 •

edited

Loading

yohann-bacha left a comment

yohann-bacha Nov 16, 2023

curzolapierre Nov 16, 2023 •

edited

Loading

yohann-bacha Nov 17, 2023

yohann-bacha Nov 16, 2023

curzolapierre Nov 16, 2023

yohann-bacha Nov 17, 2023

yohann-bacha Nov 16, 2023

yohann-bacha Nov 16, 2023

curzolapierre Nov 16, 2023

yohann-bacha Nov 17, 2023

yohann-bacha Nov 16, 2023

yohann-bacha Nov 16, 2023

yohann-bacha left a comment

notion-workspace bot commented Jan 24, 2024

	bytePassword, err := term.ReadPassword(int(os.Stdin.Fd()))
	password, err := term.ReadPassword(int(os.Stdin.Fd()))

feat(cmd): add db's user list #1019

feat(cmd): add db's user list #1019

Conversation

curzolapierre commented Nov 13, 2023 • edited Loading

List

Delete

Create

yohann-bacha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curzolapierre Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yohann-bacha left a comment

Choose a reason for hiding this comment

notion-workspace bot commented Jan 24, 2024

curzolapierre commented Nov 13, 2023 •

edited

Loading

curzolapierre Nov 16, 2023 •

edited

Loading