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

get current build artifact #147

Merged
merged 6 commits into from
Nov 5, 2014
Merged

Conversation

joelneubert
Copy link

Noted #61 and created a custom method to find and download the current build.

I'm sure it could use some cleanup I'd love to see what that is.

An example of how I'm using this can be seen here https://gist.github.com/joelneubert/19f607887a7246925a17

Joel Neubert added 2 commits September 10, 2014 10:27
@arangamani
Copy link
Owner

Sure, curl is platform dependent and is an explicit dependency. This could be replaced by using Net::HTTP. A method could be added to the Client class which could download a file given an endpoint relative to the base url and a location to save. This method can then use that to download and save the file. I would love to help.

@arangamani
Copy link
Owner

An example could be found here.

@joelneubert
Copy link
Author

I'll take a look, my main stumbling block with Net::HTTP had been getting basic_auth to work when requesting the file.

@arangamani
Copy link
Owner

The Client class already handles the authentication. I don't know if that should be any different for downloading a file.

@joelneubert
Copy link
Author

so this works for me using Net::HTTP now instead of curl. I'd love feedback on how to clean it up if you have any. But it does accept a job and filename and download the current build artifact.

request = Net::HTTP::Get.new(uri.request_uri)
request.basic_auth(@username, @password)
response = http.request(request)
File.write(filename, response.body)
Copy link
Owner

Choose a reason for hiding this comment

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

File.expand_path(filename) should be better so relative paths work cleanly.

Copy link
Owner

Choose a reason for hiding this comment

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

Also this is going to simply store the response body regardless of the response. So checking if the response code is a success (200) before saving the body should be good.

@joelneubert
Copy link
Author

Thanks for reviewing and helping me clean that up. If anything else needs to be done let me know. I'll be happy to see this contribution available for other users. Hope it is helpful for all.

@@ -21,6 +21,7 @@
#

require 'jenkins_api_client/urihelper'
require 'net/https'
Copy link
Owner

Choose a reason for hiding this comment

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

Is this library used in this file? If not it can be removed.

@arangamani
Copy link
Owner

Made a couple minor comments. Once addressed, I'll merge and release another version. Thanks again for the contribution!

@howdoicomputer
Copy link

Is this going to be merged?

@arangamani
Copy link
Owner

@howdoicomputer This will be merged shortly.

arangamani added a commit that referenced this pull request Nov 5, 2014
@arangamani arangamani merged commit da43a0c into arangamani:master Nov 5, 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.

3 participants