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

Update go-redis to v8.11.5 #424

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Conversation

johanoskarsson
Copy link

Brings the go-redis package up to the latest version.

Ran the test that uses redis: Test_BuildContainerFromDockerfile locally and it passes.
Side note: when I try to run all the tests in the repo on main I get failures. Not sure if that has to do with my local setup or if main is broken?

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #424 (d1b08e1) into main (7d22247) will decrease coverage by 0.48%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
- Coverage   70.16%   69.67%   -0.49%     
==========================================
  Files          21       21              
  Lines        2048     2048              
==========================================
- Hits         1437     1427      -10     
- Misses        490      497       +7     
- Partials      121      124       +3     
Impacted Files Coverage Δ
docker.go 71.12% <0.00%> (-1.07%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@johanoskarsson
Copy link
Author

Looked at the failing test but I'm not sure it's related to this PR?

@johanoskarsson
Copy link
Author

This should be ready for a re-review now. Thanks!

@mdelapenya
Copy link
Collaborator

HI @johanoskarsson I did not review it on purpose as we have to internally discuss about modules, and because we are adding a dependency on redis to downstream users, I'd like to maybe move that dependency (and its tests) to an eventual Redis module.

Of course, because this change is totally harmless, it can make it to the next release (hopefully super soon)

@mdelapenya mdelapenya added the dependencies Dependencies or external services label Aug 29, 2022
@mdelapenya mdelapenya self-assigned this Aug 29, 2022
@@ -1024,7 +1024,7 @@ func Test_BuildContainerFromDockerfile(t *testing.T) {
})

t.Log("pinging redis")
pong, err := client.Ping().Result()
pong, err := client.Ping(ctx).Result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm it's weird the the docs are updated since 25fc53e

Docs/Web: https://golang.testcontainers.org/examples/redis/

Copy link
Collaborator

@mdelapenya mdelapenya 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 for taking the time to add this contribution! Hopefully it will make it soon to the next release

@mdelapenya mdelapenya merged commit 097f9af into testcontainers:main Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Dependencies or external services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants