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

fix bug when using passed in project_dir when using the API directly #302

Closed
wants to merge 1 commit into from

Conversation

logicminds
Copy link
Contributor

When using the API directly for example:

    e = Librarian::Puppet::Environment.new(:project_path => project_path)

The path is ignored in the DSL because of a bug. (https://github.com/logicminds/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L45)

This is easily fixed by traversing the path method.

specfile.path.kind_of?(Pathname)

Additionally, I attempted to create some tests for this but they are incomplete as I am unsure how to create the test with regards to setting a target. Hoping you can help with this so I can finish the tests and get this PR merge and tested.

  rec = Librarian::Puppet::Receiver.new(target)

Since most people never use the API like this this bug has been masked for a while.

carlossg added a commit that referenced this pull request May 19, 2015
@carlossg
Copy link
Collaborator

I have created a test in master da341d5 based on your PR that is succeeding, do you see anything wrong in there?

@carlossg
Copy link
Collaborator

IIUC by the time you call Librarian::Dsl::run you need to pass the specfile location, environment.project_dir is no longer applicable

If you share your code I may understand better how you are trying to call it

@logicminds
Copy link
Contributor Author

Ok here is what I am doing. After reviewing the code some more and your tests the piece I refactored doesn't work in this case and also because Librarian::Specfile.new.kind_of?(Pathname) seems to magically be true and the originally code works as intended. I have no idea why this works since it doesn't extend, include or subclass Pathname.

e = Librarian::Puppet::Environment.new(:project_path => project_path)
Librarian::Puppet::Action::Resolve.new(e, :force => true).run
e.lock.dependencies.map {|d| { d.name => d.source.uri.to_s,
                                   :type => d.source.class,
                                   :version => d.requirement.to_s} }

With regards to Librarian::Puppet::Action::Resolve.new(e, :force => true).run this seems to ignore the project directory. I would prefer not to use this but I don't know the code base that well to understand how to get a list of resolved dependencies without using this method.

@logicminds
Copy link
Contributor Author

I think I found the real issue but I am not sure. It lies a little bit deeper in the code but its reproducible

This code replaces the specfile that contains the passed in project_dir with the default_specfile. Because this default_specfile is a instance of Proc the following code fails to set the proper working directory which now becomes Dir.pwd.
https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L44-L48

So because this is set incorrectly the metadata file cannot be found.
https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L61

Your tests didn't cover some of the super class code which is why they were passing. What really needs to be done is environment.dsl(environment.specfile.path, []) which is equilivent to environment.spec

Its seems like the dependent library librarianp would need to set a propertyin this part(https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L44-L48) then the
following section https://github.com/rodjek/librarian-puppet/blob/master/lib/librarian/puppet/dsl.rb#L61 would need to use this property instead of Dir.pwd as the default.

@carlossg thoughts? I have added a new test case in my tests along with rebasing against master to include your new tests.

You can test this via: bundle exec rspec spec/receiver_spec.rb:28

carlossg added a commit to voxpupuli/librarian that referenced this pull request May 20, 2015
carlossg added a commit that referenced this pull request May 20, 2015
@carlossg
Copy link
Collaborator

Found the issue, and fixed librarianp to make it work, check out the issue-308 branch, you can run the test with

DEBUG=true rspec spec/action/resolve_spec.rb

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