search

elastic / jarvis

Logstash's ChatOps bot, J.A.R.V.I.S.
41 stars 28 forks source link

Ensure UTF-8 encoding is used independent of environment. #82

Closed jakelandis closed 7 years ago

jakelandis commented 7 years ago

Fixes #81

I had a hard time testing the full workflow, so am only 90% sure this fixes the underlying issue. I was able to reproduce the error on an Ubuntu box by breaking the local setting with export LC_ALL=en_US (which is invalid), and then watch this change resolve that the invalid byte sequence in US-ASCII error.

However, I could not get a full workflow tested due to 

Failure/Error: pid, stdin, stdout, stderr = Open4::popen4("git", "-C", "#{git.dir}", "am", "--", patch.path)
    NotImplementedError:
       fork is not available on this platform

Also, the comment to the gemspec was necessary to get bundler to run, and missing require showed up in my testing the single merge command.

FWIW, I believe that this issue can also be fixed locally by ensuring the locale is setup for UTF-8. This change should allow the environment to be what ever, and will always enforce UTF-8. Note - this change may actually beak UTF-16 users.

jakelandis commented 7 years ago

@jordansissel - Could you please review ? I am unsure if forcing UTF-8 is appropriate here, or if we should just better understand the environment in which this error was seen.

Also, this change is setting a global value from the merge command, which is kinda odd. Is there better place for this ?

jordansissel commented 7 years ago

It's probably best to put the Encoding globals to somewhere more ... global (I don't have words for this).. Maybe in the lita config?

Either way, I think this change is fine, and I am not worried about where the Encoding global setting lives.

jordansissel commented 7 years ago

Hmm. Jarvis can't merge this:

{:timestamp=>"2017-07-18T04:51:09.049197+0000", :message=>"error: patch failed: docs/index.asciidoc:35", :level=>:error, :file=>"cabin/mixins/pipe.rb", :line=>"47", :method=>"block in pipe", :context=>"logstash-filter-tld#6", :branch=>"master"}

{:timestamp=>"2017-07-18T04:51:09.049333+0000", :message=>"error: docs/index.asciidoc: patch does not apply", :level=>:error, :file=>"cabin/mixins/pipe.rb", :line=>"47", :method=>"block in pipe", :context=>"logstash-filter-tld#6", :branch=>"master"}

{:timestamp=>"2017-07-18T04:51:09.049612+0000", :message=>"Patch failed at 0001 asciidocs update", :level=>:info, :file=>"cabin/mixins/pipe.rb", :line=>"47", :method=>"block in pipe", :context=>"logstash-filter-tld#6", :branch=>"master"}

I'll merge manually.