Add Upload to S3 Action

[jkmassel]

Adds an "upload to S3" action inside the release toolkit that's useful for binary storage (particularly for installable builds).

To Test:
Note that woocommerce/woocommerce-android#5862 passes and properly displays a comment with a download link – the upload functionality for this is powered by this new action.

[AliSoftware]

Why not use https://github.com/fastlane-community/fastlane-plugin-s3 instead of reimplementing our own?!

[AliSoftware]

Given SharedValues is a wide and quite global namespace I'd suggest use a more specific constant name here.

Suggested change

	UPLOADED_FILE_PATH = :UPLOADED_FILE_PATH
	S3_UPLOADED_FILE_PATH = :S3_UPLOADED_FILE_PATH

[jkmassel]

Addressed in 1dcc36b to also update its usage.

[jkmassel]

Why not use https://github.com/fastlane-community/fastlane-plugin-s3 instead of reimplementing our own?!

Good question – I'd be fine to use it if we could (though I'm curious whether it'd make more sense to have this plugin depend on that one, or just use both in the top-level app – I don't feel strongly either way).

I came across this issue which I believe is a blocker, but I'll try it out in a branch and see if it's actually been resolved already (as the thread indicates it might have been).

[jkmassel]

@AliSoftware – I took another look at using the built-in S3 plugin, and there are a few other issues (from what I can see):

• By default it'll read the result of gradle or gym and upload the generated file – this isn't necessarily what we want, and I don't see a way to opt out of it.
• Relatedly, when we upload an apk this plugin will generate (what I assume is) a manifest for the binary and an HTML page to download it. We can disable this, but it's the default, and I'm not sure I want to constantly have to go "against the grain" for simple functionality like this.
• We can provide a files list for it to upload, but not a list of keys to use for those files, which given the open nature of the artifacts bucket makes the URLs guessable (which is not good).

[jkmassel]

Rebased to fix merge conflicts.

[mokagio]

Sad that there's no tests for this action, but there's not much to do when the bulk of the logic is around a third party dependency.

Well, actually, it looks like the AWS SDK has stubbing methods for testing. It might be worth checking them out, but definitely not worth blocking this PR for me, also given this is required the Woo Android installable builds.

I implicitly verified this code's happy path in woocommerce/woocommerce-android#5862. Approving.

[mokagio]

Indentation nitpick.

Suggested change

	Aws::S3::Client.new().put_object({
	body: file,
	bucket: bucket,
	key: key
	})
	Aws::S3::Client.new().put_object(
	{
	body: file,
	bucket: bucket,
	key: key
	}
	)

[mokagio]

Or, more compact:

Suggested change

	Aws::S3::Client.new().put_object({
	body: file,
	bucket: bucket,
	key: key
	})
	Aws::S3::Client.new().put_object({ body: file, bucket: bucket, key: key })

[AliSoftware]

I even think, given how Ruby works and interprets named parameters at call site vs Hashes at def site, that you should be able to do:

Suggested change

	Aws::S3::Client.new().put_object({
	body: file,
	bucket: bucket,
	key: key
	})
	Aws::S3::Client.new().put_object(body: file, bucket: bucket, key: key)

At least that's how most of those typical Ruby client implementations (e.g. Octokit for GitHub API client) expect you to call those methods anyway 🙂

[jkmassel]

Yep – it's funny, I reviewed another PR and suggested dropping the hash syntax, but somehow missed it here. I've simplified it to just the regular named parameter syntax (though I retained the multi-line call in case of future changes).

[mokagio]

Is the wrapping in ( ) necessary?

[AliSoftware]

☝️

[jkmassel]

Probably not! 😅

Removed in ab376e7

[AliSoftware]

Left a couple of style nitpicks, otherwise seems to be working.
I still think unit tests would be quite valuable to that action, despite it interacting with a 3rd party service.

We can stub_requests or make use of double / instance_double and similar to easily stub the S3 side of things, and focus the tests on the behavior at parsing and interpreting the input ConfigItems params and checking that it calls put_object with the expected parameters as a result.

For example, writing a test that calls run_described_fastlane_action with a nil key, and checks that it would either auto-generate a key for us, or crash with a user_error! if that is not permitted (I actually have no idea which of the 2 behaviors would be the correct one for that case, hence using that as a good example for a spec 😛 )

