Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

production configuration (optional) #5286

Merged
merged 6 commits into from
May 10, 2018
Merged

production configuration (optional) #5286

merged 6 commits into from
May 10, 2018

Conversation

vtuson
Copy link
Contributor

@vtuson vtuson commented Apr 27, 2018

What this PR does / why we need it:
Added two things here:

  • an alternative (optional) values-production yaml which is opinionated on how to set up the chart for production environment (using ingress, clusterIp, RWX PVC,..)
  • comments on the README to explain how to horizontally scale the chart and how to use nfs-server-provisioner for it.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 27, 2018
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 27, 2018
@vtuson
Copy link
Contributor Author

vtuson commented Apr 27, 2018

/assign @sameersbn

@unguiculus
Copy link
Member

A separate values file doesn't make sense. You can't use it anyways when you install the chart from the repo. That's rather something that should go into the readme.

@vtuson
Copy link
Contributor Author

vtuson commented Apr 30, 2018

@unguiculus it is linked from the readme, dont you think it would make a very long readme to have a whole values file on it?

sameersbn
sameersbn suggested changes May 7, 2018
View reviewed changes
stable/wordpress/README.md Outdated
@@ -116,6 +116,23 @@ $ helm install --name my-release -f values.yaml stable/wordpress

> **Tip**: You can use the default [values.yaml](values.yaml)

## Production and horizontal scaling

The following repo contains the recommended production settings for wordpress capture in an alternative [values file](values-production.yaml). Please read carefully the comments in the values-production.yaml file to set up your environment appropiately.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo appropiately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

stable/wordpress/values-production.yaml Outdated
## Kubernetes configuration
## For minikube, set this to NodePort, elsewhere use LoadBalancer or ClusterIP
##
serviceType: ClusterIp
Copy link
Contributor

Choose a reason for hiding this comment

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

-> ClusterIP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sameersbn
Copy link
Contributor

@unguiculus: @vtuson is providing a sane default configuration for production use cases with scalability and high availability in mind. As you rightly pointed out, the values-production.yaml file will not be available directly while installing the chart from a chart repo and perhaps should also be ignored (.helmignore) while packaging the chart. However I think the appropriate place for this configuration is the the git repo of the chart.

@vtuson could you add values-production.yaml to the .helmignore file?

@vtuson
Copy link
Contributor Author

vtuson commented May 7, 2018

@sameersbn done

@sameersbn
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 May 8, 2018
@sameersbn
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2018
@sameersbn
Copy link
Contributor

@unguiculus are you okay with these changes being merged? do you have any other concerns with the addition of a values-production.yaml to the chart directory?

unguiculus
unguiculus suggested changes May 9, 2018
View reviewed changes
stable/wordpress/README.md

```console
$ helm install stable/nfs-server-provisioner --set persistence.enabled=true,persistence.size=10Gi
$ helm install --name my-release -f values-production.yaml --set persitence.storageClass=nfs stable/wordpress
Copy link
Member

Choose a reason for hiding this comment

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

This won't work as is. The docs should at least mention that values-production.yaml has to be copied to a local directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@vtuson vtuson mentioned this pull request May 10, 2018
Closed
sameersbn
sameersbn reviewed May 10, 2018
View reviewed changes
stable/wordpress/README.md Outdated
@@ -120,10 +120,11 @@ $ helm install --name my-release -f values.yaml stable/wordpress

The following repo contains the recommended production settings for wordpress capture in an alternative [values file](values-production.yaml). Please read carefully the comments in the values-production.yaml file to set up your environment appropriately.

To horizontally scale this chart:
To horizontally scale this chart, first download the values-production.yaml file to your local folder, then:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we hotlink to the file in the repo, [values-production.yaml](values-production.yaml)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is hotlink from above.. I would hot link it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @vtuson
@unguiculus could you please review

@unguiculus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sameersbn, unguiculus, vtuson

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 4fbbf97 into helm:master May 10, 2018
ichtar pushed a commit to Bestmile/charts that referenced this pull request May 15, 2018
@vtuson
production configuration (optional) (helm#5286)
9da1535 
* production configuration (optional)

* improved the command description

* bumped chart version

* addressed sameers comments

* changes suggested in review to make clear that you have to download the values-prod file

* updated readme
@chrisatomix chrisatomix mentioned this pull request Jul 10, 2018
Closed
or1can pushed a commit to or1can/charts that referenced this pull request Jul 10, 2018
@vtuson @or1can
production configuration (optional) (helm#5286)
20c9607 
* production configuration (optional)

* improved the command description

* bumped chart version

* addressed sameers comments

* changes suggested in review to make clear that you have to download the values-prod file

* updated readme
voron pushed a commit to dysnix/helm-charts that referenced this pull request Sep 5, 2018
@vtuson
production configuration (optional) (helm#5286)
ee92ed5 
* production configuration (optional)

* improved the command description

* bumped chart version

* addressed sameers comments

* changes suggested in review to make clear that you have to download the values-prod file

* updated readme

Signed-off-by: voron <av@arilot.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sameersbn sameersbn sameersbn approved these changes

@unguiculus unguiculus unguiculus requested changes

@carrodher carrodher Awaiting requested review from carrodher

@prydonius prydonius Awaiting requested review from prydonius

Assignees

@sameersbn sameersbn

@unguiculus unguiculus

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vtuson @unguiculus @sameersbn @k8s-ci-robot