-
Notifications
You must be signed in to change notification settings - Fork 262
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
Issue 748: conflit with gpnupg2 on centos8 #750
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.
Thanks for the PR! The PR code looks good to me but you will need the CI pass so that we can merge this. In particular you need to update the spec tests to account for this change. You can run the tests locally following these guidelines.
Spec file has been updated to reflect the changes. |
spec/repository_spec.rb
Outdated
it 'installs gnupg' do | ||
expect(chef_run).to install_package('gnupg') | ||
end | ||
|
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 still install gnupg
on some situations so we want to keep the spec test. One way to reflect what you did in the code is:
it 'installs gnupg' do
expect(chef_run).to install_package('gnupg') if chef_run.node['packages']['gnupg2'].nil?
end
Would you mind adding back the spec tests following that pattern?
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.
Of course... will do that in the evening...
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.
Done for unit test
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.
Awesome, thanks for taking the time to fix that!
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.
LGTM, thank you again! 🚀
Issue #748