That being said, if that is a blocker, we could also decide to merge this PR without the tests in order to unblock the migration of remaining app projects to Buildkite, and then implement the tests in a subsequent PR, but I still think we should implement those tests at some point.

[AliSoftware]

I even think, given how Ruby works and interprets named parameters at call site vs Hashes at def site, that you should be able to do:

Suggested change

	Aws::S3::Client.new().put_object({
	body: file,
	bucket: bucket,
	key: key
	})
	Aws::S3::Client.new().put_object(body: file, bucket: bucket, key: key)

At least that's how most of those typical Ruby client implementations (e.g. Octokit for GitHub API client) expect you to call those methods anyway 🙂

[AliSoftware]

☝️

[AliSoftware]

Optional? 🤔 What happens if we don't provide a key parameter to that one?

[jkmassel]

Good q – addressed in the testing PR, but now it'll emit an error.

[AliSoftware]

You can move this to the verify_block of the ConfigItem (see this suggestion) to let Fastlane report the error earlier in the process while parsing args (without having to create a Runner for nothing)

[jkmassel]

Done in e8748ee, then extended the pattern to other validation in 684fad4

[AliSoftware]

Actually found a bug while giving a try at implementing Unit Tests in #340 😉

[AliSoftware]

Note: the Base64 alphabet happens to contain the / character. Will that risk causing an issue or result in misleading or strange cases?

For example if the input file is named input_file_1 then the base64 digest of its SHA1 is k5w5OY2yQF55HiBXeP9w+F3/Yg4= with a / in the middle. Worse, I imagine there could be input file names that lead to a base64 SHA1 to end with a /, which would lead to a key containing two consecutive / once we assemble that prefix with the filename, and I can imagine a key with foo//bar to lead to errors with S3…

Discovered that use case by implementing Unit Tests for this in #340 btw… so they are really useful (in case that was still needed to be proven) 😛

I suggest we should use hexdigest instead of base64digest to avoid such situations:

Suggested change

	file_name_hash = Digest::SHA1.base64digest(file_name)
	file_name_hash = Digest::SHA1.hexdigest(file_name)

[jkmassel]

what's the result of having a / in that prefixed key?

If that happens, it'll just be nested in "directories" within S3. Doesn't hurt anything, but there's a possibility of a crash if the string ends with /, so the hexdigest is for sure the way to go 🙂

[jkmassel]

@AliSoftware – I'll wait to ensure your concerns are addressed before merging if that's ok?

[AliSoftware]

Overall LGTM by skimming the new code 👍 but left a couple of suggestions to improve the wording, error messages, and code readability.

I'll let you address those nits during your day, and review the PR again first thing on my tomorrow — at which point I expect we should finally be all good to go 🤞

[AliSoftware]

I think it would be both simpler (and DRY) and more readable to merge those two helpers in a single one, with a file name generated using sample(8) like you did if the file provided as parameter is nil (which could be the default). That would look like this:

# Executes the given block with a temporary file with the given `file_name`
def with_tmp_file(named: nil, content: '')
  in_tmp_dir do |tmp_dir|
    file_name = named || ('a'..'z').to_a.sample(8).join # 8-character random file name if nil
    file_path = File.join(tmp_dir, file_name)
    File.write(file_path, content)
    yield file_path
  ensure
    File.delete(file_path)
  end
end

And then you'd simply replace all your calls to with_tmp_file_path with this with_tmp_file (without parameters), and all your calls to with_tmp_file_path_for_file_named(x) with with_tmp_file(named: x).

[AliSoftware]

I'd suggest to provide an optional content parameter to the method to allow custom content to be written as well instead of an empty file.

i.e. making the signature of the method look more like: def with_tmp_file(basename:, content: '') and then use the content parameter on this line.

[AliSoftware]

This ends up being the only describe at that level, which makes it kinda pointless, and also containing quite a lot of tests in a single group.

I'd suggest you to split it instead, for example:

• describe 'use the proper key' (L28-L117)
• describe 'errors on invalid input' (the rest)

[jkmassel]

Fixed in 281c181 – I tried to keep to the "reads as a sentence" format

[wpmobilebot]

🚫	unexpected token tIDENTIFIER (Using Ruby 2.6 parser; configure using TargetRubyVersion parameter, under AllCops)