-
Notifications
You must be signed in to change notification settings - Fork 89
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
Upgrade go-vcr to v3 #309
Upgrade go-vcr to v3 #309
Conversation
143f49c
to
03baf19
Compare
Codecov Report
@@ Coverage Diff @@
## main #309 +/- ##
==========================================
+ Coverage 85.39% 85.68% +0.29%
==========================================
Files 39 39
Lines 8276 8299 +23
==========================================
+ Hits 7067 7111 +44
+ Misses 956 935 -21
Partials 253 253
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
03baf19
to
06cd6bf
Compare
This is only used as a dev dependency I presume, just reading go-vcr 3.1.0 which has breaking changes in their API. Not ideal in a minor. |
@cocojoe Indeed, this is only an internal dev dependency used for testing. As we upgraded we also had to regenerate all the http interaction mocks. |
internal/recorder/http_recorder.go
Outdated
delete(i.Request.Headers, header) | ||
} | ||
for _, header := range responseHeaders { | ||
delete(i.Response.Headers, header) |
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.
Do we need any particular header, or can all be removed?
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 think just the content-type and the user agent are useful for us.
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.
Maybe we could go with an allow-list of just those?
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.
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.
Leaving a few comments for posterity, didn't leave initially as we reviewed this in a zoom call together with @willvedd
@@ -24,7 +25,7 @@ data auth0_client test { | |||
` | |||
|
|||
func TestAccDataClientByName(t *testing.T) { | |||
httpRecorder := configureHTTPRecorder(t) | |||
httpRecorder := recorder.New(t) |
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.
We are now wrapping the go-vcr recorder into our own type Recorder within it's own package, so it's easier to swap with something else or perform upgrades.
@@ -46,7 +48,11 @@ func init() { | |||
} | |||
|
|||
func TestAccCustomDomain(t *testing.T) { | |||
httpRecorder := configureHTTPRecorder(t) | |||
if os.Getenv("AUTH0_DOMAIN") != recorder.RecordingsDomain { |
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.
These should never run without the recordings.
@@ -288,7 +292,7 @@ func TestAccLogStreamEventGrid(t *testing.T) { | |||
resource.TestCheckResourceAttr("auth0_log_stream.my_log_stream", "name", fmt.Sprintf("Acceptance-Test-LogStream-azure-%s", t.Name())), | |||
resource.TestCheckResourceAttr("auth0_log_stream.my_log_stream", "type", "eventgrid"), | |||
resource.TestCheckResourceAttr("auth0_log_stream.my_log_stream", "sink.0.azure_subscription_id", "b69a6835-57c7-4d53-b0d5-1c6ae580b6d5"), | |||
resource.TestCheckResourceAttr("auth0_log_stream.my_log_stream", "sink.0.azure_region", "northeurope"), | |||
resource.TestCheckResourceAttr("auth0_log_stream.my_log_stream", "sink.0.azure_region", "westeurope"), |
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.
We also added an interaction for this test and we stop skipping it always. After adding it it was found that the assertion was wrong so this corrects it.
@@ -1,81 +1,182 @@ | |||
--- | |||
version: 1 | |||
version: 2 |
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.
Unfortunately we had to re-record everything because go-vcr 3 uses a different format for the cassettes.
🔧 Changes
Upgrade go-vcr to v3. This is a dev dependency used only in testing to help us with pre-recorded http interactions.
📚 References
🔬 Testing
📝 Checklist