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 stringData or encodedData on secrets #282

Merged
merged 4 commits into from
Dec 27, 2023
Merged

allow stringData or encodedData on secrets #282

merged 4 commits into from
Dec 27, 2023

Conversation

emmanuelm41
Copy link
Contributor

@emmanuelm41 emmanuelm41 commented Oct 12, 2023 •
edited
Loading

In this way, secrets will be able to set string values, instead of base64 only

@github-actions
Copy link

@emmanuelm41 validation successful`

@rasheedamir
Copy link
Member

@d3adb5 @aslafy-z please review it

d3adb5
d3adb5 requested changes Oct 13, 2023
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @emmanuelm41! I added a few comments below.

application/templates/secret.yaml Outdated Show resolved Hide resolved
application/templates/secret.yaml Outdated Show resolved Hide resolved
application/templates/secret.yaml Outdated Show resolved Hide resolved
application/templates/secret.yaml Show resolved Hide resolved
@emmanuelm41
Copy link
Contributor Author

@d3adb5 I just applied a few changes, and add a couple of unit tests. Could you please give me your feedback now?

d3adb5
d3adb5 requested changes Oct 14, 2023
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

I left a suggestion and a request for change.

application/values-test.yaml Outdated Show resolved Hide resolved
application/templates/secret.yaml Outdated Show resolved Hide resolved
@d3adb5
Copy link
Collaborator

d3adb5 commented Oct 24, 2023

Hi @emmanuelm41, have you had the chance to review my comments? If anything's unclear, please let me know so I can help clarify what I said.

@emmanuelm41 emmanuelm41 force-pushed the feat/secret-stringData branch 2 times, most recently from 6a860ae to 8f05e67 Compare October 25, 2023 22:01
@emmanuelm41
Copy link
Contributor Author

Hi @emmanuelm41, have you had the chance to review my comments? If anything's unclear, please let me know so I can help clarify what I said.

Sorry for my late response. I just applied your suggestion. Please, let's review it and see if we can merge. Thanks!

@emmanuelm41 emmanuelm41 changed the title allow stringData on secrets allow stringData or encodedData on secrets Oct 25, 2023
d3adb5
d3adb5 previously approved these changes Oct 26, 2023
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

Left a comment about whitespace-only changes, but otherwise this looks good to me! Thank you, @emmanuelm41!

application/values-test.yaml Outdated Show resolved Hide resolved
d3adb5
d3adb5 previously approved these changes Oct 28, 2023
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

LGTM!

@d3adb5
Copy link
Collaborator

d3adb5 commented Nov 4, 2023

@rasheedamir, @aslafy-z, can anybody else take a quick look and figure out if we can merge this?

@karl-johan-grahn
Copy link
Contributor

@emmanuelm41 can you please resolve the merge conflicts?

@emmanuelm41
Copy link
Contributor Author

@karl-johan-grahn Done!

@mustafaStakater
Copy link
Contributor

mustafaStakater commented Nov 23, 2023 •
edited
Loading

Update the README.md and CHANGELOG

@karl-johan-grahn
Copy link
Contributor

@emmanuelm41 can you please look into resolving the comments and merge conflicts?

@emmanuelm41
Copy link
Contributor Author

@emmanuelm41 can you please look into resolving the comments and merge conflicts?

Everything should be covered now!

d3adb5
d3adb5 previously approved these changes Dec 26, 2023
View reviewed changes
Copy link
Collaborator

@d3adb5 d3adb5 left a comment

Choose a reason for hiding this comment

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

This looks good to me. I left a comment about whitespace changes in the chart's values file, but it could be considered nitpicking a little.

application/values.yaml Outdated Show resolved Hide resolved
mustafaStakater
mustafaStakater reviewed Dec 27, 2023
View reviewed changes
Copy link
Contributor

@mustafaStakater mustafaStakater left a comment

Choose a reason for hiding this comment

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

Looks good @emmanuelm41 , is it possible to resolve @d3adb5 last comment, and we can proceed with merging this

@emmanuelm41
Copy link
Contributor Author

Looks good @emmanuelm41 , is it possible to resolve @d3adb5 last comment, and we can proceed with merging this

Resolved!

mustafaStakater
mustafaStakater approved these changes Dec 27, 2023
View reviewed changes
Copy link
Contributor

@mustafaStakater mustafaStakater left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the contribution @emmanuelm41 – your efforts are appreciated 👍

@mustafaStakater mustafaStakater merged commit 2165b37 into stakater:master Dec 27, 2023
2 checks passed
@emmanuelm41 emmanuelm41 deleted the feat/secret-stringData branch December 27, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mustafaStakater mustafaStakater mustafaStakater approved these changes

@aslafy-z aslafy-z Awaiting requested review from aslafy-z

@d3adb5 d3adb5 Awaiting requested review from d3adb5

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

Successfully merging this pull request may close these issues.
5 participants
@emmanuelm41 @rasheedamir @d3adb5 @karl-johan-grahn @mustafaStakater