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

allow setting of cache control header on s3 text file #33

Merged
merged 2 commits into from
Jul 11, 2019

Conversation

jpslav
Copy link
Member

@jpslav jpslav commented Jul 11, 2019

allow setting of cache control header on s3 text file

@jpslav jpslav requested a review from steveburkett July 11, 2019 21:02
expect(instance.read).to eq "something new"
end
end

it 'sets the cache control header' do
with_temp_bucket(files: [{key: key, path: test_file_path}]) do
instance = described_class.new(bucket_name: bucket_name, bucket_region: region, key: key)
Copy link
Contributor

Choose a reason for hiding this comment

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

optional of course. this could be cleaned up a bit via a subject http://www.betterspecs.org/#subject

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a let

body: StringIO.new(string_contents)
}

args[:content_type] = content_type if !content_type.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

args[:content_type] = content_type if content_type ? (a bit more concise)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... for some reason I generally don't do that. Some sort of long-lasting objection to boolean checks on non boolean things in C++ I think :-)

@jpslav jpslav merged commit 9f63172 into master Jul 11, 2019
@jpslav jpslav deleted the s3-text-file-set-cache-control branch July 11, 2019 21:22
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