Skip to content

Conversation

@cocker-cc
Copy link
Contributor

  • as there is no Function unwrap_if_necessary() we make the Function unwrap()
    more tolerant and let it return a non-sensitive Value just like it is

@cocker-cc cocker-cc requested review from a team June 21, 2021 23:30
@joshcooper
Copy link
Contributor

Hi @cocker-cc, thanks for submitting a pull request! Can you provide more detail about your use case? Is the value originating in hiera or in a different module?

@cocker-cc
Copy link
Contributor Author

Hi @cocker-cc, thanks for submitting a pull request! Can you provide more detail about your use case? Is the value originating in hiera or in a different module?

In the Process of transitioning several Component-Modules to be sensitive-aware, but stay backwards-compatible, I frequently have a Block like:

class blubb(
…
  Variant[String, [Sensitive[String]] $password,
…
) {
  $password_unsensitive = if $password =~ Sensitive {
    $password.unwrap
  } else {
    $password
  }
}

With the suggested Pullrequest this would reduce to just

  $password_unsensitive = $password.unwrap

Copy link
Contributor

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

I also thought about this. It would certainly be useful to be able to treat Sensitive[String] and String the same in code. Right now I consider Sensitive mostly useless and just a painful thing which is why I've never bothered to use it. Being able to say "I need the raw value now, I know what I'm doing" vs "just log what you have and if it's sensitive, obscure the value" can certainly be a step in the right direction.

@joshcooper
Copy link
Contributor

Thanks for your contribution @cocker-cc! Could you file a JIRA ticket in the PUP project and git commit --amend your commit summary with (PUP-XXX) ... This is necessary for our release automation.

Also I wanted to note that Sensitive.new accepts either Sensitive or Any, so it makes sense that its inverse (unwrap) does the same.

@cocker-cc cocker-cc changed the title make Function unwrap more tolerant (PUP-11123) make Function unwrap more tolerant Jun 23, 2021
@cocker-cc
Copy link
Contributor Author

Could you file a JIRA ticket in the PUP project and git commit --amend your commit summary with (PUP-XXX) ... This is necessary for our release automation.

Done.

Also I wanted to note that Sensitive.new accepts either Sensitive or Any, so it makes sense that its inverse (unwrap) does the same.

Great. I copied the Variablenames from there to keep it consistent.

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

LGTM. Note main branch is for 7.x releases. I'd be fine releasing this in 6.x, you'd just need to retarget your PR against the 6.x branch.

@cocker-cc cocker-cc changed the base branch from main to 6.x June 23, 2021 18:54
@cocker-cc
Copy link
Contributor Author

Note main branch is for 7.x releases. I'd be fine releasing this in 6.x, you'd just need to retarget your PR against the 6.x branch.

I rebased it against 6.x and changed the Merge-Target.

- as there is no Function unwrap_if_necessary() we make the Function unwrap()
  more tolerant and let it return a non-sensitive Value just like it is
@joshcooper joshcooper merged commit 359fea6 into puppetlabs:6.x Jun 24, 2021
@ekohl
Copy link
Contributor

ekohl commented Jun 24, 2021

This was merged to 6.x, but it should also be in main. In fact, normally I'd say that it should first be merged to main and then cherry picked to 6.x.

@GabrielNagy
Copy link
Contributor

@ekohl we have automation that promotes/merges up commits from 6.x to main once our internal nightly acceptance CI passes

@joshcooper
Copy link
Contributor

Thanks for your contribution @cocker-cc! This should be released in the next set of releases in July.

@cocker-cc cocker-cc deleted the make_Function_unwrap_more_tolerant branch June 29, 2021 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants