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

feat: allow connection to redis using uri #1011

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

iwpnd
Copy link
Member

@iwpnd iwpnd commented Nov 18, 2024

closes: #1010

@iwpnd iwpnd requested review from gdey and ARolek as code owners November 18, 2024 07:43
@coveralls
Copy link

coveralls commented Nov 18, 2024

Pull Request Test Coverage Report for Build 19f74247e-PR-1011

Details

  • 10 of 12 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 42.78%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cache/redis/redis.go 10 12 83.33%
Totals Coverage Status
Change from base Build 0f3131fbe: 0.03%
Covered Lines: 7013
Relevant Lines: 16393

💛 - Coveralls

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

Looks good, and I agree that this it's the right move to deprecate the old config properties in favor of the connection string approach. A couple of requests:

  • Please update the README for this package indicating that connection string is the favored approach.
  • What do you think about upgrading the underlying go-redis package? looks like the one we have in go.mod is pretty old.

cache/redis/redis_test.go Outdated Show resolved Hide resolved
@iwpnd
Copy link
Member Author

iwpnd commented Nov 18, 2024

I would upgrade redis in a separate PR. v9 passes contexts to commands, so some changes are required there. Will otherwise address your comments momentarily. :)

closes: go-spatial#1010

chore: resolve review comments
@iwpnd iwpnd force-pushed the feat/connect-redis-with-uri branch from d1e1bcd to bea24e1 Compare November 19, 2024 09:50
Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

Looks good!

@ARolek ARolek merged commit 93b917e into go-spatial:master Nov 19, 2024
12 checks passed
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.

add support for username for redis as cache in tegola config
3 participants