Skip to content

Option to disable display of diff in the puppet log#276

Open
teluq-pbrideau wants to merge 4 commits into
voxpupuli:masterfrom
teluq-pbrideau:feat/show_diff
Open

Option to disable display of diff in the puppet log#276
teluq-pbrideau wants to merge 4 commits into
voxpupuli:masterfrom
teluq-pbrideau:feat/show_diff

Conversation

@teluq-pbrideau

@teluq-pbrideau teluq-pbrideau commented Sep 29, 2022

Copy link
Copy Markdown

Pull Request (PR) description

In addition to #275
There is conditions where the sensitive information is displayed in the logs. This add option to completely disable output in the logs.

class example {
  class { 'yum' :
    show_diff => false,
  }
}

Tests

I’m not a pro in the tests suite alley, feel free to comment on any other way I could have fixed the tests… It took me way longer to fix the tests than to add the new config.

As the yum::config now includes yum to retreive yum::show_diff, the tests are done on every supported OS for the variables to be set correctly.

I had to modify the versionlock tests to make them pass. The diff is quite hard to read, so here the summary of what I did:

  • The tests are now run on every supported OS.
    - I moved the context 'with a simple, well-formed package name title bash and a version' in the context 'with package_provider set to yum' as they were duplicated facts definition edit: did not fix this in my new rebase
  • I merge the fact package_provider with the facts provided by every OS

@teluq-pbrideau teluq-pbrideau changed the title Feat/show diff Option to disable display of diff in the puppet log Oct 6, 2022
@jhoblitt jhoblitt added the enhancement New feature or request label Nov 8, 2022

@jhoblitt jhoblitt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This LGTM but I would like a second opinion from a maintainer with more experience with this module.

@teluq-pbrideau

teluq-pbrideau commented Jul 26, 2023

Copy link
Copy Markdown
Author

I’ve merged the changes in master here, but the static validation test fail, is it because of REFERENCE.md is outdated ? How can I make this test pass? I've generated the REFERENCE.md file with puppet strings generate --format markdown successfully

@teluq-pbrideau

Copy link
Copy Markdown
Author

Here, I’ve successfully merged and successfully make tests pass again. Can we move forward with this please?

Comment thread spec/defines/config_spec.rb Outdated
@teluq-pbrideau

Copy link
Copy Markdown
Author

Maybe I could add a notice in the README about the risks of using sensitive parameters, as noted on my original issue? What do you think?

The password is not censored when another config is changed:

--- /etc/yum.conf       2022-09-28 10:53:13.958280359 -0400
+++ /etc/yum.conf.augnew        2022-09-28 11:44:01.581689900 -0400
@@ -10,5 +10,5 @@
 metadata_expire=0
 mirrorlist_expire=0
 proxy=http://host.example.com:3128
-proxy_username=user
+proxy_username=anotheruser
 proxy_password=mysecretpassword

Notice: /Stage[main]/Yum/Yum::Config[proxy_username]/Augeas[yum.conf_proxy_username]/returns: executed successfully (corrective)

It might lure people in a false sense of security by using Sensitive parameters...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants