-
Notifications
You must be signed in to change notification settings - Fork 91
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 #125
Conversation
I did many test (snapshots, static version, latest, etc...), many times. I also tried to make artifact_file resources upgrade/downgrade perfectly (new file is downloaded). Idempotency is still OK. But it would be nice to all of you to test it again :). Thx |
I got a problem with latest tag (snap is ok), and still have indent issues. Fixing this right now |
Ready to be tested ! |
OK ... my artifact's version resolution is wrong : |
We have worked around this issue in the past by creating hard links in the after proc. myartifact-1.2.3-timestamp.jar -> myartifact-1.2.3-SNAPSHOT.jar (also a work around for #112 ) |
I'm not sure about it, but hardlink in the after proc suppose that we can guess filename or resolve it from nexus metadata. That why a providing a built-in procedure to resolve filename or making a symlink would be nice. Tell me if I'm wrong :/ |
Absolutely, a proper solution would be much better. Just doc'ing the work around for anyone googling until this gets a fix.
Granted this is work around is more of a hack then anything... |
fix: load attr from lwrp name before require
…fact-cookbook into file-handle-snapshot
rebased |
File symlink Don't hesitate to fix this code if needed
File symlink
OK this time I think all cases are covered. So ... I have worked and tested a lot to finally bring a very simple modification into Symlinking is totally optionnal and you still can download a "latest" artifact without bothering about the filename. This is a silly example :
We get something like :
if we put a symlink in the resource :
We get something like :
Symlink obviously works with non-rolling (ie. static) versions too. I tried to keep the changes very easy to read and to understand. I think it's ok, but feel free to comment, review or modify this PR. Thanks ! |
@@ -37,6 +37,16 @@ def load_current_resource | |||
|
|||
@nexus_configuration = new_resource.nexus_configuration | |||
@nexus_connection = Chef::Artifact::Nexus.new(node, nexus_configuration) | |||
|
|||
if Chef::Artifact.snapshot?(new_resource.location.split(':')[-1]) || Chef::Artifact.latest?(new_resource.location.split(':')[-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should come up with a way to refactor this so that we don't have to do this splitting logic. I think adding a new snapshot?
and latest?
to Chef::Artifact::Nexus
would do the trick - see https://github.com/RiotGames/artifact-cookbook/blob/master/libraries/chef_artifact_nexus.rb#L30-L31 for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll work on this and think about adding new snapshot/latest method directly at Nexus object level.
To piggyback on the discussion in #112, would it make sense that the following example:
Should give you:
I think perhaps that Nexus downloads should again go under your Edit: I'm not sure I fully understand the use case behind the usage of symlink. I think based on the above, the expectation would be that the file's name gets changed and placed in the appropriate directory. Requiring a symlink to get that functionality might be unexpected. |
Overall this has come along nicely. We certainly appreciate all the effort. Would it be possible to squash this whole pull request into a couple of reasonable-sized commits? |
ok, i'll clean this up a bit |
Closing this PR. |
issue #115
This patch allows
artifact_file
corectly downloading *-SNAPSHOT and LATEST version :Anyway, we are now able to download the artifact properly but we still do not know its name. I think it's still very interesting to correct the issue #112