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

Add support for puppetfile resolver #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

logicminds
Copy link
Contributor

  • This adds experimental support for the puppetfile resolver.
    Since the resolver is better, faster, stronger it might be
    useful to use this library in the future. A resolve task
    task has been added but it is not clear what that yet provides.

@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.76% 🎉

Comparison is base (eee5054) 65.44% compared to head (80c6504) 66.20%.

❗ Current head 80c6504 differs from pull request most recent head 09f6cef. Consider uploading reports for the commit 09f6cef to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   65.44%   66.20%   +0.76%     
==========================================
  Files          12       13       +1     
  Lines         544      574      +30     
==========================================
+ Hits          356      380      +24     
- Misses        188      194       +6     
Files Changed Coverage Δ
lib/ra10ke/resolver.rb 77.77% <77.77%> (ø)
lib/ra10ke.rb 97.82% <100.00%> (+0.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@logicminds
Copy link
Contributor Author

logicminds commented Apr 22, 2022

Hey @glennsarti ^^

Not sure what exactly the output should look like. Currently looks broken.

This adds a r10k::resolver task which implements your example code.

Example output? https://github.com/voxpupuli/ra10ke/runs/6136277429?check_suite_focus=true

class Instance
attr_reader :puppetfile

def initialize(puppetfile_path = File.expand_path(Dir.pwd))

Choose a reason for hiding this comment

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

Isn't this a directory, not a file? Cant read a dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. why yes that would fail.

# Create the resolver
# - Use the document we just parsed (puppetfile)
# - Don't restrict by Puppet version (nil)
resolver = ::PuppetfileResolver::Resolver.new(puppetfile, nil)

Choose a reason for hiding this comment

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

puppetfile should be @puppetfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an attr_reader for puppetfile

@glennsarti
Copy link

The spec should be:

  it 'resolves the puppetfile' do
   expect(instance.resolve).to be_a(PuppetfileResolver::ResolutionResult)
  end

@glennsarti
Copy link

glennsarti commented Apr 23, 2022

There's a bunch of problems with your fixture Puppetfile (spec/fixtures/Puppetfile)

mod 'puppet-acl',
    git: 'https://github.com/dobbymoodge/puppet-acl.git',
    branch: 'master'

That git URL redirects to https://github.com/voxpupuli/puppet-posix_acl which is actually puppet-posix_acl module.

The gitlab module conflicts with choria, i.e. There is no version of apt that can satisfy both module requirements.

 - `puppetlabs-apt >=1.8.0 <3.0.0` required by `Git vshn-gitlab-1.4.1`
 - `puppetlabs-apt >= 4.5.1 < 9.0.0` required by `Forge choria-choria-0.26.2`

@logicminds
Copy link
Contributor Author

In the test result it shows a stacktrace. Additionally I was not expecting output instead I was hoping to just capture PuppetfileResolver::ResolutionResult and output the results later. So question is why is stuff being thrown to stdout? Guessing this is because of the thrown exception.

The fixtures have issues because they are used to show deprecations and other problems. So they work as designed but have the problems on purpose in order to tell the user to change their Puppetfile. I bet we don't have code that shows the module moved in github though. Would be a nice feature.

Regarding apt issue. I would like to programmatically detect this situation and suggest to the user to change something. I feel like the casual user won't be able to understand the output without some interpretation from Ra10ke.

Aside from this task what else can the resolver gem do that we should add?

1) Ra10ke::Resolver::Instance resolves the puppetfile
     Failure/Error: result = resolver.resolve(opts)

     PuppetfileResolver::Puppetfile::DocumentVersionConflictError:
       Puppetfile Resolver could not find compatible versions for possibility named "acl":
         In Puppetfile:
           puppet-acl >= 0

       Puppetfile Resolver could not find compatible versions for possibility named "inifile":
         In Puppetfile:
           -r10k >= 0 was resolved to Git zack-r10k-3.1.1, which depends on
             puppetlabs-inifile >= 1.0.0

           puppetlabs-inifile =2.2.0

       Puppetfile Resolver could not find compatible versions for possibility named "stdlib":
         In Puppetfile:
           -gms >= 0 was resolved to Git abrader-gms-1.0.1, which depends on
             puppetlabs-stdlib >=3.2.0 <5.0.0

           -r10k >= 0 was resolved to Git zack-r10k-3.1.1, which depends on
             puppetlabs-stdlib >= 4.6.0

           -gitlab >= 0 was resolved to Git vshn-gitlab-1.4.1, which depends on
             puppetlabs-stdlib 4.x

           puppet-archive =3.1.1 was resolved to Forge puppet-archive-3.1.1, which depends on
             puppetlabs-stdlib >= 4.13.1 < 5.0.0

           puppetlabs-ntp =6.4.1 was resolved to Forge puppetlabs-ntp-6.4.1, which depends on
             puppetlabs-stdlib >= 4.13.1 < 5.0.0

           puppetlabs-concat =4.0.0 was resolved to Forge puppetlabs-concat-4.0.0, which depends on
             puppetlabs-stdlib >= 4.13.1 < 5.0.0

           puppetlabs-stdlib =4.24.0

           puppetlabs-ruby =1.0.1 was resolved to Forge puppetlabs-ruby-1.0.1, which depends on
             puppetlabs-stdlib >= 4.13.1 < 7.0.0

           choria-choria =0.26.2 was resolved to Forge choria-choria-0.26.2, which depends on
             puppetlabs-stdlib >= 4.24.0 < 8.0.0
     # ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolution.rb:317:in `raise_error_unless_state'
     # ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolution.rb:299:in `block in unwind_for_conflict'
     # <internal:kernel>:90:in `tap'
     # ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolution.rb:297:in `unwind_for_conflict'
     # ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolution.rb:257:in `process_topmost_state'
     # ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolution.rb:182:in `resolve'
     # ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolver.rb:43:in `resolve'
     # ./vendor/bundle/ruby/3.0.0/gems/puppetfile-resolver-0.5.0/lib/puppetfile-resolver/resolver.rb:[37](https://github.com/voxpupuli/ra10ke/runs/6136277291?check_suite_focus=true#step:4:37):in `resolve'
     # ./lib/ra10ke/resolver.rb:[49](https://github.com/voxpupuli/ra10ke/runs/6136277291?check_suite_focus=true#step:4:49):in `resolve'
     # ./spec/ra10ke/resolver_spec.rb:18:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # Molinillo::VersionConflict:
     #   Unable to satisfy the following requirements:
     #   
     #   - `puppetlabs-stdlib =4.24.0` required by `Puppetfile`
     #   - `puppetlabs-stdlib >= 4.24.0 < 8.0.0` required by `Forge choria-choria-0.26.2`
     #   - `puppetlabs-stdlib >= 4.13.1 < 7.0.0` required by `Forge puppetlabs-ruby-1.0.1`
     #   - `puppetlabs-stdlib >= 4.13.1 < 5.0.0` required by `Forge puppetlabs-concat-4.0.0`
     #   - `puppetlabs-stdlib >= 4.13.1 < 5.0.0` required by `Forge puppetlabs-ntp-6.4.1`
     #   - `puppetlabs-stdlib >= 4.13.1 < 5.0.0` required by `Forge puppet-archive-3.1.1`
     #   - `puppetlabs-stdlib 4.x` required by `Git vshn-gitlab-1.4.1`
     #   - `puppetlabs-stdlib >= 4.6.0` required by `Git zack-r10k-3.1.1`
     #   - `puppetlabs-stdlib >=3.2.0 <5.0.0` required by `Git abrader-gms-1.0.1`
     #   - `puppetlabs-inifile =2.2.0` required by `Puppetfile`
     #   - `puppetlabs-inifile >= 1.0.0` required by `Git zack-r10k-3.1.1`
     #   - `puppet-acl >= 0` required by `Puppetfile`
     #   ./vendor/bundle/ruby/3.0.0/gems/molinillo-0.8.0/lib/molinillo/resolution.rb:317:in `raise_error_unless_state'

