-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
chart: incrementals-publisher unit tests for secrets #1077
chart: incrementals-publisher unit tests for secrets #1077
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sarathchandra24 thanks for this PR.
In order to help maintainers reviewing changes, it is always needed to either:
- Link to an issue where the problem is described and eventually discussed
- Or at leas set both the PR title and body to describe the problem you want to solve (e.g. "why did you open this PR?").
I'm blocking this PR because it is not clear which problem do you want to solve (e.g. what is the value of this PR)?
Technically it is neither wrong or right. It might work, or not, but is it justified or needed? And why?
Note: changing the default values of an helm chart is always a breaking change and must be treated as such.
I specified the issue in the comment above at the end. May be it is not visible. Here is the issue to which this PR is linked. I will forward the discussion to the issue. |
My apologies, I missed the link to the issue which was the last item in the body. Thanks for the reminder. Let's continue in the issue then! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: you might want to take care of following the POSIX rules regarding end of files.
The current git diff shows no empty line at the end of files:
See https://thoughtbot.com/blog/no-newline-at-end-of-file to have more informations. Usually you can set your IDE/text editor to automatically add these end of file empty characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new version looks really good!
I've added suggestions (2 are blocking + 1 typo) but it looks almost ready!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Unit tests for verifying secrets values and their assignment to deployment env.
I believe this is backward compatible as only the unit tests and values are changes.
returns the following
#656