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.

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 request/response logging for SetDoNotParseResponse(true) #836

Merged
merged 6 commits into from
Sep 2, 2024

Conversation

kon3gor
Copy link
Contributor

@kon3gor kon3gor commented Aug 29, 2024

Bug fix for #829

Changes

  • responseLogger is not middleware anymore for consistency reasons
  • responseLogger is now being called whenever execute exits
  • Parameter name in SetDoNotParseResponse has been changed to notParse

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@kon3gor Thanks for creating a PR. The code updates LGTM, can you please check test failures and code conflicts?

@kon3gor
Copy link
Contributor Author

kon3gor commented Aug 30, 2024

@jeevatkm Tests and merge conflicts are fixed now🫡

@kon3gor kon3gor requested a review from jeevatkm August 30, 2024 16:40
@jeevatkm
Copy link
Member

@kon3gor Thanks for taking care of the tests. It seems the errors.Join method is causing a failure on tests. Since it's added in go1.20, but resty supports go min version 1.19

Please see https://github.com/go-resty/resty/pull/836/checks

@kon3gor
Copy link
Contributor Author

kon3gor commented Aug 31, 2024

@jeevatkm Hi! I've removed erros.Join and replaced it with the self-made replica. Also made some logic fixes for logging logging errors.

@kon3gor
Copy link
Contributor Author

kon3gor commented Aug 31, 2024

@jeevatkm Looks like errors.Is logic has been changed for joinError support. In go1.19 there was no Unwrap []error interface, that why my custom joinErr doesn't work. I'll change logic a bit to match this old behaviour

@jeevatkm
Copy link
Member

@kon3gor Okay, please let me know when you are done.

Also, please add attribution to the code for the method Join.

// Code obtained from `golang:src/errors/join.go` to backfill until Resty go minimum version gets updated to go1.20
// TODO: Remove this Join-related code.

@jeevatkm
Copy link
Member

jeevatkm commented Sep 1, 2024

@kon3gor I was giving it a second thought on the go minimum version. Let's update the go minimum version to go1.20.

Please update go.mod and GitHub CI actions files. Then, you can work on the logic update in line with the go1.20 and above version.

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.85%. Comparing base (c22b968) to head (72da0db).
Report is 1 commits behind head on v2.

Files with missing lines Patch % Lines
client.go 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v2     #836      +/-   ##
==========================================
- Coverage   96.89%   96.85%   -0.05%     
==========================================
  Files          14       14              
  Lines        1773     1780       +7     
==========================================
+ Hits         1718     1724       +6     
- Misses         35       36       +1     
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

@kon3gor Thanks for updating PR and test cases.

@jeevatkm jeevatkm merged commit 08e6170 into go-resty:v2 Sep 2, 2024
3 of 4 checks passed
renovate bot added a commit to Michsior14/transmission-gluetun-port-update that referenced this pull request Sep 15, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/go-resty/resty/v2](https://redirect.github.com/go-resty/resty)
| `v2.14.0` -> `v2.15.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgo-resty%2fresty%2fv2/v2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fgo-resty%2fresty%2fv2/v2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fgo-resty%2fresty%2fv2/v2.14.0/v2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgo-resty%2fresty%2fv2/v2.14.0/v2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>go-resty/resty (github.com/go-resty/resty/v2)</summary>

###
[`v2.15.0`](https://redirect.github.com/go-resty/resty/releases/tag/v2.15.0)

[Compare
Source](https://redirect.github.com/go-resty/resty/compare/v2.14.0...v2.15.0)

### Release Notes

#### New Features

- feat: add max reponse body limit by
[@&#8203;trim21](https://redirect.github.com/trim21) in
[go-resty/resty#830
- feat: add SetClientRootCertificate method support clientCAs usage by
[@&#8203;MagHErmit](https://redirect.github.com/MagHErmit) in
[go-resty/resty#826

#### Enhancements

- feat: add ability to set custom multipart boundary value by
[@&#8203;PokeGuys](https://redirect.github.com/PokeGuys) in
[go-resty/resty#820
- feat(refactor): for pr
[#&#8203;826](https://redirect.github.com/go-resty/resty/issues/826) by
[@&#8203;jeevatkm](https://redirect.github.com/jeevatkm) in
[go-resty/resty#848

#### Bug Fixes

- Fix request/response logging for SetDoNotParseResponse(true) by
[@&#8203;kon3gor](https://redirect.github.com/kon3gor) in
[go-resty/resty#836
- fix(enhancement): add explicit option to enable generate curl command
in conjunction with debug mode and few clean ups
[#&#8203;828](https://redirect.github.com/go-resty/resty/issues/828) by
[@&#8203;jeevatkm](https://redirect.github.com/jeevatkm) in
[go-resty/resty#842

#### Build

- feat(enhancement): update bazel config by
[@&#8203;frank30941](https://redirect.github.com/frank30941) in
[go-resty/resty#833
- build: go min version and build config update
[#&#8203;835](https://redirect.github.com/go-resty/resty/issues/835) by
[@&#8203;jeevatkm](https://redirect.github.com/jeevatkm) in
[go-resty/resty#837
- Update go vesion to 1.20 by
[@&#8203;kon3gor](https://redirect.github.com/kon3gor) in
[go-resty/resty#841

#### Documentation

- doc: godoc improvements and corrections by
[@&#8203;jeevatkm](https://redirect.github.com/jeevatkm) in
[go-resty/resty#849,
[go-resty/resty#851
- doc(readme): Add a note, GenerateCurlCommand by
[@&#8203;ahuigo](https://redirect.github.com/ahuigo) in
[go-resty/resty#817
- release: version bump and readme update for v2.15.0 by
[@&#8203;jeevatkm](https://redirect.github.com/jeevatkm) in
[go-resty/resty#852

#### New Contributors

- [@&#8203;PokeGuys](https://redirect.github.com/PokeGuys) made their
first contribution in
[go-resty/resty#820
- [@&#8203;trim21](https://redirect.github.com/trim21) made their first
contribution in
[go-resty/resty#830
- [@&#8203;frank30941](https://redirect.github.com/frank30941) made
their first contribution in
[go-resty/resty#833
- [@&#8203;kon3gor](https://redirect.github.com/kon3gor) made their
first contribution in
[go-resty/resty#841
- [@&#8203;MagHErmit](https://redirect.github.com/MagHErmit) made their
first contribution in
[go-resty/resty#826

**Full Changelog**:
go-resty/resty@v2.14.0...v2.15.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/Michsior14/transmission-gluetun-port-update).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguNzQuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants