Closed codekitchen closed 8 years ago
@codekitchen I don't think we should be checking in client jars to GitHub repo. These are required only at build time -- when the Gem is created, so we should fetch them at gem creation time. For example, this is how we handle Kafka jars, which is similar to this library. We use a library called jar-dependencies which fetches the jar directly from maven.
https://github.com/logstash-plugins/logstash-input-kafka/blob/master/Rakefile
rake install_jars
is called when gem is built
If you would like, I can make the change as well, but we should definitely remove the jars from this PR
Hi @codekitchen, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?
Sure @suyograo that sounds reasonable -- this was my first jruby project, so I just followed what seemed to be best practice for other logstash plugins back in March 2015 when I started out. But I'm sure things have changed a lot since then.
To be clear though, you say "we should definitely remove the jars from this PR" -- the jars were already committed to the project, the only way to remove them would be to remove the git history and start fresh (or do some invasive rewriting). Do you want me to do that? Doesn't seem worth the loss of information. Or are you just asking me to create a new commit and append it to the PR? That I can do.
@codekitchen 779e6ec
works. We need one more file to make jar_dependencies
work. https://github.com/logstash-plugins/logstash-input-kafka/blob/master/lib/logstash-input-kafka_jars.rb. Can you copy contents as is under lib/
, but call it logstash-input-kinesis_jars.rb
Yeah that file already exists with those contents. However, there's something wrong with my change, the class com.amazonaws.services.kinesis.clientlibrary.interfaces.v2.IRecordProcessorFactory
cannot be found even though the other classes in that jar can be found. I'm looking into it.
OK there we go... I haven't had a chance to test out the re-built gem yet, but the specs pass and the contents of the gem look correct, including all the jars.
@codekitchen one small comment about git ls
and we can merge this.
👍 done
LGTM
I did some rebasing here on the assumption that you'll want to preserve the git history.