vim-dadbod icon indicating copy to clipboard operation
vim-dadbod copied to clipboard

URL param support for redis adapter

Open guruor opened this issue 1 year ago • 11 comments

This should be better alternative to #129

Test case:

:echo db#adapter#dispatch("redis://localhost:6482/0?c&no-auth-warning", "interactive")

Result:

['redis-cli', '-c', '--no-auth-warning', '-h', 'localhost', '-p', '6482', '-n', '0']

guruor avatar Apr 29 '23 15:04 guruor

This is indeed a better approach. But we should limit it to connection parameters; half or more of these (e.g., eval) have no business being in a connection URI. And we don't need single character aliases.

tpope avatar Apr 30 '23 14:04 tpope

@tpope I just went through the redis-cli --help to get the non-boolean flags and added here. You are right we can limit to only connection parameters. I think few flags like cert, key, cacert, capath and tls-ciphers should be sufficient. I might be missing some crucial flags here but we can add those later when there is a need of it.

I added single character flag support because few of the flag are single character but not alias like u,r,i,d,D and c.

guruor avatar May 01 '23 10:05 guruor

@tpope Have added the suggested changes, please review once again and let me know if any other change is required.

guruor avatar May 07 '23 15:05 guruor

Didn't notice those single character options weren't aliases. But they don't really seem necessary to me, with the possible exception of -c. I would drop all of them; if someone wants -c we can add it back.

tpope avatar May 07 '23 17:05 tpope

@tpope Removed the other single char flags (u, r, i, d and D) and weren't explicitly handling the -c anyways. Now we are good to go.

guruor avatar May 08 '23 02:05 guruor

Options like no-auth-warning should be set globally. Set it for every connection is tiresome.

hiberabyss avatar May 16 '23 07:05 hiberabyss

  --no-auth-warning  Don't show warning message when using password on command
                     line interface.

Reading this description, yes, the fix for this option isn't a configuration option or a URL parameter, we should just pass it by default (or better, fix it to pass the password as an environment variable).

tpope avatar May 17 '23 04:05 tpope

Pass password by environment is a good idea. The mysql warning could also be suppressed via environment password like MYSQL_PWD=pass mysql -u user -h host.

hiberabyss avatar May 17 '23 04:05 hiberabyss

@tpope Had pushed the updated changes, I think it is ready to merge now. let me know if you still have any suggestion or concerns.

guruor avatar Jun 08 '23 12:06 guruor

@tpope Were you able to check the new changes?

guruor avatar Jul 10 '23 06:07 guruor

@tpope bumping it once again, apologies for spamming. Just wanted to make sure if it being checked.

guruor avatar Aug 04 '23 11:08 guruor