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.

Sign up for GitHub

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

Jump to bottom

Fix body reader of http backend #407

Merged
merged 5 commits into from
Apr 30, 2021
Merged

Fix body reader of http backend #407

merged 5 commits into from
Apr 30, 2021

Conversation

sundowndev
Copy link
Contributor

@sundowndev sundowndev commented Apr 6, 2021 •
edited

Q A
πŸ› Bug fix? no
πŸš€ New feature? no
⚠ Deprecations? no
❌ BC Break no
πŸ”— Related issues #...
❓ Documentation no

Description

As discussed in #406 , we should mock the http.Client instead of running real HTTP calls on the network.

This change also fixes an issue we recently discovered: the response body is always empty because a buffer was reading the body content already.

@sundowndev sundowndev added the kind/maintenance Refactoring or changes to the workspace label Apr 6, 2021
@sundowndev sundowndev requested a review from a team as a code owner April 6, 2021 16:56
@sundowndev sundowndev marked this pull request as draft April 6, 2021 16:56
@codecov
Copy link

codecov bot commented Apr 6, 2021 •
edited

Codecov Report

Merging #407 (71e7dda) into v0.8 (6db9ff1) will increase coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             v0.8     #407      +/-   ##
==========================================
+ Coverage   70.71%   70.72%   +0.01%     
==========================================
  Files         286      286              
  Lines        6446     6442       -4     
==========================================
- Hits         4558     4556       -2     
+ Misses       1517     1516       -1     
+ Partials      371      370       -1
Impacted Files Coverage Ξ”
pkg/iac/terraform/state/backend/backend.go 44.44% <0.00%> (ΓΈ)
pkg/iac/terraform/state/backend/http_reader.go 88.88% <100.00%> (+7.07%) ⬆️

@sundowndev sundowndev marked this pull request as ready for review April 7, 2021 17:07
@eliecharra eliecharra added this to the v0.8.0 milestone Apr 9, 2021
@eliecharra eliecharra added this to Review in driftctl via automation Apr 9, 2021
wbeuil
wbeuil previously approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@wbeuil wbeuil left a comment

Choose a reason for hiding this comment

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

LGTM

@sundowndev sundowndev force-pushed the test/httpBackend branch 2 times, most recently from f8c4811 to 7c4cc33 Compare April 14, 2021 10:44
@moadibfr moadibfr added this to Review in driftctl Apr 16, 2021
@eliecharra eliecharra modified the milestones: v0.8.0, v0.9.0 Apr 16, 2021
@sundowndev sundowndev changed the base branch from main to v0.8 April 28, 2021 15:06
@sundowndev sundowndev added kind/bug Something isn't working priority/0 and removed kind/maintenance Refactoring or changes to the workspace labels Apr 28, 2021
@sundowndev sundowndev self-assigned this Apr 28, 2021
@sundowndev sundowndev modified the milestones: v0.9.0, v0.8.0 Apr 28, 2021
@sundowndev sundowndev changed the title Mock http client in http backend unit tests Fix body reader of http backend Apr 28, 2021
@sundowndev sundowndev requested a review from wbeuil April 28, 2021 15:44
wbeuil
wbeuil previously approved these changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@wbeuil wbeuil left a comment

Choose a reason for hiding this comment

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

LGTM, @eliecharra @moadibfr ping on this one.

eliecharra
eliecharra suggested changes Apr 29, 2021
View reviewed changes
pkg/iac/terraform/state/backend/http_reader.go Outdated Show resolved Hide resolved
pkg/iac/terraform/state/backend/http_reader.go Outdated Show resolved Hide resolved
pkg/iac/terraform/state/backend/http_reader_test.go Outdated Show resolved Hide resolved
eliecharra
eliecharra suggested changes Apr 30, 2021
View reviewed changes
mocks/http.go Outdated Show resolved Hide resolved
@sundowndev sundowndev force-pushed the test/httpBackend branch 2 times, most recently from 5eee94e to b57fa54 Compare April 30, 2021 12:45
@sundowndev sundowndev merged commit ea749fe into v0.8 Apr 30, 2021
driftctl automation moved this from Review to Done Apr 30, 2021
@sundowndev sundowndev deleted the test/httpBackend branch April 30, 2021 13:04
This was referenced May 5, 2021
Closed
Merged
@sundowndev sundowndev moved this from Review to Done in driftctl May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eliecharra eliecharra eliecharra approved these changes

@wbeuil wbeuil wbeuil left review comments

Assignees

@sundowndev sundowndev

Labels
kind/bug Something isn't working
Projects
No open projects
driftctl
Done
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sundowndev @eliecharra @wbeuil