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

Couple of syntax and unmarshaling fixes. closes #232 #233

Closed
wants to merge 1 commit into from

Conversation

Erog38
Copy link

@Erog38 Erog38 commented Jul 16, 2020

This MR adds a field to the webhook struct, fixes struct tag syntax, and adjusts the payload for sending responder requests to match the documentation. Closes #232

@Erog38 Erog38 mentioned this pull request Jul 16, 2020
@bvwells bvwells mentioned this pull request Sep 12, 2020
@stmcallister
Copy link
Contributor

Thanks for making this change! And, sorry for the delay! Were you able to get it to work? I needed to also change the type of ResponderRequest.Targets to []ResponderRequestTargets, like you did in ResponderRequestOptions in order for me to use the function with out errors.

@theckman
Copy link
Collaborator

@Erog38 is this ready to go? Happy to review and look at merging if so.

@theckman theckman added the open question there is an open question on the issue / PR label Feb 20, 2021
@theckman theckman added this to the v1.4.0 milestone Feb 20, 2021
@theckman theckman modified the milestones: v1.4.0, v1.5.0 Mar 7, 2021
@theckman theckman added the breaking change This PR contains a breaking change label Mar 8, 2021
@Erog38
Copy link
Author

Erog38 commented Apr 5, 2021

My goodness, sorry about the long MR time folks! Let me spruce up a bit in here and double check a few things.

@Erog38
Copy link
Author

Erog38 commented Apr 5, 2021

@theckman @stmcallister I've added the array definition to the ResponderRequest struct. I've found several other things over the last year or so. Would you prefer a different MR from here or just review a slightly larger MR?

adding array definition to ResponderRequest

vet your code kids.
@theckman
Copy link
Collaborator

@Erog38 hey there; we're in the v1.5.0 window now so we're ready to look at getting this merged-in. I would assume what you have found overlaps with #251. Feel free to broaden this PR and we can work on getting it merged.

On a tactical note, it seems like the tests aren't passing in CI right now. If this PR is the broadened version, I'm happy to take a look once the tests are green.

@theckman
Copy link
Collaborator

theckman commented May 4, 2021

@Erog38 wanted to ping again to see if you be able to get the tests green on this one.

@theckman
Copy link
Collaborator

theckman commented May 16, 2021

I'm going to close this PR since there hasn't been an update, and there are other PRs raised to accomplish the same.

Broken struct tags were fixed in #275. The alert was already added in #276. Closing in favor of #328.

@theckman theckman closed this May 16, 2021
@theckman theckman removed this from the v1.5.0 milestone May 16, 2021
@theckman theckman removed the open question there is an open question on the issue / PR label May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR contains a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improper unmarshalling
3 participants