Skip to content

Add support for AWS_REGION env variable#6

Open
Bernardstanislas wants to merge 1 commit intopaulhammond:mainfrom
Bernardstanislas:add-region-support
Open

Add support for AWS_REGION env variable#6
Bernardstanislas wants to merge 1 commit intopaulhammond:mainfrom
Bernardstanislas:add-region-support

Conversation

@Bernardstanislas
Copy link
Copy Markdown

I was facing some HTTP code 307 redirects when using the scripts, so I added the possibility to use a AWS_REGION environment variable.

@Bernardstanislas
Copy link
Copy Markdown
Author

@paulhammond do you accept this change?

Copy link
Copy Markdown
Owner

@paulhammond paulhammond left a comment

Choose a reason for hiding this comment

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

Hi! I like the change in general. I left a few nitpicky suggestions that I'd want to change.

In general I prefer atomic commits that change just one thing - this commit includes other formatting changes. I've just pushed a change to main so all scripts are formatted with shfmt so you shouldn't need to fix the formatting in unrelated lines any more.

If you want to rebase this change on main and make the changes I'll merge it in. If that's too much work I'll make a commit myself in a few days and credit you in the message. Whatever is best for you!

export AWS_SECRET_ACCESS_KEY=zzzz
# optionally provide a temporary session token
export AWS_SESSION_TOKEN=wwww...
# optionally specify the AWS region to avoid 307 redirects
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
# optionally specify the AWS region to avoid 307 redirects
# optionally specify the AWS region as an optimization

Comment on lines +75 to +82
local url
if [ z "${AWS_REGION-}" ]; then
url="s3-${AWS_REGION}.amazonaws.com"
else
url="s3.amazonaws.com"
fi

curl "${args[@]}" -s -f -H "Date: ${date}" -H "Authorization: ${authorization}" "https://${bucket}.${url}/${key}"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
local url
if [ z "${AWS_REGION-}" ]; then
url="s3-${AWS_REGION}.amazonaws.com"
else
url="s3.amazonaws.com"
fi
curl "${args[@]}" -s -f -H "Date: ${date}" -H "Authorization: ${authorization}" "https://${bucket}.${url}/${key}"
local domain
if [ z "${AWS_REGION-}" ]; then
domain="s3-${AWS_REGION}.amazonaws.com"
else
domain="s3.amazonaws.com"
fi
curl "${args[@]}" -s -f -H "Date: ${date}" -H "Authorization: ${authorization}" "https://${bucket}.${domain}/${key}"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think domain would be a better name for this variable - the whole thing is a URL, this is a component of the hostname so domain seems to make the most sense to me

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