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 possibility to stop recursion on gnupghome #290

Merged
merged 17 commits into from
Oct 17, 2020
Merged

Add possibility to stop recursion on gnupghome #290

merged 17 commits into from
Oct 17, 2020

Conversation

bahner
Copy link
Contributor

@bahner bahner commented Oct 9, 2020

Pull Request (PR) description

When puppet runs it gets an error that it can't handle sockets. Since gpg-agent is integral to gpg now,
that is an error that is unavoidable with thee current setup.
If you do this manually, ie. chmod -r 0600 $GNUPGHOME, then there is no need to run this and get an error
every run.

@bahner
Copy link
Contributor Author

bahner commented Oct 9, 2020

If this pull request gets accepted in the end, it should probably be rebased onto master.

@bahner
Copy link
Contributor Author

bahner commented Oct 9, 2020

So I changed a lot off indentation and layout. But how on earth have anyone else been able to get travis checks to pass, I wonder. Well now it works. Now rebase and merge, pretty please :-)

README.md Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
@bastelfreak
Copy link
Member

@bahner thanks for the work! a linter plugin was recently updated and is now stricter. Had had not enough time to fix this on all of our modules. I made two inline comments, please have a look.

@bastelfreak bastelfreak added bug Something isn't working needs-feedback Further information is requested labels Oct 9, 2020
@bastelfreak bastelfreak added needs-tests and removed needs-feedback Further information is requested labels Oct 10, 2020
@bastelfreak
Copy link
Member

@bahner thanks for the updates. can you add a few tests for this?

@bahner
Copy link
Contributor Author

bahner commented Oct 11, 2020

I am confused as how to run the unit tests the way things are set up. (I'm a n00b and only use pdk test unit for now). I can't really figure out how to get things setup for running test here. I'm guessing rake test, but that does definitely not work. How should test be invoked correctly?

I found some checks missing from rake runs, and fixed
them. Doing this I mangled hiera_spec.rb to achieve
consistent naming of keysdir. When I started I used
default and that caused 3 different namings of the
same directory. That seems silly.

The reason it ended up being /dev/null/keys is that
theam YAML eyaml loader sets confdir to "/dev/null"
The string "/dev/null" is not mentioned anywhere in
the code. So I went with it and changed keysdir to
/dev/null/keys instead of /etc/keys. This is a small
change, that increases the coverage percentage when
I check some file in the eyaml_gpg_spec.rb.

Also added a check for the deep_merge class, just
becasue it was easy and on my way. I am getting the
hang of this now - but if you would ask me what I
have done and how, I would have to do answer in a
Mr. Miagi-style.

  Don't know. Firsta time.
@bahner
Copy link
Contributor Author

bahner commented Oct 12, 2020

Well, that was a mouthful. As stated, please rebase and squash this onto master, when ready. The git log is way too messy.

@kenyon
Copy link
Member

kenyon commented Oct 12, 2020

I am confused as how to run the unit tests the way things are set up. (I'm a n00b and only use pdk test unit for now). I can't really figure out how to get things setup for running test here. I'm guessing rake test, but that does definitely not work. How should test be invoked correctly?

You can run the tests locally like this: bundle exec rake rubocop

Only let key be used for (de)encryption.
A key without passphrase shouldn't sign anything
or be used to authenticate anything. You can trust
it to decrypt messages destined to be decrypted
by it, but you can never trust anything that is
signed by it.
My bad, this has nothing to do in this pull request.
This reverts commit 1871e63.
describe 'param manage_package => false' do
let(:params) do
{
eyaml: true,
eyaml_gpg: true,
manage_package: false,
keysdir: '/etc/keys'
keysdir: '/dev/null/keys'
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason for changing the parameter here and a few lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. For coverage. The same dir "keys" had 3 different names. "So just to use the same dir in all tests, so that they are easier to write. It's very confusing, when the same dir has different paths. I wrote an explanation in the commit, but the commit message drown a bit here.

The coverage percentage gives a more correct measurement of how tested the module is now. The percentage is now around 67% instead of the 30 something, that was reported. Obviously /etc/keys is also just a fake dir and /dev/null van't (easily) be changed. It doesn't come from the code, but most likely from YAML.load(:eyaml). But the path I can't find defined anywhere.

@bastelfreak bastelfreak merged commit 86dbcd2 into voxpupuli:master Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants