Added option for custom environment variables for PostgreSQL. #7589

bokysan · 2018-09-06T21:47:47Z

This commit enables the user to specify additional environment
variables in the stable/postgres chart.

What this PR does / why we need it:
This is especially important for custom Postgres images which might have different environment variables for different options or for enabling additional features.

Special notes for your reviewer:
Nothing special here, really, the same pattern for adding variables is also used in other charts.

cpanato · 2018-09-06T21:49:36Z

@bokysan please bump the chart version

bokysan · 2018-09-06T21:52:27Z

Version bumped.

stable/postgresql/values.yaml

stable/postgresql/Chart.yaml

desaintmartin · 2018-09-14T10:47:25Z

stable/postgresql/values.yaml

@@ -3,7 +3,7 @@ image: "postgres"
 ## postgres image version
 ## ref: https://hub.docker.com/r/library/postgres/tags/
 ##
-imageTag: "9.6.2"
+imageTag: "10.5"


You are changing the postgres version here, which seems out of the scope of this PR.

I agree. But then again so should changing the verison of the helm chart.

Happy to revert, but frankly:

I'm not sure anymore what should be the scope and what not

I have tested the chart with this new version and does work OK.

I'd kindly ask that this PR finally gets merged, since it works and it's an improvement given current situation either way.

It would be healthier to separate concerns and do two differents PRs. A lot of people are cherry-picking commits and may see their postgres version change without realizing (no hint from commit message).

Although I am happy with both changes. :)

What if we just change the commit message then?

I'm personally against this, but let's see other people's point of view. It is easy to do two different PRs for different concerns.

bokysan · 2018-09-18T08:47:24Z

Hello gentlemen,

There were some comments from @cpanato, which were resolved. We had a discussion about raising a postgres version with @desaintmartin, which I would leave as is (and maybe fix the commit message).

What do we do about this pull request now? Any possibility of merging it?

cpanato · 2018-09-18T10:45:17Z

/lgtm

cpanato · 2018-09-18T10:55:17Z

/lgtm cancel

cpanato · 2018-09-18T10:56:49Z

@bokysan talking with @desaintmartin the bump the Postgres version will be good in another PR. it is just my opinion as re-thinking this bump. Since this is a bump to a major version of Postgres maybe will be good to have in another pr

bokysan · 2018-09-19T10:11:26Z

I have rolled back the PostgreSQL version upgrade change.

I'll open a separate PR once this one gets merged.

cpanato · 2018-09-27T10:42:03Z

@bokysan rebase and lets try to get this merged!

bokysan · 2018-10-02T06:45:36Z

@bokysan rebase and lets try to get this merged!

I'm sorry, I don't follow -- this has been rebased into one commit from the start and with every change we did. Everything is in commit 19b6e55751a2c12fce08f9a3be9820211ab0f19d. Do you need me to do anything else?

cpanato · 2018-10-02T17:56:13Z

@bokysan you need to rebase and then bump the chart version

bokysan · 2018-10-02T18:31:16Z

@bokysan you need to rebase

I've rebased everything onto charts:master.

and then bump the chart version

Version has already been bumped to 1.0.0 since Sep 6, 2018 at your request on that day.

desaintmartin · 2018-10-03T07:23:18Z

Hm, I think you merged some branch into some another.

bokysan · 2018-10-03T12:15:00Z

Hi,

@desaintmartin, @cpanato. I've reverted the rebase onto master. As we apparently don't understand each other, could you kindly spell it out for me (or direct me to a web page) what exactly you need me to do to get this merged?

Thank you,
B

desaintmartin · 2018-10-04T10:10:25Z

You should do a "rebase", i.e
git fetch upstream && git rebase upstream/master && git push -f origin master, upstream being this repository and origin your fork.
https://git-scm.com/book/en/v2/Git-Branching-Rebasing
The goal is for your branch to be up-to-date AND to have only your changes in this PR, nothing else.

cpanato · 2018-10-04T10:42:39Z

yeah, you should do a rebase, usually I add the -i to make interactive git rebase -i upstream/master

bokysan · 2018-10-08T05:55:19Z

Ok, done. I don't know how I was able to pull all the changes in the last time.

cpanato · 2018-10-08T11:12:48Z

/ok-to-test

This commit enables the user to specify additional environment variables in the postgres chart. This is especially important for custom Postgres images which might have different environment variables for different options or for enabling additional features. Furthermore versions have been updated: - Chart version has been set to 1.0.0, as this is a stable chart Signed-off-by: Bojan Čekrlić <[email protected]>

