-
Notifications
You must be signed in to change notification settings - Fork 383
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
fix(cache): Nil panic from elasticcache new endpoint #1849
fix(cache): Nil panic from elasticcache new endpoint #1849
Conversation
d500748
to
8ba21e9
Compare
LGTM. Thank you very much @dee0. Can you solve the conflicts and squash your changes into a single commit? |
18ff591
to
210840a
Compare
Hey @MisterMX Squashed my commits and merged the latest master into my branch to resolve the merge conflict. While the merge conflicts are one and there is just 1 commit from me showing, the PR is saying there are 11 commits total. Think Github may be confused. :) In any case, please let me know if there is something more for me to do. |
@dee0 thanks for doing the fix. It is generally not recommended to use To provide a clean history (only a single commit) can you do a clean rebase so you only have a single commit left? You can drop the other commits in an interactive rebase using I hope that isn't too inconvenient to you, but I would like to keep the upstream repo as clean as possible. |
Hey @MisterMX No worries I'll try to sort out. |
f6d9d9f
to
6d03003
Compare
6d03003
to
fd1cd2e
Compare
Hey @MisterMX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you very much @dee0!
Description of your changes
Add a test case that replicates issue #1838 and a fix for same.
NOTE:
Test case is a little different from others because I wanted the mock to replicate as closely as possible the we saw from AWS when we encountered #1838. That is, I didn't want to fix the panic we saw only to find out there were other problems we weren't aware of. Anyhoo, this meant setting some fields in the mock that no other Observe tests are setting.
Fixes #1838
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
I added a test case that reproduces the issue to the Observe test.
I made my change and confirmed that my new test case passed.
I then confirmed that all existing cache tests still passed.