-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
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 |
ca90029
to
78a72bf
Compare
class Instance | ||
attr_reader :puppetfile | ||
|
||
def initialize(puppetfile_path = File.expand_path(Dir.pwd)) |
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.
Isn't this a directory, not a file? Cant read a dir
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.
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) |
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.
puppetfile
should be @puppetfile
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.
There is an attr_reader for puppetfile
The spec should be: it 'resolves the puppetfile' do
expect(instance.resolve).to be_a(PuppetfileResolver::ResolutionResult)
end |
There's a bunch of problems with your fixture Puppetfile ( mod 'puppet-acl',
git: 'https://github.com/dobbymoodge/puppet-acl.git',
branch: 'master' That git URL redirects to The gitlab module conflicts with choria, i.e. There is no version of apt that can satisfy both module requirements.
|
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?
|
Ahh ok... that makes more sense... As you guessed. The resolver is throwing an error 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 e.g. given a module name of 'x' and the metadata.json says the module name is 'y'. It could throw an error. In ( |
@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.
78a72bf
to
5f0f3d8
Compare
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. |
@kenyon thanks for the rebase. Any chance you can take a look at the failing unit test? |
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. |
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.