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.

Sign up for GitHub

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

Jump to bottom

[puppet-jenkins#366] Replace -toolLocations with --toolLocation #367

Merged
merged 2 commits into from
Oct 7, 2015
Merged

[puppet-jenkins#366] Replace -toolLocations with --toolLocation #367

merged 2 commits into from
Oct 7, 2015

Conversation

patcadelina
Copy link

Should fix issue #366

@jenkinsadmin
Copy link

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@patcadelina patcadelina force-pushed the tool-location branch 2 times, most recently from c097225 to 7a99cde Compare September 15, 2015 03:11
rtyler
rtyler reviewed Sep 17, 2015
View reviewed changes
manifests/slave.pp
@@ -49,7 +49,7 @@
# Not required. Single string of whitespace-separated list of labels to be assigned for this slave.
#
# [*tool_locations*]
# Not required. Single string of whitespace-separated list of tool locations to be defined on this slave. A tool location is specified as 'toolName:location'.
# Not required. Array list of tool locations to be defined on this slave. A tool location is specified as 'toolName:location'.
Copy link

Choose a reason for hiding this comment

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

This does change an API which concerns me, it would be better to take an Array or the single String for the parameter and then prepare the appropriate data for the template

@patcadelina
Copy link
Author

Updated.

@rtyler
Copy link

rtyler commented Sep 18, 2015

Without some rspec-puppet tests this will have to wait until I have a chance to write some, which will likely be in the next couple weekends

@patcadelina patcadelina force-pushed the tool-location branch 2 times, most recently from 60dd474 to 6da207b Compare September 21, 2015 07:42
@patcadelina
Copy link
Author

I went ahead to write one. Also, from the docs,

-t (--toolLocation)            : A tool location to be defined on this slave.
                              It is specified as 'toolName=location'

Not sure if we have to go with --toolLocation toolName=location. Right now this PR resolves to --toolLocation toolName:location as we are directly referencing slave::tool_locations.

[ tool_locations ]
Not required. Single string of whitespace-separated list of tool locations to be defined on this slave. A tool location is specified as 'toolName:location'.

How do you intend to test which works?

@jhoblitt
Copy link
Member

I have not used the --toollocation flag with swarm, so I can't comment on what the correct calling semantics are.

The string splitting logic introduced into templates/jenkins-slave-defaults.erb is not covered by the spec tests. It might also be a good idea to validate the toollocation param as either undef or string with validate_string() to catch an array being passed by accident. Another possibility would be to support both a white space delimited string or an array of options.

@jhoblitt
Copy link
Member

A hash might be better than an array if it's worth the effort to support multiple types. Would there ever be a use case to build up a list of a tool paths from hiera?

@jhoblitt
fix rspec-puppet raise_error warning
ac2ca09 
Resolves this warning:

    WARNING: Using the `raise_error` matcher without providing a specific
    error or message risks false positives, since `raise_error` will match
    when Ruby raises a `NoMethodError`, `NameError` or `ArgumentError`,
    potentially allowing the expectation to pass without even executing the
    method you are intending to call. Instead consider providing a specific
    error class or message. This message can be supressed by setting:
    `RSpec::Expectations.configuration.warn_about_potential_false_positives
    = false`.
@patcadelina
Copy link
Author

I tested for the right syntax, updated the diff for this as well.

$ java -jar swarm-client-2.0-jar-with-dependencies.jar --toolLocation foo:bar
An argument for setting a Map must contain a "="
$ java -jar swarm-client-2.0-jar-with-dependencies.jar --toolLocation foo=bar
Discovering Jenkins master

I'm not sure about hiera, haven't used it myself.

jhoblitt
jhoblitt reviewed Sep 22, 2015
View reviewed changes
spec/classes/jenkins_slave_spec.rb

describe 'with multiple tool locations' do
let(:params) { { :tool_locations => 'Python-2.7:/usr/bin/python2.7 Java-1.8:/usr/bin/java' } }
it { should contain_file(slave_runtime_file).with_content(/--toolLocation Python-2.7=\/usr\/bin\/python2.7|--toolLocation Java-1.8=\/usr\/bin\/java/) }
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't test that both option string are present. How about something along these lines?

it do
  should contain_file(slave_runtime_file).
    with_content(/--toolLocation Python-2.7=\/usr\/bin\/python2.7/).
    with_content(/--toolLocation Java-1.8=\/usr\/bin\/java/)
end

@patcadelina
Copy link
Author

Thanks for the feedback.

jhoblitt
jhoblitt reviewed Sep 22, 2015
View reviewed changes
templates/jenkins-slave-defaults.erb
<% if @tool_locations %>
TOOL_LOCATIONS_ARG=""
<% @tool_locations.split.each do |tool_location| %>
TOOL_LOCATIONS_ARG='$TOOL_LOCATIONS_ARG --toolLocation <%= tool_location.split(':').first -%>=<%= tool_location.split(':').last -%>'
Copy link
Member

Choose a reason for hiding this comment

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

What if the string is already in the Python-2.7=/usr/bin/python2.7 format? If there are any existing users of the tool_location param, wouldn't they have to be using that form?

@patcadelina
Copy link
Author

@rtyler was concerned about breaking the API. I could go either way really.

@jhoblitt
Copy link
Member

It looks like version 2.0 intentionally made a breaking change to the -toollocation[s] semantics.

jenkinsci/swarm-plugin#28

I'd vote for supporting the current plugin version since, as-is, user's will experience breakage when upgrading swarm

@rtyler rtyler added this to the 1.6.0 - Kato milestone Sep 23, 2015
@rtyler
Copy link

rtyler commented Sep 23, 2015

Great catch @jhoblitt! I can buy that, we'll just have to document it thoroughly, sorry for putting this through such scrutiny @patcadelina

@jhoblitt
Copy link
Member

A validation regex should be able to catch the old format with a useful warn/fail message.

@patcadelina
Copy link
Author

No worries.

@jhoblitt
Copy link
Member

@patcadelina - I'd like to echo @rtyler and thank you for your persistence in enduring such a long review process.

rtyler pushed a commit that referenced this pull request Oct 7, 2015
Merge pull request #367 from patcadelina/tool-location
b51d4c7 
[puppet-jenkins#366] Replace -toolLocations with --toolLocation
@rtyler rtyler merged commit b51d4c7 into voxpupuli:master Oct 7, 2015
@jhoblitt jhoblitt mentioned this pull request Oct 8, 2015
Closed
@patcadelina patcadelina deleted the tool-location branch October 9, 2015 02:37
@br0ch0n br0ch0n mentioned this pull request Jul 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.6.0 - Kato
Development

Successfully merging this pull request may close these issues.
4 participants
@patcadelina @jenkinsadmin @rtyler @jhoblitt