-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix s3 example URLs in the artifacts docs #15123
Conversation
Unfortunately, s3 urls prefixed with https:// do NOT work with the underlying go-getter library. As such, this fixes the examples so that they are working examples that won't cause problems for people reading the docs. See discussion in #1113 circa 2016.
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 fix @twunderlich-grapl!
Per your findings and existing documentation ("non-Amazon S3-compatible endpoints like Minio are supported, but you must explicitly set the "s3::" prefix.") using the explicit s3://
protocol may be the example that is most likely to work in different scenarios.
How do you feel about setting it in the docs?
@@ -210,7 +210,7 @@ This example uses path-based notation on a publicly-accessible bucket: | |||
|
|||
```hcl | |||
artifact { | |||
source = "https://my-bucket-example.s3-us-west-2.amazonaws.com/my_app.tar.gz" | |||
source = "my-bucket-example.s3-us-west-2.amazonaws.com/my_app.tar.gz" |
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.
source = "my-bucket-example.s3-us-west-2.amazonaws.com/my_app.tar.gz" | |
source = "s3://my-bucket-example.s3-us-west-2.amazonaws.com/my_app.tar.gz" |
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've updated this in a follow-up commit
@@ -240,7 +240,7 @@ Alternatively you can use virtual hosted style: | |||
|
|||
```hcl | |||
artifact { | |||
source = "https://my-bucket-example.s3-eu-west-1.amazonaws.com/my_app.tar.gz" | |||
source = "my-bucket-example.s3-eu-west-1.amazonaws.com/my_app.tar.gz" |
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.
source = "my-bucket-example.s3-eu-west-1.amazonaws.com/my_app.tar.gz" | |
source = "s3://my-bucket-example.s3-eu-west-1.amazonaws.com/my_app.tar.gz" |
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've updated this in a follow-up commit
Per the discussion in #15123, we're going to use the explicit s3 protocol in the examples since that is the likeliest to work in all scenarios
@lgfa29, |
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.
Awesome, thanks!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Unfortunately, s3 urls prefixed with https:// do NOT work with the underlying go-getter library. As such, this fixes the examples so that they are working examples that won't cause problems for people reading the docs.
This is a known limitation of the go-getter lib since at least 2016 per #1113
I've also put up a PR with the underlying go-getter lib to fix their docs as well: hashicorp/go-getter#394