Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: add redis-socket scheme for connect to redis with unix socket #598

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ru-asdx
Copy link

@ru-asdx ru-asdx commented Nov 23, 2022

Ref: #595

address of redis storage now can be specified as :

redis_brocker: "redis-socket://pwd@/run/redis/redis.sock?db=0"

Copy link
Member

@mrd0ll4r mrd0ll4r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

I've added one comment. Also, there's documentation, could you add a note about the addresses in there, too, please? And maybe in the example config as well.

Don't worry about CI failing. We'll fix it at some point and merge then.

Thank you :)

storage/redis/redis.go Outdated Show resolved Hide resolved
mrd0ll4r
mrd0ll4r previously approved these changes Nov 25, 2022
Copy link
Member

@mrd0ll4r mrd0ll4r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Again, the CI is broken, we'll fix it at some point. Because of that I won't merge it now, just leave it open, we'll come back to it :)

@mrd0ll4r
Copy link
Member

mrd0ll4r commented Feb 3, 2023

Hello! The CI is fixed :) If you rebase this PR, we can probably merge it. Thanks again!

mrd0ll4r
mrd0ll4r previously approved these changes Feb 6, 2023
@mrd0ll4r
Copy link
Member

mrd0ll4r commented Feb 6, 2023

Sorry, I missed this earlier: Can you squash your two commits into one and give it a name of this schema:

<subsystem, e.g. storage>: <what changed>

As described here in more detail.

Sorry for missing that last time! The changes look good.

@ru-asdx ru-asdx changed the title Ref #595 - add redis-socket:// scheme for connect to redis with unix socket storage: add redis-socket scheme for connect to redis with unix socket Feb 28, 2023
@ru-asdx
Copy link
Author

ru-asdx commented Feb 28, 2023

is something wrong again?

@mrd0ll4r
Copy link
Member

Oh, sorry, this fell under the table. Changes look good, as before :) We have a data race which will probably block CI, again... really sorry.

Anyway, just leave this open, we'll fix it eventually and merge. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants