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

fix: Check for HA as part of estimating max app count. #1591

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

andreas-kupries
Copy link
Contributor

Description

Replaced fixed value of 1 with a ternary returning either 1 or 3 based on the HA flag.

Motivation and Context

See #1479

How Has This Been Tested?

Local minikube. Deploy only as far as needed to see the NOTES.txt in the terminal.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code has security implications.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@andreas-kupries andreas-kupries linked an issue Nov 16, 2020 that may be closed by this pull request
@andreas-kupries andreas-kupries changed the title Check for HA as part of estimating max app count. fix: Check for HA as part of estimating max app count. Nov 16, 2020
@f0rmiga f0rmiga merged commit 501ed79 into master Nov 16, 2020
@f0rmiga f0rmiga deleted the ak-kubecf-1479-notes.txt branch November 16, 2020 21:07
@@ -17,7 +17,7 @@
to monitor continuously.

{{- if not .Values.features.eirini.enabled }}
{{- $cell_count := .Values.sizing.diego_cell.instances | default 1 }}
{{- $cell_count := .Values.sizing.diego_cell.instances | default (ternary 3 1 .Values.high_availability) }}
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this will ever change, but instead of hard-coding the 3 you should look it up in the manifest:

{{- $_ := include "_config.lookupManifest" (list $ "instance_groups/name=diego-cell/instances") }}
{{- $cell_count := .Values.sizing.diego_cell.instances | default (ternary $.kubecf.retval 1 .Values.high_availability) }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Will make a follow-up PR for this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NOTES.txt making wrong calculation with maximum app count
4 participants