RiotGamesCookbooks / artifact-cookbook

Provides your cookbooks with the Artifact Deploy LWRP
Other
133 stars 91 forks source link

Dont unzip war/jars #142

Closed jmickle closed 10 years ago

jmickle commented 10 years ago

War/JAR files should not always be unzipped.

https://github.com/RiotGames/artifact-cookbook/blob/master/providers/deploy.rb#L194-L207

capoferro commented 10 years ago

This is true, good catch. Are you able to provide a fix?

jmickle commented 10 years ago

I suppose i can fix

capoferro commented 10 years ago

That would be appreciated!

This is not something that we are affected by (we mostly deploy tars via this cookbook) so it's unlikely this will get fixed except through a community contribution.

jmickle commented 10 years ago

Great, i will get a PR in the next couple of days on this

jmickle commented 10 years ago

PR https://github.com/RiotGames/artifact-cookbook/pull/146 created

this is a temporary fix, the problem here is that this will copy a file from an already downloaded location into a new location. This could be bad because it may result in loading the file into memory while ruby does its read buffer which could cause slow operations or possibly OOM problems for users. The proper fix for this would be to download directly to the release path for jar/war files. Though at the moment it seems that would require modifications to package.rb/artifact.rb and about 95% refactor of the deploy rb. So we may want to discuss an alternative better approach to this.

florentdupont commented 10 years ago

I don't understand why the use of is_tarball= false is not enough in your case ? you could provide an artifact with a WAR/JAR extension and assure it will not be extracted by using the is_tarball parameter. This is the (only) goal of this parameter as shown here.

if new_resource.is_tarball
  extract_artifact!
else
  copy_artifact
end

In the Pull Request #146 you provided, we are expecting to extract an artifact but instead this one is just copied (the contract of this method is wrong then...). I think that keeping a way to extract JAR/WAR files is useful for some users (me included). Don't you think we could go back on this functionality and use is_tarball (and by the way maybe rename it) to manage if an artifact should be extracted or not ?

jmickle commented 10 years ago

@florentdupont precisely, the problem is. it does not work. The way the rest of it is written is it still seems to pass it directly to this function. Thats why if you read my last comment there needs to be some refactoring done on this. For now the copying was a quick fix to stop the tar -zxvf on a .war

florentdupont commented 10 years ago

I'm probably missing something here. To clarify the situation, when I use the following code inside extract_artifact! (in providers/deploy.rb) :

   when /zip$|war|jar|ear/
      if Chef::Artifact.windows?
        windows_zipfile release_path do
          source    cached_tar_path
          overwrite true
          retries 2
        end
      else
        package "unzip"
        execute "extract_artifact!" do
          command "unzip -q -u -o #{cached_tar_path} -d #{release_path}"
          user    new_resource.owner
          group   new_resource.group
          retries 2
        end
      end

Test 1. with a cookbook like this :

artifact_deploy "xxx" do
  version             "LATEST"
  artifact_location   "xxx:xxx:war"
  nexus_configuration nexus_configuration_object
  deploy_to           "c:/temp/deploy_test"
  is_tarball          false
end

My war is deployed at C:\Temp\deploy_test\releases\14.9.1-20140829.121052-5\xxx-14.9.1-20140829.121052-5.war (it stays zipped)

Test 2. With a second cookbook like :

artifact_deploy "xxx" do
  version             "LATEST"
  artifact_location   "xxx:xxx:war"
  nexus_configuration nexus_configuration_object
  deploy_to           "c:/temp/deploy_test"
  is_tarball          true
end

is deploying like this : My war is unzipped and deployed at C:\Temp\deploy_test\releases\14.9.1-20140829.121052-5\which contains WEB-INF, META-INF (the content of my war).

Some details here :

So, for me - under Windows - it's working as I expected. In my case war/jar are not always unzipped. Do you think your problem could be specific to Unix platform ? Or I'm probably misunderstanding something. Thanks for your help.

jmickle commented 10 years ago

very possible specific to unix platform, we may need to investigate further.

rampire commented 9 years ago

Excepting minor changes and fixes in 1.12.1 (it's other question why version number is less) from master branch got significant functionality break. For me at least. Got to pin 1.12.2 version from 1-12-stable branch.