Add test to check tar archive contents

[ehelms]

Aims to allow testing for regressions when certificates are added or removed when aiming to support Katello N-1 support. Additionally aims to detect if new items are added that should be checked for.

[wbclark]

For some reason this is getting the apache cert RPM with release foreman-proxy.example.com-apache-1.0-2.noarch.rpm when all the others have the release 1.0-1

[ehelms]

On Mon, Sep 6, 2021, 5:16 AM Ewoud Kohl van Wijngaarden < @.***> wrote:

@.**** commented on this pull request.

Overall 👍 for testing the tar contents

In spec/support/acceptance/tar.rb https://github.com/theforeman/puppet-certs/pull/381#discussion_r702740674 :

• def command_result

• @command_result ||= @runner.run_command(command)

• end

Since @.*** https://github.com/mizzy/serverspec/commit/2e7e24a90522df8e435a689bd59d110ed56799d3 (included in serverspec 2.41.8) this is technically redundant. You may need to make command protected instead of private. However, we didn't change other ocurrances so I'm fine either way.

In spec/support/acceptance/tar.rb https://github.com/theforeman/puppet-certs/pull/381#discussion_r702741875 :

@@ -0,0 +1,37 @@

+begin

• require 'serverspec'

+rescue LoadError

• Not using acceptance tests

+else

• module Serverspec

• module Type

• class Tar < Command

• def contents

• command_result.stdout

Why do you treat this as a string? Shouldn't you split it into multiple lines? Then you can use its(:contents) { should include('ssl-build/ foreman-proxy.example.com/foreman-proxy.example.com-qpid-router-server.key') }.

Simplicity, mostly. I figured start with what it returns and avoid having to parse the data, do matchers or splits. At the time, I did not figure I gained much by doing an include vs a match.

—

You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/theforeman/puppet-certs/pull/381#pullrequestreview-747006148, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACHT447P56L4FZQKXHZBFDUASBE7ANCNFSM5DNPM5OQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[ekohl]

You can actually do include on a string as well, so even today I'd say that include is better than match. It's a pattern that we use a lot, but IMHO is actually wrong. See https://relishapp.com/rspec/rspec-expectations/v/3-10/docs/built-in-matchers/include-matcher#string-usage as well.

[ekohl]

@wbclark want to take a look as well before merging?

[wbclark]

This looks great to me. I had one remaining question:

Looking at an older $hostname-certs.tar archive from a lab system, it can contain potentially many RPM releases. Now that we include both RPMs as well as unpackaged CA certificates, certificates, and keys, what should be the behavior when using regenerate => true and do we need to test it?

Was having the complete release history of the RPMs ever actually necessary and if so, is it sufficient in the newer design to have the versioned RPMs alongside only the unversioned (latest) raw certificates, keys, etc?

[ehelms]

So regenerate will force recreate everything -- certificates, keys and thus the RPMs. They all end up living in the same directory as nothing gets cleaned up. This translates to being put into the new tarball that gets generated as well. That being said, we could look into in the future some pruning or creating of that tarball smarter. For this change, and the change to deploy certs from the build directory rather than the RPMs I do not think any changes are necessary at present.