Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: LOGSTASH_PATH auto expanding #100

Merged
merged 11 commits into from
Feb 11, 2020
Merged

Feat: LOGSTASH_PATH auto expanding #100

merged 11 commits into from
Feb 11, 2020

Conversation

kares
Copy link
Contributor

@kares kares commented Jan 8, 2020

this is mostly to be able to publish LS plugins, a Gemfile usually looks like:

logstash_path = ENV["LOGSTASH_PATH"] || "../../logstash"
use_logstash_source = ENV["LOGSTASH_SOURCE"] && ENV["LOGSTASH_SOURCE"].to_s == "1"

if Dir.exist?(logstash_path) && use_logstash_source
  gem 'logstash-core', :path => "#{logstash_path}/logstash-core"
  gem 'logstash-core-plugin-api', :path => "#{logstash_path}/logstash-core-plugin-api"
end

plugins usually depend on logstash-core and/or logstash-code-plugin-api gems
plugin-api hasn't been released in a while and possible logstash-core releases will halt as well

publish command adds --env "LOGSTASH_SOURCE=1 LOGSTASH_PATH=#{releases[last]}" defaults
where LOGSTASH_PATH='#{latest}' is tracked and #(enclosed) value gets substituted based on LS' release information (one could also override a specific version with LOGSTASH_PATH='#{7.5.1}')

the brackets #{...} part (#(...) also works - did not figure out anything more suitable) causes a LS release tar.gz download and gems extraction into a temp dir to substitute a valid LOGSTASH_PATH=...

@elasticsearch-bot elasticsearch-bot self-assigned this Jan 8, 2020
@yaauie yaauie assigned yaauie and unassigned elasticsearch-bot Jan 8, 2020
@kares
Copy link
Contributor Author

kares commented Jan 15, 2020

should have noted that both @jarvis publish ... and the new @jarvis run ... rake release
where tested manually (using --env "LOGSTASH_SOURCE=1 LOGSTASH_PATH=#{latest}" for run cmd)

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality makes sense. I have left comments about potentially making the usage more clear.

@@ -18,7 +20,7 @@ class Bug < ::Jarvis::Error; end
banner "Publish a logstash plugin"

option "--workdir", "WORKDIR", "The place where this command will download temporary files and do stuff on disk to complete a given task."
option "--env", "ENV", "ENV variables passed to publish task.", :default => 'JARVIS=true LOGSTASH_SOURCE=false' # to be able to bundle wout LS
option "--env", "ENV", "ENV variables passed to publish task.", :default => 'JARVIS=true LOGSTASH_SOURCE=1 LOGSTASH_PATH=#{releases[last]}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the injection of ruby-style string interpolation into a shell command is likely to cause confusion, may require shell escaping on certain shells, and is likely more heavy-handed than we need.

Could we special-case LOGSTASH_PATH values starting with RELEASE@? This would make usage a bit more straight-forward and eliminate some of the complexity of shell-escaping values when using it:

With the above, we could also support SNAPSHOT@ prefixes for situations where we rely on a fix that hasn't made it into a release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @yaauie ... struggled with the 'auto-expanding' naming convention.
very glad you looked into it - will look into changing the format as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to use the suggested RELEASE/SNAPSHOT@ qualifier ...

@kares kares merged commit 0e35ddc into elastic:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants