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

Unseal templates even when encryptedData is empty #653

Merged
merged 2 commits into from
Jan 27, 2022

Conversation

twz123
Copy link
Contributor

@twz123 twz123 commented Oct 21, 2021

Previously, SealedSecrets with empty encryptedData fields but with non-empty template.data fields were unsealed into empty Secrets. Change this by removing the check for non-empty encryptedData fields.

@KarstenSiemer
Copy link

KarstenSiemer commented Oct 29, 2021

@twz123 nice pr but looks like no one from bitnami will review

@alvneiayu
Copy link
Collaborator

hi @twz123

Let me investigate a little bit to understand the change in the code and I will review it. I am going to be one of the maintainer in charge of the review. Sorry for the delay

Thanks a lot

Álvaro

@alvneiayu
Copy link
Collaborator

hi @twz123

Could you do a rebase, please? The PR was created before to fix the pipeline and the checks jobs are blocked. Just to verify that we are not breaking anything.

Thanks

Álvaro

Introduce some utility functions to make tests more concise.
Previouly, SealedSecrets with empty encryptedData fields but with
non-empty template.data fields were unsealed into empty Secrets. Change
this by removing the check for non-empty encryptedData fields.
@twz123 twz123 force-pushed the templates-with-empty-encrypted-data branch from 62d2fa1 to 89dbfc8 Compare January 13, 2022 10:15
@twz123 twz123 requested a review from mkmik as a code owner January 13, 2022 10:15
@twz123
Copy link
Contributor Author

twz123 commented Jan 13, 2022

@alvneiayu rebased as requested

Copy link
Collaborator

@alvneiayu alvneiayu left a comment

Choose a reason for hiding this comment

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

LGTM, do you have any concern about this @mkmik @juan131 @agarcia-oss?

Copy link
Collaborator

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome, thanks so much for this great PR

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.

5 participants