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

feat(charts): add resource requests to chart #311

Merged
merged 1 commit into from
Feb 15, 2017
Merged

feat(charts): add resource requests to chart #311

merged 1 commit into from
Feb 15, 2017

Conversation

bookshelfdave
Copy link
Contributor

  • this allows the router to be used in a horizontal pod autoscaler out the the box. Without resources.requests, the HPA can't calculate a CURRENT value

@deis-admin
Copy link

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

@mboersma
Copy link
Member

mboersma commented Feb 7, 2017

Could you amend the commit message to follow the commit style guide? That way it will get picked up by deisrel when we generate the release changelog.

Jenkins, add to whitelist.

@mboersma mboersma added this to the v2.12 milestone Feb 7, 2017
@codecov-io
Copy link

codecov-io commented Feb 7, 2017 •
edited
Loading

Codecov Report

Merging #311 into master will not impact coverage.

@@           Coverage Diff           @@
##           master     #311   +/-   ##
=======================================
  Coverage   55.69%   55.69%           
=======================================
  Files           6        6           
  Lines         386      386           
=======================================
  Hits          215      215           
  Misses        151      151           
  Partials       20       20

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a38f6...74449a6. Read the comment docs.

@bookshelfdave bookshelfdave changed the title add resource requests section to deployment config feat(charts): add resource requests to chart Feb 7, 2017
@bookshelfdave
feat(charts): add resource requests to chart
74449a6 
This change allows router resource requests to be specified in addition
to the existing limits templates, allowing for easy use with
a horizontal pod autoscaler.
@bookshelfdave
Copy link
Contributor Author

fixed, thank you @mboersma

krancour
krancour reviewed Feb 7, 2017
View reviewed changes
charts/router/templates/router-deployment.yaml
@@ -32,16 +32,28 @@ spec:
- name: deis-router
image: quay.io/{{.Values.org}}/router:{{.Values.docker_tag}}
imagePullPolicy: {{.Values.pull_policy}}
{{- if or (.Values.limits_cpu) (.Values.limits_memory)}}
{{- if or (.Values.limits_cpu) (.Values.limits_memory) (.Values.requests_cpu) (.Values.requests_memory)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

or can take more than two arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so: https://github.com/golang/go/blob/master/src/text/template/funcs.go#L295

@bookshelfdave bookshelfdave mentioned this pull request Feb 8, 2017
Closed
@mboersma
Copy link
Member

I've tested this manually and it looks like the resources: stanza is created correctly when need be.

@mboersma mboersma added the LGTM1 label Feb 15, 2017
@mboersma mboersma merged commit 8ded714 into deis:master Feb 15, 2017
@bookshelfdave bookshelfdave deleted the dp_router_requests branch February 15, 2017 23:50
@bookshelfdave
Copy link
Contributor Author

thank you @mboersma!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krancour krancour krancour left review comments

@mboersma mboersma mboersma approved these changes

Assignees
No one assigned
Labels
LGTM1 LGTM2
Projects
None yet
Milestone
v2.12
Development

Successfully merging this pull request may close these issues.
6 participants
@bookshelfdave @deis-admin @mboersma @codecov-io @krancour @bacongobbler