feat: allow additional methods in http datasource #13190
Conversation
|
|
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
There was a problem hiding this comment.
Hi @hegerdes,
Thanks for the PR! I left a few comments, most are nits tbh, the code looks good to me overall.
The main thing I'd like to see addressed one way or another in this would be the methods supported, I believe I understand the rationale to limit it to a subset of what HTTP supports, that said I worry this might block some users unnecessarily, so I propose relaxing this check so all methods are accepted.
It's open for discussion of course, please feel free to push-back if you think I'm being too cautious here.
Thanks!
datasource/http/data.go
Outdated
|
|
||
| // Check if the input is in the list of allowed methods | ||
| validMethod := false | ||
| allowedMethods := []string{"HEAD", "GET", "POST", "PUT"} |
There was a problem hiding this comment.
Is there a reason why we support a subset of the methods allowed by HTTP? Notably PATCH is missing, and a bunch of others as well.
To be fair most of the methods won't be used with this datasource I'd think (typically HEAD or OPTIONS), but I wonder if we shouldn't relax this limitation so we don't hinder valid use-cases because of a misplaced assumption.
There was a problem hiding this comment.
I limited it because it is limited in terraform (there even PUT is not allowed. I'm fully behind being more open, but there still should be some validation. Should we allow all HTTP methods, including CONNECT and TRACE from the standard? I don't know if these have to be handled diffferently.
This adds support for additional http methods for the http datasource. Fixes hashicorp#13169 Signed-off-by: Henrik Gerdes <[email protected]>
hegerdes
force-pushed
the
feat/extra-http-methods
branch
from
c2a75ff to
e858b68
Compare
hegerdes
changed the title
|
Squashed the previous commits and fixed the expected text output. Also updated the generated docs, forgot this the last time. |
There was a problem hiding this comment.
LGTM in the rerolled state, thanks for persevering with this change @hegerdes !
Merging now if tests are green
|
Only Vercel's failing so merging since it's not relevant to the PR |
|
Thanks for merging and taking a look at this at in such a quick time 😊 Awesome contributing experience! I noted that the windows test in the main branch failed. Is this caused by my changes? |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
What changed
This adds support for additional
httpmethods for thehttp datasource.I strongly orientated myself on the terrafrom http-datasource. With this PR the following methods are allowed:
HEAD,GET,POST,PUT,DELETE,OPTIONS,PATCHIf this list should be extended or reduced let me know.
Tests
I added one test to check for invalid methods, but there should probably be more. The problem is HTTP tests rely on the internet and it can be unstable. I was thinking of putting a
POSTtest in forhttps://postman-echo.com/postbut I don't know how hashicorp thinks about "dependencies" to external websitesOther
I am a total go nooby. I want to get more in to it and this seemed a good opportunity. I tried to follow existing style but I'm still lerning. If things should be done different, please tell me.
If your PR resolves any open issue(s), please indicate them like this so they will be closed when your PR is merged:
Closes #13169
EDIT1: Updated allowed HTTP methods