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

Add app-autoscaler-release #127

Merged
merged 48 commits into from
Dec 6, 2019

Conversation

aqan213
Copy link
Contributor

@aqan213 aqan213 commented Nov 8, 2019

Description

Add the app-autoscaler bosh release to kubecf helm chart .
https://bosh.io/releases/github.com/cloudfoundry-incubator/app-autoscaler-release?version=2.0.0

Motivation and Context

How Has This Been Tested?

Manually.

Screenshots (if appropriate):

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.

@aqan213
Copy link
Contributor Author

aqan213 commented Nov 8, 2019

The current release images need to make some changes. We now using the personal image repo since we have no access to the docker.io/cfcontainerization.
The two releases that need to use the common image repo:
app-autoscaler:

- name: "app-autoscaler"
  version: "2.0.0"
  url: "https://bosh.io/d/github.com/cloudfoundry-incubator/app-autoscaler-release?v=2.0.0"
  sha1: "b2bda4bd9ff9b902fa6a6f3d37c13b681b3402b0" 

postgres:

name: "postgres"
  version: "25"
  url: "https://bosh.io/d/github.com/cloudfoundry/postgres-release?v=25"
  sha1: "20929ee4b0c64fd97072a266311a6d00714124a7" 

@f0rmiga f0rmiga self-requested a review November 8, 2019 04:18
Copy link
Member

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot for the PR. I left a first change request, then we can go from there given the structure will change.

deploy/helm/kubecf/templates/ops-add-autoscaler.yaml Outdated Show resolved Hide resolved
@f0rmiga f0rmiga changed the title add app-autoscaler-release to kubecf Add app-autoscaler-release Nov 8, 2019
@aqan213
Copy link
Contributor Author

aqan213 commented Nov 21, 2019

@f0rmiga I have changed the PR against your last comment. Any other comments about this PR? Thanks,

@f0rmiga
Copy link
Member

f0rmiga commented Nov 21, 2019

@aqan213 The .Values.replicas don't seem the right direction, as we want to control the number of replicas per instance group. #179 introduces it. I'll get to that first, then we can rebase this PR and work from there. I also believe this PR is still blocked by not having the images published to our dockerhub account. It's in our backlog though. Thanks for following up!

@f0rmiga f0rmiga added Status: Blocked Dependencies on other issues and/or pull requests and removed Status: Blocked Dependencies on other issues and/or pull requests labels Nov 21, 2019
@aqan213
Copy link
Contributor Author

aqan213 commented Dec 3, 2019

@aqan213 I bumped the autoscaler to version 3.0.0 and postgres to version 39. I also did other improvements to get everything working. Could you check if it's all good? I tested manually.

@f0rmiga Thanks for the changes. I tested manually and just a little values needed to be modified. Such as the health port for the metricsforwarder. I have changed them.

@f0rmiga f0rmiga requested a review from mook-as December 4, 2019 01:21
"${LOG_DIR}/pg_janitor_ctl.log" \
"${LOG_DIR}/pg_janitor_ctl.err.log"

# TODO: set these skip_ssl_validation to false if the cf API (api.((system_domain))) public cert is provided by the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is because those jobs can't take extra certs (only the system ones)?
How will that work if the user provides a non-public cert (as in, a custom cert that chains up to their company-local CA)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the cert is trust and the skip_ssl_validation can be set to false. But if the cert is not trust(if access the url with the cert from browser, it reports "Your connection is not secure"), the skip_ssl_validation value should be true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, those would be the system certs (VeriSign, Let's Encrypt, CNNIC, Microsoft, …)
I'm thinking about cases of actual deployment: for example, SUSE has an internal CA, which is used for some internal services. That CA does not sign anything public (and is not in the standard list of public CAs), but is deployed to any internal systems to be trusted. How would that be propagated correctly in this case?

deploy/helm/kubecf/templates/uaa_autoscaler_client.yaml Outdated Show resolved Hide resolved
deploy/helm/kubecf/templates/uaa_autoscaler_client.yaml Outdated Show resolved Hide resolved
mook-as
mook-as previously approved these changes Dec 5, 2019
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Generally I like this a lot better, thanks for the changes!

"${LOG_DIR}/pg_janitor_ctl.log" \
"${LOG_DIR}/pg_janitor_ctl.err.log"

# TODO: set these skip_ssl_validation to false if the cf API (api.((system_domain))) public cert is provided by the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, those would be the system certs (VeriSign, Let's Encrypt, CNNIC, Microsoft, …)
I'm thinking about cases of actual deployment: for example, SUSE has an internal CA, which is used for some internal services. That CA does not sign anything public (and is not in the standard list of public CAs), but is deployed to any internal systems to be trusted. How would that be propagated correctly in this case?

@f0rmiga f0rmiga dismissed their stale review December 6, 2019 19:47

I became an author of the PR as well

Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

I guess I'll try making a follow-up PR for the script thing, if I manage to do it… 😿

@f0rmiga f0rmiga merged commit df0228b into cloudfoundry-incubator:master Dec 6, 2019
bikramnehra pushed a commit that referenced this pull request Apr 21, 2020
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.

3 participants