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

Feature/puppet helper num executors #260

Merged
merged 4 commits into from
May 3, 2015
Merged

Feature/puppet helper num executors #260

merged 4 commits into from
May 3, 2015

Conversation

jhoblitt
Copy link
Member

  • add get_num_executors()/set_num_executors() methods to puppet_helper
  • add jenkins::cli::exec defined type
  • add executors param to jenkins class

@jhoblitt
Copy link
Member Author

I ended up making a fair number of minor house keeping changes while working on this branch so I've split them out as separate commits and moved them to the beginning of the patch series. I would be happy to submit them as a separate PR, if requested.

I had some difficulty choosing a description name for the new defined type. I originally called it jenkins::exec_cli_helper but eventually decided to rename it to jenkins::cli::exec. I am willing to rename it if required to get this PR merged.

As jenkins::cli::exec is now, the title/command params are non-orthogonal with unless. Does it make sense to give up some convenience and use $HELPER_CMD from the env as is required with the unless parameter? I'm wondering if I should have implemented this as type/provider instead of a wrapper around exec but this was the path I started down...

One of my intents for jenkins::cli::exec is to make it easy to replace the existing exec resources that are calling the cli helper. I did not include that as part of this PR in order to keep the review complexity down. I will volunteer to make that conversion if this PR is merged.

@jhoblitt jhoblitt force-pushed the feature/puppet_helper_num_executors branch from 3de868f to b0953c6 Compare March 26, 2015 20:01
@jhoblitt
Copy link
Member Author

It looks like my local bundle was in a funny state. The travis build failed as some forward porting is required for rspec > 3.

@jhoblitt jhoblitt force-pushed the feature/puppet_helper_num_executors branch 2 times, most recently from f869b46 to 6f3ef22 Compare March 26, 2015 22:39
@jenkinsadmin
Copy link

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

@jhoblitt
Copy link
Member Author

The build failed due to puppet 3.5.0/ruby 1.9.3 and I suspect Travis gltiched. I can't manually restart the build myself so I'll push some sort of whitespace change to kick it.

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated

@jhoblitt jhoblitt force-pushed the feature/puppet_helper_num_executors branch from 6f3ef22 to 5382627 Compare March 27, 2015 20:42
This was referenced Mar 31, 2015
Closed
Closed
rtyler
rtyler reviewed Apr 25, 2015
View reviewed changes
Gemfile
gem 'rspec-its'
gem 'puppet-lint', '>= 0.3.2'
gem 'rspec-puppet', '>= 1.0.1'
gem 'puppetlabs_spec_helper', :github => 'jenkins-infra/puppetlabs_spec_helper'
gem 'puppetlabs_spec_helper', :require => false
Copy link

Choose a reason for hiding this comment

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

Why was the jenkins-infra fork of the spec helper removed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't recall specifically what it was breaking for me at the time, likely beaker, but 0.10.2 is needed for future parser/puppet 4.0.0 anyways.

@jhoblitt
Copy link
Member Author

@rtyler I'd be happy to split out the house keeping changes as a separate PR if that would help get them merged. I have a fairly substantially amount on work in this module that I'm not quite ready to break up into PRs and feed back yet but it all needs the basic house keeping so unit/acceptance testing works.

@jhoblitt jhoblitt force-pushed the feature/puppet_helper_num_executors branch 4 times, most recently from 5a6c4d5 to 78c350e Compare May 2, 2015 00:19
@jhoblitt
Copy link
Member Author

jhoblitt commented May 2, 2015

@rtyler My working branch is now over 70 commits and 3000 lines added ahead of your master. I'd really like to get some of these small house keeping commits merged as they will be common to any feature branches I split with a clean history for a PR. I have broken 11 commits out from this branch as individual PRs. #270 #271 #272 #273 #274 #275 #276 #277 #278 #279 #280

@rtyler
Copy link

rtyler commented May 2, 2015

@jhoblitt all merged up, thanks for the house cleaning! I think this change can be merged once it's rebased atop the newest master

@jhoblitt jhoblitt force-pushed the feature/puppet_helper_num_executors branch from 78c350e to f693e20 Compare May 2, 2015 23:40
Joshua Hoblitt added 4 commits May 2, 2015 16:47
@jhoblitt
add get_num_executors()/set_num_executors() methods to puppet_helper
df133b4 
These methods may be used to get/set the number of executors for the
master. Eg.:

    java -jar /usr/lib/jenkins/jenkins-cli.jar -s http://localhost:8080 groovy /usr/lib/jenkins/puppet_helper.groovy get_num_executors

    java -jar /usr/lib/jenkins/jenkins-cli.jar -s http://localhost:8080 groovy /usr/lib/jenkins/puppet_helper.groovy set_num_executors 42
@jhoblitt
move Exec['reload-jenkins'] from jenkins::cli -> jenkins::cli::reload
979cb01 
Due to resource containment, it was not possible for a consumer of
`jenkins::cli` to both have a dependency on `Class['jenkins::cli']` and to
notify `Exec['reload-jenkins']`.  It is recommended that notifications be
instead be sent to `Class['jenkins::cli::reload']` to ease any future
refactoring.
@jhoblitt
add jenkins::cli::exec defined type
dad38cb
@jhoblitt
add executors param to jenkins class
f6ebc21 
Similar semantics as `class { 'jenkins::slave': executors => ... }` but for
the Jenkins master.
@jhoblitt jhoblitt force-pushed the feature/puppet_helper_num_executors branch from f693e20 to f6ebc21 Compare May 2, 2015 23:47
@jhoblitt
Copy link
Member Author

jhoblitt commented May 3, 2015

@rtyler thanks for the merge campaign! I've rebased on the current master.

@rtyler rtyler added this to the 1.4.0 - Smithers milestone May 3, 2015
rtyler pushed a commit that referenced this pull request May 3, 2015
Merge pull request #260 from jhoblitt/feature/puppet_helper_num_execu…
2404c06 
…tors

Feature/puppet helper num executors
@rtyler rtyler merged commit 2404c06 into voxpupuli:master May 3, 2015
@jhoblitt jhoblitt deleted the feature/puppet_helper_num_executors branch May 3, 2015 15:34
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.4.0 - Smithers
Development

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