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.

Sign up for GitHub

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

Jump to bottom

Allow content parameter of concat_fragment to be Sensitive #757

Merged
merged 2 commits into from
Apr 11, 2023
Merged

Allow content parameter of concat_fragment to be Sensitive #757

merged 2 commits into from
Apr 11, 2023

Conversation

baurmatt
Copy link
Contributor

Fixes #742.

@baurmatt baurmatt requested a review from a team as a code owner March 10, 2023 11:41
@puppet-community-rangefinder
Copy link

concat_file is a type

Breaking changes to this file WILL impact these 1 modules (exact match):
Breaking changes to this file MAY impact these 2 modules (near match):

concat_fragment is a type

Breaking changes to this file WILL impact these 8 modules (exact match):
Breaking changes to this file MAY impact these 12 modules (near match):

concat::fragment is a type

Breaking changes to this file WILL impact these 539 modules (exact match):
Breaking changes to this file MAY impact these 101 modules (near match):

This module is declared in 169 of 580 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@baurmatt baurmatt mentioned this pull request Mar 10, 2023
Merged
ekohl
ekohl reviewed Mar 10, 2023
View reviewed changes
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Recently I've been thinking about a type alias Stdlib::File::Content that's an alias for what the file type accepts for the content parameter so I opened puppetlabs/puppetlabs-stdlib#1298

manifests/fragment.pp Outdated Show resolved Hide resolved
@ekohl
Copy link
Collaborator

ekohl commented Mar 10, 2023

I think dropping Puppet 5 compatibility code deserves at least a mention in the changelog. I'd suggest to submit that as its own PR for visiblity and then rebase this once it's merged.

@baurmatt
Copy link
Contributor Author

I don't think it's worth the trouble creating two separate MRs, rebased, ... Officially, Puppet 5 support has been drop with #685. This just manifests it.

@LukasAud
Copy link
Contributor

Hey @baurmatt, thanks for taking the time to contribute to our module. Regarding the Puppet 5 code removal topic, while it is true that it has been deprecated for a while, we still prefer (where possible) to have changes properly separated and documented (title and description) in different PRs. This helps us and the rest of the community keep better track of changes being made. Otherwise, pinpointing certain changes can often become a quite challenging.

We would appreciate if you could push the Puppet 5 code removal changes in a different PR. Sorry for the inconvenience.

@b4ldr
Copy link
Collaborator

b4ldr commented Mar 15, 2023

i have created #761 for the puppet5 removal, once that's merged this should be able to be rebased

@LukasAud
Copy link
Contributor

#761 has been merged. As soon as this one is rebased and conflicts are resolved, it should be good to merge too.

@alexjfisher
Copy link
Contributor

I'm still not convinced the r[:content].unwrap line is reachable though. In other types, where I've had a property/parameter that was a hash and within that there's a sensitive, I'd have to unwrap it, but otherwise I think it's automagic???

@baurmatt
Copy link
Contributor Author

baurmatt commented Apr 3, 2023

Sorry, got sick :( Yeah, seems like this isn't needed. I've removed the unwrap part.

LukasAud
LukasAud approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@LukasAud LukasAud left a comment

Choose a reason for hiding this comment

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

LGTM

@LukasAud LukasAud merged commit 1762b4d into puppetlabs:main Apr 11, 2023
@natemccurdy
Copy link

Thanks for adding Sensitive support! But it appears that the entire content has to be wrapped in Sensitive. It's not possible to use epp() to render the content where certain variables contain Sensitive values.

Opened a bug here: https://tickets.puppetlabs.com/browse/MODULES-11429

For example, this doesn't work as expected. A warning is thrown and the sensitive content is not redacted:

concat::fragment { 'foo':
  target  => $foo,
  order   => '01',
  content => epp('module/template.epp',
    {
      'password' => Sensitive($password),
      'other'.   => $other,
    }
  ),
}

Warning: /Concat_fragment[foo]: Unable to mark 'content' as sensitive: content is a parameter and not a property, and cannot be automatically redacted.

@baurmatt
Copy link
Contributor Author

baurmatt commented May 5, 2023

@natemccurdy Thanks for bringing this up! I wasn't able to reproduce this with version 7.4.0, but with 8.0.0. So something must have been broken with the latest version.

(Talking about concat version, not Puppet)

@baurmatt
Copy link
Contributor Author

baurmatt commented May 5, 2023

This seems to be broken with bc6478a#diff-176bed02b2636bba558658d6d610f63f1c894e45cfa6d7b54e8257cc57ba5d49L98

Ping @LukasAud :)

@alexjfisher
Copy link
Contributor

oh, you definitely can't just rename that function just because rubocop doesn't like it beginning with set_...

@alexjfisher
Copy link
Contributor

I'll open a PR.

@alexjfisher
Copy link
Contributor

Fixed in #777

@LukasAud Are there any other modules where a similar change might have slipped in?

@alexjfisher
Copy link
Contributor

Maybe remove this https://github.com/puppetlabs/puppetlabs-vcsrepo/blob/main/.rubocop_todo.yml#L45 and explicitly disable the cop on the offending line

@ekohl
Copy link
Collaborator

ekohl commented May 5, 2023

@LukasAud Are there any other modules where a similar change might have slipped in?

puppetlabs/puppetlabs-postgresql#1433 was exactly the same thing.

@alexjfisher
Copy link
Contributor

https://github.com/puppetlabs/puppetlabs-chocolatey/blob/bc40015f36960b241651953eaa9aa832a183cbcc/.rubocop_todo.yml#L59 also maybe an accident waiting to happen

@alexjfisher
Copy link
Contributor

Good news is I've not been able to find any other modules that have the breaking change in them.

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

@b4ldr b4ldr b4ldr left review comments

@ekohl ekohl ekohl left review comments

@alexjfisher alexjfisher alexjfisher left review comments

@LukasAud LukasAud LukasAud approved these changes

Assignees
No one assigned
Labels
bugfix community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for content with sensitive type
7 participants
@baurmatt @ekohl @LukasAud @b4ldr @alexjfisher @natemccurdy @ia-content