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

2023-11-03: making the Response parsing consistent #114

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

tombuildsstuff
Copy link
Owner

Now when deserializing the Response we're checking if resp and resp.Response are both != nil before trying to unmarshal.

See hashicorp/terraform-provider-azurerm#25521 for an example of where this manifests by being done inconsistently - as such doing this across the board means this'll take effect everywhere, plus be included in any new operations which are based on the existing one

Now when deserializing the Response we're checking if `resp` and `resp.Response` are both != nil before trying
to unmarshal.

See hashicorp/terraform-provider-azurerm#25521 for an example of where this manifests
by being done inconsistently - as such doing this across the board means this'll take effect everywhere, plus be
included in any new operations which are based on the existing one
Copy link
Collaborator

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

LGTM!

@tombuildsstuff tombuildsstuff force-pushed the b/making-response-parsing-consistent branch from c1db60f to 57ad29f Compare April 5, 2024 11:42
@tombuildsstuff tombuildsstuff force-pushed the b/making-response-parsing-consistent branch from 57ad29f to bbf0f64 Compare April 5, 2024 11:58
@tombuildsstuff
Copy link
Owner Author

Tests pass:

Screenshot 2024-04-05 at 14 38 17

@tombuildsstuff tombuildsstuff merged commit f7e1986 into main Apr 5, 2024
1 check passed
@tombuildsstuff tombuildsstuff deleted the b/making-response-parsing-consistent branch April 5, 2024 12:38
Copy link
Collaborator

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

spotted one import needing sorted but otherwise LGTM!

@@ -3,6 +3,8 @@ package files
import (
"context"
"fmt"
"github.com/hashicorp/go-azure-helpers/lang/pointer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

imports need sorted here

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.

2 participants