-
Notifications
You must be signed in to change notification settings - Fork 66
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 VCR gem #3583
Update VCR gem #3583
Conversation
Two major versions in one go? No version pinning in Could you provide some additional context, given there's no logic/test changes? This approach to dependency upgrade seems a bit dubious to me - how can we be certain there aren't any significant breakages? What features do we gain? I'm in general supportive of getting up-to-date for our libraries, just curious 😸 |
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.
I'm curious as well as to what we gain from the major version upgrades and what has changed. Possibly include a link in the PR description 😃
Copy pasted here for the lazy:
|
Might I recommend that you record (or re-record) a new cassette just as a test for added assurance that all is well. Once that's done it LGTM. |
Thanks for the changelog link - looks pretty minimal in terms of risk! +1 to the "record a cassette" smoke test, then I'm happy to ✔️ |
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.
+1 on a new cassette recording. Could you link to the changelog in the PR description too (looks like bill did in a comment). After that, ✔️
Updated this PR with a re-recorded Yes, the body response is now ASCII-binary-encoded, but that's actually how it should be. The readable JSON string that did exist was manually entered, by me & Harry when this service was created. It was mostly for reference/FE use but we shouldn't need to be looking at cassette contents directly IMO. We could write a custom serializer but I don't see the point. @johnpaulashenfelter will leave up to you if you want to merge or not w/ the binary-encoded string. |
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. See comment above.
Description of change
We are pretty far behind (5 v 3.0.3) but the upgrade seemed pretty seemless. All the specs still work so cassettes are running properly.
Testing done
Testing planned
Acceptance Criteria (Definition of Done)
Unique to this PR
Applies to all PRs
Appropriate loggingSwagger docs have been updated, if applicableProvide link to originating GitHub issue, or connected to it via ZenHubProvide which alerts would indicate a problem with this functionality (if applicable)