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

Add language scala #524

Merged
merged 2 commits into from
Mar 21, 2016
Merged

Add language scala #524

merged 2 commits into from
Mar 21, 2016

Conversation

cmluciano
Copy link

No description provided.

@thommay
Copy link
Contributor

thommay commented May 21, 2015

LGTM 👍

@cmluciano
Copy link
Author

@mcquin good on your side?

@mcquin
Copy link
Contributor

mcquin commented Jun 18, 2015

LGTM

Should we add a languages/scala/sbt attribute, based on the presence of sbt? These aren't packaged together, but they tend to go hand-in-hand.

@cmluciano
Copy link
Author

Sounds great to me. Should we collect just the sbt info or also say that if sbt is found, we should label this machine as having the language scala installed?

@mcquin
Copy link
Contributor

mcquin commented Jun 19, 2015

Some people may have scala, but not sbt.

I'm +1 for having sbt as a subattribute of languages/scala (i.e, provides "languages/scala/sbt") which would contain version info (scala[:sbt][:version] = ...), or be nil if sbt is not present on the system (scala[:sbt] = nil).

@cmluciano
Copy link
Author

@mcquin I added a step to check for sbt.

scala = Mash.new
so = shell_out("scala -version")
if so.exitstatus == 0
output = so.stdout.split
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] Looks like you've got two spaces after your = sign.

@mcquin
Copy link
Contributor

mcquin commented Jun 22, 2015

Some minor things, but looking great @cmluciano ! =)

@mcquin
Copy link
Contributor

mcquin commented Jun 22, 2015

👍

@cmluciano
Copy link
Author

Can we get a few more 👍 from @chef/client-core

@mcquin
Copy link
Contributor

mcquin commented Jul 8, 2015

We need another review from @chef/client-core

@ranjib
Copy link

ranjib commented Jul 8, 2015

👍
Q: do we know if sbt -version and scala -version will slow down ohai ?

@mcquin
Copy link
Contributor

mcquin commented Jul 9, 2015

@ranjib Added time appears negligible

time scala -version time sbt --version
os x w/ scala, sbt installed 0.36s user
0.07s system
0.368 total
0.32s user
0.08s system
0.322 total
ubuntu vm w/o scala, sbt installed real 0m0.115s
user 0m0.038s
sys 0m0.071s
real 0m0.115s
user 0m0.038s
sys 0m0.071s

context "if scala is not installed" do

before(:each) do
allow(plugin).to receive(:shell_out)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we've done something special in Ohai, this isn't what happens when scala isn't installed. On my machine:

[1] pry(main)> require 'mixlib/shellout'
=> true
[2] pry(main)> include Mixlib
=> Object
[3] pry(main)> c = ShellOut.new("scala -v")
=> <Mixlib::ShellOut#70321135567700: command: 'scala -v' process_status: nil stdout: '' stderr: '' child_pid: nil environment: {} timeout: 600 user:  group:  working_dir:  >
[4] pry(main)> c.run_command
Errno::ENOENT: No such file or directory - scala
from /Users/ddeleo/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/mixlib-shellout-2.1.0/lib/mixlib/shellout/unix.rb:338:in `exec'

I think ohai still catches all failures and ignores them, but it'd be nice to rescue that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. I assumed shellout caught these errors, appears that is not the case. Good to know, taking note.

Copy link
Author

Choose a reason for hiding this comment

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

Should I just remove this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, don't remove this context.

Dan's comment shows shell_out("scala -v") throws an Errno::ENOENT, rather than returning what's been mocked here. The spec should be updated to reflect that behavior.

@mcquin
Copy link
Contributor

mcquin commented Dec 4, 2015

LGTM

@btm
Copy link
Contributor

btm commented Dec 9, 2015

What are the two begin/end blocks for?

Does it make sense if we can't find scala to return rather than look for sbt too?

@lamont-granquist
Copy link
Contributor

Yeah the begin/end should go and the initial check should probably just be a shell_out! and let the exception abort the plugin.

@cmluciano
Copy link
Author

Ok begin/end have been removed. Just waiting on CI :)

@cmluciano
Copy link
Author

Failures seemed related to style, fixed up

@mcquin
Copy link
Contributor

mcquin commented Mar 21, 2016

👍 @chef/client-core

# Check for scala
output = nil

scala = Mash.new
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to go after the check for the exist status. Otherwise we get an empty hash when scala isn't around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohai won't add attribute to its data until you do attribute Mash.new.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore me

tas50 added a commit that referenced this pull request Mar 21, 2016
@tas50 tas50 merged commit 218d894 into chef:master Mar 21, 2016
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 24, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants