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

feat: Add string concatenation for secrets #2841

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ravishankar15
Copy link
Contributor

@ravishankar15 ravishankar15 commented Feb 26, 2025

PR Description

Adds string concatenation for the secrets type

Which issue(s) this PR fixes

Fixes #1514

Note to reviewers

I am not sure why we have secrets and optionalsecrets. I believe secrets was the older implementation. Hence i skipped adding this functionality on the secrets type. Let me know if we have to add this to the secrets type it is easily extendable.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@ravishankar15 ravishankar15 requested a review from a team as a code owner February 26, 2025 08:31
Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, I think that this is the right approach and it works well for optional secrets.
We do still have values of the type "Secret", for example the remote.value component will export secrets and not optional secrets. It would be nice to also have concatenation for secrets and not only optional secrets. optional secret + secret = secret and string + secret = secret.
The type casts and switch nesting make the code hard to follow and I'm afraid that it might become even harder with the secret extension. Maybe you can cast the types at the beginning of the map function:

lhsOptSecret, lhsIsOptSecret := lhs.Interface().(alloytypes.OptionalSecret)
lhsSecret, lhsIsSecret := lhs.Interface().(alloytypes.Secret)
etc...

That's just an idea, if you find a better way please go ahead.
One last little thing: can you update the doc here with Concatenate strings/secrets

if rhs.Type() == value.TypeCapsule {
rhs = tryUnwrapOptionalSecret(rhs)
if lhs.Type() == value.TypeCapsule || rhs.Type() == value.TypeCapsule {
lhs, rhs = mapCapsules(lhs, rhs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a more precise name like the one before, maybe "tryUnwrapSecrets"

@ravishankar15
Copy link
Contributor Author

I see your thought about nesting getting complicated. I wanted to make this implementation little generic so that it can be extended to any type of capsules in future. I have split the handling to separate functions with comments. Let me know if this looks over engineered and we wanted to stick to handling only secrets for now.

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.

Use string concatenation (+) on secrets
2 participants