-
Notifications
You must be signed in to change notification settings - Fork 103
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
Various Rubocop and lint fixes #164
Conversation
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.
For some reason Actions didn't run.
I did some initial cleanup of metadata and dropping older versions, but the acceptance tests will fail because they try to install ancient Ruby versions. |
I updated the ruby versions, let's see if this passes now (or at least fails in a different way) |
fa9a74c
to
9261660
Compare
187ba88
to
0be4110
Compare
This Pr now also contains: |
03bbc66
to
4167e8c
Compare
it now passes on centos 7 not nowhere else :( |
Should we mark the Passenger test on Debian/Ubuntu as pending just to get it to pass? |
I think that's a good idea. you want to do that? otherwise I can take a look later. |
I guess neither of us had time for this in the past 3 weeks. |
c7388eb
to
822c1d9
Compare
bb54285
to
2d507c5
Compare
Solaris is not supported by the gnupg module and we cannot test it. cherry-picked from voxpupuli#164
Solaris is not supported by the gnupg module and we cannot test it. cherry-picked from voxpupuli#164
2d507c5
to
e61e009
Compare
This was dropped by accident in af82679
e61e009
to
0240fb2
Compare
$gnupg_key_id = false | ||
} | ||
# sadly the gpg module is ages old and doesn't support long key ids | ||
$gnupg_key_id = [ |
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.
we should probably rename this to $gnupg_key_ids
?
if resource[:proxy_url] && !resource[:proxy_url].empty? | ||
command << '--http-proxy' << resource[:proxy_url] | ||
end | ||
command << '--http-proxy' << resource[:proxy_url] if resource[:proxy_url] && !resource[:proxy_url].empty? |
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 I disagree with Rubocop on this change. It makes the line very long.
before => Exec['system-rvm'], | ||
include gnupg | ||
|
||
# https keys are downloaded with wget |
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.
Should there be a require
on the package somewhere? like gnupg_key
?
r.stdout.should =~ Regexp.new(Regexp.escape(ruby27_version)) | ||
r.stdout.should =~ Regexp.new(Regexp.escape(ruby26_version)) |
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.
Also, much easier is to just use String.include?
instead of using a regex:
r.stdout.should =~ Regexp.new(Regexp.escape(ruby27_version)) | |
r.stdout.should =~ Regexp.new(Regexp.escape(ruby26_version)) | |
expect(r.stdout).to include(ruby27_version) | |
expect(r.stdout).to include(ruby26_version) |
Though the old code was checking that it was at the start of a line with some indenting. That could be more explicit.
My PR #178 is failing due to rubocop, which will be fixed by this PR. @bastelfreak Any help needed? Some mentioned things will be changed by my PR (if accepted). |
I'll merge this now and improve it in a further PR. |
modulesync 4.0.0