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

added auth header, refactored, added tests #1

Merged
merged 1 commit into from
Aug 25, 2014
Merged

added auth header, refactored, added tests #1

merged 1 commit into from
Aug 25, 2014

Conversation

harlanbarnes
Copy link
Contributor

  • I added the auth header to the "jenkins list" call as opposed to JUST the build calls. Our jenkins installation is all behind an auth screen (not just the build calls). I did some testing that seemed to indicate that sending the authorization header for URLs that didn't need it had no real effect.
  • Refactored ...
    • I moved the authorization handling into a single method called headers. I guessed that if later headers were needed they'd be easy to inject here.
    • The auth header is now optional, however, I doubt very many people have their Jenkins installation setup that way. 😄
    • I changed the caching of the job list to lazy load instead of explicitly calling the cache method. This was also important because there was a bug where if the user hadn't called "jenkins list" first and just tried to do a build, the build would fail.
    • Collapsed the jenkins_build_by_id into the jenkins_build method
  • Fixed typo in the help output
  • Changed the regex to cover both "jenkins b ID" and "jenkins build ID"
  • Added tests to cover the "jenkins build" action

@daniely
Copy link
Owner

daniely commented Aug 25, 2014

Sorry I somehow missed this. Looks great!

daniely added a commit that referenced this pull request Aug 25, 2014
added auth header, refactored, added tests
@daniely daniely merged commit f4803a7 into daniely:master Aug 25, 2014
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.

2 participants