Add s3 support #1
Conversation
[WIP] fix condition when target shouldn't build, but target_folder not exists
|
Sorry for the slow response |
|
@Rag0n No worries. |
|
@Rag0n Any luck on review my changes? |
| end | ||
|
|
||
| # Path of the target's cache | ||
| # | ||
| # @return [Pathname] | ||
| def self.framework_cache_path_for(target, options) | ||
| framework_cache_path = cache_root + xcode_version | ||
| framework_cache_path = Pathname.new('') | ||
| if include_cache_root |
There was a problem hiding this comment.
include_cache_root parameter is missing in method signature. Also I would rather split this method in two separate methods: local_framework_cache_path and s3_framework_cache_path, because flag parameter is a code smell.
| Dir.mktmpdir {|dir| | ||
| s3.bucket(Podfile::DSL.s3_options[:bucket]).object("#{s3_cache_path}").get(response_target: "#{dir}/framework.zip") | ||
| unzip("#{dir}/framework.zip", path) | ||
| return true |
There was a problem hiding this comment.
Actually this return relates to mktmpdir's closure scope. And because of it return value of self.has? is actually equal to the return value of Dir.mktmpdir. Instead you should assign result to true
| @@ -12,27 +14,89 @@ class SharedCache | |||
| # @return [Boolean] | |||
| def self.has?(target, options) | |||
| if Podfile::DSL.shared_cache_enabled | |||
| framework_cache_path_for(target, options).exist? | |||
| path = framework_cache_path_for(target, options, true) | |||
There was a problem hiding this comment.
I would rather move local and s3 checks into separate methods to improve readability and here just call them: has_local_cache_for(target, options) || has_s3_cache_for(target, options)
|
@jaximan |
|
@Rag0n Made a changes based on your comments. There is one problem I have, fastlane-plugin-s3 is using aws sdk v2 with hardcoded version, which is incompatible with these changes. Not sure of moving to v2 or introduce version bump for that plugin. |
|
Looks good. I think it would be better to move to v2, but it's not a big issue. I mean I can merge it as is. |
|
Let's merge it, I created PR for that library: fastlane-community/fastlane-plugin-s3#72 |
Let me know if I can improve my changes, first time code in Ruby, maybe there is better way for some steps