mattfarina · 2018-10-16T14:53:35Z

/lgtm

k8s-ci-robot · 2018-10-16T14:53:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bokysan, mattfarina

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattfarina]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) This commit enables the user to specify additional environment variables in the postgres chart. This is especially important for custom Postgres images which might have different environment variables for different options or for enabling additional features. Furthermore versions have been updated: - Chart version has been set to 1.0.0, as this is a stable chart Signed-off-by: Bojan Čekrlić <[email protected]>

) This commit enables the user to specify additional environment variables in the postgres chart. This is especially important for custom Postgres images which might have different environment variables for different options or for enabling additional features. Furthermore versions have been updated: - Chart version has been set to 1.0.0, as this is a stable chart Signed-off-by: Bojan Čekrlić <[email protected]> Signed-off-by: Jakob Niggel <[email protected]>

) This commit enables the user to specify additional environment variables in the postgres chart. This is especially important for custom Postgres images which might have different environment variables for different options or for enabling additional features. Furthermore versions have been updated: - Chart version has been set to 1.0.0, as this is a stable chart Signed-off-by: Bojan Čekrlić <[email protected]>

k8s-ci-robot requested review from linki and mattfarina September 6, 2018 21:47

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 6, 2018

ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 6, 2018

cpanato suggested changes Sep 6, 2018

View reviewed changes

stable/postgresql/values.yaml Outdated Show resolved Hide resolved

stable/postgresql/Chart.yaml Outdated Show resolved Hide resolved

ey-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Sep 7, 2018

gkarthiks mentioned this pull request Sep 8, 2018

Update postgres to use statefulset? #809

Closed

desaintmartin reviewed Sep 14, 2018

View reviewed changes

k8s-ci-robot assigned cpanato Sep 18, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 19, 2018

ey-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Oct 2, 2018

ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Oct 3, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 8, 2018

k8s-ci-robot assigned mattfarina Oct 16, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2018

k8s-ci-robot merged commit 65403e8 into helm:master Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option for custom environment variables for PostgreSQL. #7589

Added option for custom environment variables for PostgreSQL. #7589

bokysan commented Sep 6, 2018

cpanato commented Sep 6, 2018

bokysan commented Sep 6, 2018

desaintmartin Sep 14, 2018

bokysan Sep 14, 2018

desaintmartin Sep 14, 2018

bokysan Sep 14, 2018

desaintmartin Sep 18, 2018

bokysan commented Sep 18, 2018

cpanato commented Sep 18, 2018

cpanato commented Sep 18, 2018

cpanato commented Sep 18, 2018

bokysan commented Sep 19, 2018

cpanato commented Sep 27, 2018

bokysan commented Oct 2, 2018

cpanato commented Oct 2, 2018

bokysan commented Oct 2, 2018 •

edited

Loading

desaintmartin commented Oct 3, 2018

bokysan commented Oct 3, 2018

desaintmartin commented Oct 4, 2018

cpanato commented Oct 4, 2018

bokysan commented Oct 8, 2018

cpanato commented Oct 8, 2018

mattfarina commented Oct 16, 2018

k8s-ci-robot commented Oct 16, 2018

Added option for custom environment variables for PostgreSQL. #7589

Added option for custom environment variables for PostgreSQL. #7589

Conversation

bokysan commented Sep 6, 2018

cpanato commented Sep 6, 2018

bokysan commented Sep 6, 2018

desaintmartin Sep 14, 2018

Choose a reason for hiding this comment

bokysan Sep 14, 2018

Choose a reason for hiding this comment

desaintmartin Sep 14, 2018

Choose a reason for hiding this comment

bokysan Sep 14, 2018

Choose a reason for hiding this comment

desaintmartin Sep 18, 2018

Choose a reason for hiding this comment

bokysan commented Sep 18, 2018

cpanato commented Sep 18, 2018

cpanato commented Sep 18, 2018

cpanato commented Sep 18, 2018

bokysan commented Sep 19, 2018

cpanato commented Sep 27, 2018

bokysan commented Oct 2, 2018

cpanato commented Oct 2, 2018

bokysan commented Oct 2, 2018 • edited Loading

desaintmartin commented Oct 3, 2018

bokysan commented Oct 3, 2018

desaintmartin commented Oct 4, 2018

cpanato commented Oct 4, 2018

bokysan commented Oct 8, 2018

cpanato commented Oct 8, 2018

mattfarina commented Oct 16, 2018

k8s-ci-robot commented Oct 16, 2018

bokysan commented Oct 2, 2018 •

edited

Loading