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

Disable HPA and clear resource requests for XS clusters #34

Merged
merged 7 commits into from
Mar 10, 2020
Merged

Disable HPA and clear resource requests for XS clusters #34

merged 7 commits into from
Mar 10, 2020

Conversation

sslavic
Copy link

@sslavic sslavic commented Mar 8, 2020

No description provided.

@sslavic sslavic self-assigned this Mar 8, 2020
@sslavic sslavic requested review from a team March 9, 2020 08:52
@sslavic sslavic marked this pull request as ready for review March 9, 2020 08:52
rossf7
rossf7 approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@rossf7 rossf7 left a comment

Choose a reason for hiding this comment

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

LGTM

helm/nginx-ingress-controller-app/Configuration.md Outdated Show resolved Hide resolved
@rossf7
Update helm/nginx-ingress-controller-app/Configuration.md
d6af25b 
Co-Authored-By: Ross Fairbanks <rossf7@users.noreply.github.com>
pipo02mix
pipo02mix approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@pipo02mix pipo02mix left a comment

Choose a reason for hiding this comment

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

My only concern is if we could use numeric cluster size so we could do it something like

if .Values.cluster_profile > 2
...
-end

Knowing that cluister profiles are

Size Number
XS 1
S 2
L 3
XL 4
XXL 5

@pipo02mix pipo02mix mentioned this pull request Mar 9, 2020
Merged
pipo02mix
pipo02mix reviewed Mar 9, 2020
View reviewed changes
helm/nginx-ingress-controller-app/templates/controller-hpa.yaml Outdated
@@ -1,4 +1,4 @@
{{- if not ( eq .Values.cluster.profile "XS" ) }}
{{- if or (eq .Values.cluster.profile 0) (gt .Values.cluster.profile 1) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

but it does mean is always enabled?

Copy link
Author

Choose a reason for hiding this comment

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

nope, not when equal 1 i.e. xs

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM then :)

@sslavic
Copy link
Author

sslavic commented Mar 10, 2020 •
edited
Loading

Tested on geckon and giraffe, works as expected with cluster-operator setting cluster profile, and same as before (with 1.6.0 release) when cluster profile is not available.

@sslavic sslavic merged commit 70aadc7 into master Mar 10, 2020
@sslavic sslavic deleted the cluster-profile-specific-resources branch March 10, 2020 14:33
@sslavic sslavic mentioned this pull request Mar 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pipo02mix pipo02mix pipo02mix approved these changes

@rossf7 rossf7 rossf7 approved these changes

Assignees

@sslavic sslavic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sslavic @rossf7 @pipo02mix