@glennsarti
Copy link

Ahh ok... that makes more sense...

As you guessed. The resolver is throwing an error PuppetfileResolver::Puppetfile::DocumentVersionConflictError and rspec is just dumping out the text.

I basically wrapped the original molinillo error (https://www.rubydoc.info/gems/molinillo/Molinillo/VersionConflict) so all the information is there... but detecting the apt version specification problem is ... tricky. It would probably be possible to detect, but navigating the molinillo trees etc. requires indepth work. No idea how long it would take to figure out.

As for the https://github.com/voxpupuli/puppet-posix_acl issue. That could be detected in the puppetfile resolver.

e.g. given a module name of 'x' and the metadata.json says the module name is 'y'. It could throw an error. In (https://github.com/glennsarti/puppetfile-resolver/blob/main/lib/puppetfile-resolver/models/module_specification.rb#L41)

@bastelfreak
Copy link
Member

@logicminds hey, any chance you could pick this up again? I think this would be a very awesome addition to ra10ke

  * This adds experimental support for the puppetfile resolver.
    Since the resolver is better, faster, stronger it might be
    useful to use this library in the future.  A resolve task
    task has been added but it is not clear what that yet provides.
@kenyon
Copy link
Member

kenyon commented Sep 11, 2023

Rebased on current master (commit eee5054), resolved merge conflicts, and resolved rubocop complaints. I was looking at this because it would be nice to have a Puppetfile dependency resolver that could restrict module versions based on Puppet version.

@bastelfreak
Copy link
Member

@kenyon thanks for the rebase. Any chance you can take a look at the failing unit test?

@kenyon
Copy link
Member

kenyon commented Sep 13, 2023

Well, I realized that since I read all of the module changelogs when I'm updating my Puppetfile, it's really a manual process so a rake task like this isn't useful enough to work on this PR at the moment.

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.

4 participants