Skip to content

Remove double-unmarshalling#1046

Merged
jacobbednarz merged 6 commits intocloudflare:masterfrom
zuplo:master
Aug 19, 2022
Merged

Remove double-unmarshalling#1046
jacobbednarz merged 6 commits intocloudflare:masterfrom
zuplo:master

Conversation

@vazexqi
Copy link
Copy Markdown

@vazexqi vazexqi commented Aug 16, 2022

Description

Fixes #1045

Has your change been tested?

Modified existing tests and ran go test.

Screenshots (if appropriate):

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

@vazexqi vazexqi requested a review from jacobbednarz as a code owner August 16, 2022 19:41
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 16, 2022

changelog detected ✅

Comment thread tunnel.go
}

uri := fmt.Sprintf("/accounts/%s/cfd_tunnel/%s/configurations", rc.Identifier, params.TunnelID)
res, err := api.makeRequestContext(ctx, http.MethodPut, uri, params.Config)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is a slightly different bug but also has to do with the marshalling/unmarshalling of the config param.

@vazexqi
Copy link
Copy Markdown
Author

vazexqi commented Aug 16, 2022

I am new to this process so let me know if there's anything that I'm missing.
In the mean time, I am using my forked version at https://github.com/zuplo/cloudflare-go to unblock myself.

@jacobbednarz
Copy link
Copy Markdown
Contributor

let me confirm with the service team as this was the correct API response when it was added.

Comment thread tunnel.go Outdated
@@ -73,9 +73,9 @@ type TunnelConnectionResponse struct {
// other than in the HTTP response unmarshaling. Use `TunnelConfigurationResult`
// for the correct types.
type TunnelConfigurationStringifiedConfigResult struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be renamed now to better reflect it's use

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think it can be removed now (see my latest PR).

I think this is OK. It's not safe since it can be breaking because TunnelConfigurationStringifiedConfigResult was an exported type before in the SDK but I doubt anyone was directly referencing those types.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 17, 2022

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.94%. Comparing base (6c5ea4a) to head (87eb8f9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1046      +/-   ##
==========================================
+ Coverage   49.06%   49.94%   +0.88%     
==========================================
  Files         108      115       +7     
  Lines       10428    10991     +563     
==========================================
+ Hits         5116     5490     +374     
- Misses       4200     4338     +138     
- Partials     1112     1163      +51     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jacobbednarz
Copy link
Copy Markdown
Contributor

this still needs a CHANGELOG too

Comment thread .changelog/1046.txt Outdated
Co-authored-by: Jacob Bednarz <jacob.bednarz@hey.com>
@jacobbednarz jacobbednarz merged commit 31436d7 into cloudflare:master Aug 19, 2022
@github-actions github-actions Bot added this to the v0.48.0 milestone Aug 19, 2022
@jacobbednarz
Copy link
Copy Markdown
Contributor

thanks for getting this over the line 🎉

github-actions Bot pushed a commit that referenced this pull request Aug 19, 2022
@vazexqi
Copy link
Copy Markdown
Author

vazexqi commented Aug 19, 2022

@jacobbednarz - Thanks for helping with this PR. Do you know when you will cut a new release so that I can switch over to the official version instead of my fork?

@jacobbednarz
Copy link
Copy Markdown
Contributor

details in https://github.com/cloudflare/cloudflare-go/blob/master/docs/release-process.md.

you can also check the milestone that this PR was added to.

@github-actions
Copy link
Copy Markdown

This functionality has been released in v0.48.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Double-unmarshalling of config from cfd_tunnel/{{TUNNEL_ID}}/configurations is unnecessary

3 participants