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

Let Function to_symbolized_yaml handle Datatype Sensitive #972

Merged
merged 1 commit into from
Jul 22, 2021
Merged

Let Function to_symbolized_yaml handle Datatype Sensitive #972

merged 1 commit into from
Jul 22, 2021

Conversation

cocker-cc
Copy link
Contributor

No description provided.

@cocker-cc
Copy link
Contributor Author

This is a simplified Version, of what I recently did here.

@ehelms ehelms requested a review from ekohl July 19, 2021 13:29
@ehelms ehelms mentioned this pull request Jul 19, 2021
5 tasks
Copy link
Member

@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.

Overall I think this looks correct, but is it possible to write a test that it returns a sensitive?

@cocker-cc
Copy link
Contributor Author

Overall I think this looks correct, but is it possible to write a test that it returns a sensitive?

I guess the Test is implicite with this Line:

expect(subject.execute({'a' => sensitive('b')}).unwrap).to eq("---\n:a: b\n")

If the Returnvalue wasn't Sensitive the unwrap would fail.

@ekohl
Copy link
Member

ekohl commented Jul 20, 2021

For now. However, now that puppetlabs/puppet#8651 is merged is that still true? My initial reading suggests it's still true, bu I must admit that I have some doubts about the implicit testing. Another way to ensure would be testing that executing with non-sensitive values doesn't respont to unwrap.

Another possible test is passing a hash with one sensitive value and one non-sensitive value. That should still return a sensitive.

@ekohl ekohl merged commit 7996011 into theforeman:master Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants