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

Allow artifact_file to handle snapshot and latest version correctly (V2 !) #128

Merged
merged 5 commits into from
Mar 27, 2014
Merged

Conversation

BarthV
Copy link
Contributor

@BarthV BarthV commented Mar 25, 2014

Hi !

I've cleaned up my previous PR and I have taken into account the comments and requests.
I successfully tested many update/upgrade/downgrade/init scenarios from Nexus.

Please test and review this work.

PS : I know some if tests may be quite long ... Perhaps it would be interesting to simplify this into a def

@BarthV
Copy link
Contributor Author

BarthV commented Mar 25, 2014

Target file is now renamed with provided name during the process.

#
# @return [Boolean] true when the coordinates's version is a snapshot.
def snapshot?(coordinates)
coordinates.split(':')[-1].include?('-SNAPSHOT')
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think the interim step here is important. Assuming that the error handling is good, we should avoid this array splicing and just use

artifact = NexusCli::Artifact.new(coordinates)
aritfact.version.include?('-SNAPSHOT')

@BarthV
Copy link
Contributor Author

BarthV commented Mar 25, 2014

Roger that, I'll update this 2 tests using artifact method calls

@BarthV
Copy link
Contributor Author

BarthV commented Mar 25, 2014

I'll verify checksuming tomorrow, retest and fix if needed. (It was OK when i tested this so far .. But I could have missed something)
Thx for the review btw

@BarthV
Copy link
Contributor Author

BarthV commented Mar 26, 2014

updated.
Previous comments are now fixed.

@KAllan357
Copy link
Contributor

This looks good for now, though I do agree that the if statements are getting a bit crazy. 👍, but I think I'd like to see us revisit this whole Provider and see if it can be streamlined a bit.

TYVM for the contribution!

KAllan357 added a commit that referenced this pull request Mar 27, 2014
Allow artifact_file to handle snapshot and latest version correctly (V2 !)
@KAllan357 KAllan357 merged commit 6410b2f into RiotGamesCookbooks:master Mar 27, 2014
@KAllan357 KAllan357 added this to the 2.0.0 milestone Mar 27, 2014
@BarthV BarthV deleted the file-snap-v2 branch March 28, 2014 10:50
@bdemers
Copy link

bdemers commented Apr 2, 2014

Works Here! Got an ETA on the next release ?

@KAllan357
Copy link
Contributor

I'd like to see most of the 2.0.0 milestones closed before we cut a release. To me, the big ones are #113, #57 (in general, some updates / support for Chef 11). I also thought I had an issue to refactor the data bag loading logic to simplify it, but I can't seem to find that.

@BarthV
Copy link
Contributor Author

BarthV commented May 14, 2014

Do you need some help to work on #113 & #57 ?
I'll be happy to have a new release for this CB too 😺

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