Fix: make DefaultMatcher strict

[fornellas-udemy]

Ditto.

Closes #97.

[dnaeon]

Merged, thanks!

[fornellas-udemy]

@dnaeon as this is now, it is definitely safe, but I bumped into a corner case today, that you may want to consider updating. Some libraries set the user agent header with things like GOOS and GOARCH, which means that, the fixture would have for example linux/amd64 from one machine, and the build would fail on another machine which is darwin/arm64 for example. It should be safe-ish to ignore the user agent in the default case:

    // User-Agent sometimes contain GOOS or GOARCH values, which breaks the build across different
    // machines. We ignore these to prevent this issue.
    requestHeader := maps.Clone(request.Header)
    delete(requestHeader, "User-Agent")
    cassetteRequestHeaders := maps.Clone(cassetteRequest.Headers)
    delete(cassetteRequestHeaders, "User-Agent")
    if !deepEqualContents(requestHeader, cassetteRequestHeaders) {
        return false
    }

Maybe we could have this as some sort of option? Safe by default, but trivial to tweak? WDYT?

[dnaeon]

Hey @fornellas-udemy ,

I'm okay with adding such functionality, however I don't have spare time these days to actually work on it, but would be happy to review any proposed changes.

Thanks!

[fornellas-udemy]

NP, I'll cook a PR then, TY.

[jsoriano]

It looks like this change (and/or https://github.com/dnaeon/go-vcr/pull/99) breaks existing tests, would it be possible to make this stricter mode optional, and maybe later consider making it the default in a future major?

We have found that updating from 3.2.0 to 3.2.1 (https://github.com/elastic/elastic-package/pull/2001) breaks all our tests that make use of go-vcr, what is unexpected for a patch version. Regenerating all our recorded interactions would be burdensome.

Apart of that there are header fields or part of bodies that may change between executions, for example the multipart writer in the Go stdlib includes randomly generated boundaries.

[dnaeon]

@jsoriano I've just retracted v3.2.1

https://github.com/dnaeon/go-vcr/commit/4bc3b10e78a7f43b795db1fa1937a72245357836

Users can still install v3.2.1 but for now it is retracted. Will be carving out a new minor release soon, sorry for the inconvenience.

[jsoriano]

Thanks @dnaeon for the quick answer, let me know if we can help testing the new release, vcr has helped a lot improving the reliability and speed of our builds!

[fornellas-udemy]

Apart of that there are header fields or part of bodies that may change between executions, for example the multipart writer in the Go stdlib includes randomly generated boundaries.

Hum... @jsoriano it looks like this may be another case to extend on #99, as I can see how this logic would be reusable across various projects.

[dnaeon]

Hey @fornellas-udemy and @jsoriano , I've just pushed the v4 branch. We will use that one for the next release of go-vcr.

[fornellas-udemy]

@dnaeon I see you changed v4 to make heavy use of the foo.NewFoo(foo.WithBar()...) pattern. While this works, I is objectively more complex (than a simple type Options struct for example), and will require me (and others) to push a fair amount of code refactoring to upgrade. In my cases in particular, as I had a base recorder object, which I decorated on a per-test basis with extra stuff, I'll have to update virtually everything to pile up Option functions, delay the recorder creation until the end. Doable, but I feel like it'll be a lot of code & energy, for no tangible gain. If we look at HTTP server design for example, it is just a simple object, no need for a lot of methods and complications on top.

Is this design something you're strongly attached to, or are you open to exploring options?

[dnaeon]

Hey @fornellas-udemy ,

Perhaps we can open up a separate issue and discuss this as part of the v4 release? Also, if you could provide some examples and code why do you think going with the older options might be a better choice, or not, that would be really helpful.

Thanks!