-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feat: add http datasource #11658
feat: add http datasource #11658
Conversation
@nywilken Looks like this needs Vercel permission, is that something you can please grant so CI can run. If there is something I need to run on my end please let me know. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening the PR! I think is looking good so far, but I miss the documentation. Could add it, please?
The documentation for this would be placed in website/content/docs/datasources
, you will also have to add them to the navigation in website/data/docs-nav-data.json
.
|
||
// Most of this code comes from http terraform provider data source | ||
// https://github.com/hashicorp/terraform-provider-http/blob/main/internal/provider/data_source.go | ||
func (d *Datasource) Execute() (cty.Value, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The returned error message can be improved a little bit to help understand the step it failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some print statements before throwing errors to make them more traceable. But I assume we still want to throw current error up the chain.
@@ -0,0 +1,5 @@ | |||
<!-- Code generated from the comments of the Config struct in datasource/http/data.go; DO NOT EDIT MANUALLY --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use these generated partials in the docs. See the other datasources documentation as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR with docs
var testDatasource404Url string | ||
|
||
func TestHttpDataSource(t *testing.T) { | ||
tests := []struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 😄
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
… http-data-resource
@sylviamoss Updated PR based on feedback. Please let me know if I should update anything else. |
@@ -0,0 +1,5 @@ | |||
<!-- Code generated from the comments of the Config struct in datasource/http/data.go; DO NOT EDIT MANUALLY --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't find where this one is coming from. Does the CI fail if you remove it? I think it was generated the first time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try to remove it and see what happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like website changes passed with removing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes! I made some suggestions on the docs.
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
Co-authored-by: Sylvia Moss <sylviamoss.m@gmail.com>
Updated docs with suggestions (thanks again for suggestions, writing better docs is something I need to improve on). If there's anything else I should change or if you want to to rebase to clean up commit history please let me know. |
@sylviamoss It looks like the vercel deployment failed but it's unclear why, any ideas? |
@@ -0,0 +1,49 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build is failing because the file type should be mdx
.
The best place to put this doc is directly under the datasources
folder, as datasources/http.mdx
because it's a single datasource type.
website/data/docs-nav-data.json
Outdated
@@ -712,6 +712,10 @@ | |||
"title": "Image", | |||
"path": "datasources/hcp/hcp-packer-image" | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, you put the datasource under the HCP datasources. You need to move this one up and update the path to accordingly to the changes I proposed above datasources/http
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patience! I've been focused on other stuff in parallel and sometimes it takes longer to switch context
To test your website changes locally, you can do
It requires docker running. |
@sylviamoss Thanks again for all the reviews.I think I addressed issues you mentioned and the container builds and runs locally for me. Please let me know if there's anything else I should change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍🏼 LGTM
req, err := http.NewRequestWithContext(ctx, "GET", url, nil) | ||
// TODO: How to make a test case for this? | ||
if err != nil { | ||
fmt.Println("Error creating http request") | ||
return cty.NullVal(cty.EmptyObject), err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shouldn't be here, don't tell anyone, but the httptest
library should help here: https://pkg.go.dev/net/http/httptest#NewServer, you can make it return actual data without starting a server, I think.
Also, NewRequestWithContext
uses a global client, and I'd recommend on having a more local one.
🥷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make a PR with this update. Thanks!
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. |
Description
Creating http datasource similar to terraform http data source.
Testing
Commands to run tests:
make PACKER_ACC=1 go test -count 1 -v ./datasource/http/data_acc_test.go -timeout=2m
Closes #11552