stable/concourse: separate worker, web deployments #12920

YoussB · 2019-04-08T19:38:03Z

What this PR does / why we need it:

Added .Values.Web.enabled and .Values.worker.enabled (with default to
true).
if only .Values.web.enabled is enabled: only web resources are going to
be created, as well as secrets namespace.
if only .Values.worker.enabled is enabled: only worker resources are going to
be created.
moved the worker-specific and web-specific secrets each to a separate
file and secrets object.
added .Values.concourse.worker.tsa.port and utilised
.Values.concourse.worker.host in order to allow the user to set the
CONCOURSE_TSA_HOST easily.
removed the template concourse.concourse.fullname as it is not used
anymore.
bumped the chart version to 6.0.0, as this adds new ways to use the
chart.
added the new variables to the README doc.

Which issue this PR fixes

fixes [stable/concourse] Creation of worker-only releases #11280

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

cc: @taylorsilva

k8s-ci-robot · 2019-04-08T19:38:17Z

Hi @YoussB. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cirocosta · 2019-04-08T23:42:58Z

/ok-to-test

cirocosta · 2019-04-08T23:43:39Z

/assign cirocosta

taylorsilva · 2019-04-09T12:34:23Z

Looks like we forgot to push up the chart version bump. Just need to do that and then this will be good to test.

YoussB · 2019-04-09T12:41:05Z

@taylorsilva pushed that!

cirocosta · 2019-04-09T16:24:59Z

Hey,

Some thoughts that @YoussB and I were talking about when pairing on reviewing the PR:

we could make required that either one or both of {web|worker}.enabled is set to true,
given that the chart now internally creates and uses two secrets (one for the workers, another for the web components), there is a breaking change for consumers of the chart who are bringing their own secrets (which is what we recommend)
add a required to concourse.worker.tsa.host to ensure that when web is not enabled, we fail there
PostgreSQL dependency could have a check for web.enabled

Also, as follow-ups:

we can also go ahead with [stable/concourse] Provide a changelog that communicate our changes #11512 and provide the first documentation of breaking changes (in a separate PR).
we can leave worker-key-pub out of worker-secrets and worker-statefulset as that's not needed (this can be done in a separate PR)
we could have the labels updated to ensure that they all match the new ones (https://github.com/helm/charts/blob/2e42062101349b82aa9995bfc8e2db88870bc1c8/REVIEW_GUIDELINES.md#metadata) - also in a separate PR
perhaps on the Concourse team side, we might want to have some coverage regarding RBAC (separate PR, in the concourse/concourse repository)
concourse.worker.tsa.hosts could be an array that takes multiple TSA hosts (separate PR)

Thanks!

YoussB · 2019-04-12T14:10:02Z

Updates:

we could make required that either one or both of {web|worker}.enabled is set to true. added a hacky way to check if either of the worker or web are enabled.
given that the chart now internally creates and uses two secrets (one for the workers, another for the web components), there is a breaking change for consumers of the chart who are bringing their own secrets (which is what we recommend)
add a required to concourse.worker.tsa.host to ensure that when web is not enabled, we fail there
~~PostgreSQL dependency could have a check for web.enabled~~ not possible! All options wether tags or condition have no and option only able to add postgress based on one value being added. Will go with the enabled disabled option

Also, as follow-ups:

we can also go ahead with [stable/concourse] Provide a changelog that communicate our changes #11512 and provide the first documentation of breaking changes (in ~~a separate~~ this PR).
we can leave worker-key-pub out of worker-secrets and worker-statefulset as that's not needed (this can be done in a separate PR) [stable/concourse] Investigate and remove mounting workerKeyPub inside the worker pod #13019
we could have the labels updated to ensure that they all match the new ones (https://github.com/helm/charts/blob/2e42062101349b82aa9995bfc8e2db88870bc1c8/REVIEW_GUIDELINES.md#metadata) - also in a separate PR stable/concourse update labels to latest helm labels #13024 submitted.
perhaps on the Concourse team side, we might want to have some coverage regarding RBAC (separate PR, in the concourse/concourse repository) topgun/k8s: Add coverage for RBAC concourse/concourse#3733
concourse.worker.tsa.hosts could be an array that takes multiple TSA hosts (separate PR) stable/concourse support multiple TSA hosts #13025

YoussB · 2019-04-12T22:16:49Z

@cirocosta ready for testing 😁

cirocosta · 2019-04-15T01:45:00Z

/retest

cirocosta

Hey,

I've made quite a few suggestions in terms of phrasing, feel free to not accept them right away 😁 I tried making them a bit more coherent to the rest of the comments & docs, let me know what you think!

The overall functionality seems good to me 👍 Thanks for adding the breaking changes too!

stable/concourse/CHANGELOG.md

stable/concourse/README.md

stable/concourse/values.yaml

cirocosta · 2019-04-17T21:28:30Z

stable/concourse/values.yaml

@@ -1312,6 +1321,12 @@ web:
 ## Concourse Workers, see https://concourse-ci.org/concourse-worker.html
 ##
 worker:
+
+  ## enable/disable worker component
+  ## This can allow users to create web only releases by setting this to false


Suggested change

## This can allow users to create web only releases by setting this to false

## This allows the creation of web-only releases by setting this to false.

YoussB · 2019-04-22T12:41:02Z

@cirocosta updated!

- Added .Values.Web.enabled and .Values.worker.enabled (with default to true). - if only .Values.web.enable is enabled: only web resources are going to be created, as well as secrets namespace. - if only .Values.worker.enable is enabled: only worker resources are going to be created. - moved the worker specific and web specific secrets each to a seprate file and secrets object. - added .Values.concourse.worker.tsa.port and utilised .Values.concourse.worker.host in order to alow the user to set the `CONCOURSE_TSA_HOST` easily. - removed the template `concourse.concourse.fullname` as it is not used anymore. - bumped the chart version to 6.0.0, as this adds new ways to use the chart. - added the new variables to the README doc. for the issue: #11280 Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io> Signed-off-by: Taylor Silva <tsilva@pivotal.io>

Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io>

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

…ents Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

cirocosta

SGTM

cirocosta · 2019-04-25T20:29:36Z

/lgtm

k8s-ci-robot · 2019-04-25T20:29:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cirocosta, YoussB

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:

~~stable/concourse/OWNERS~~ [cirocosta]

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

* stable/concourse: separate worker, web deployments - Added .Values.Web.enabled and .Values.worker.enabled (with default to true). - if only .Values.web.enable is enabled: only web resources are going to be created, as well as secrets namespace. - if only .Values.worker.enable is enabled: only worker resources are going to be created. - moved the worker specific and web specific secrets each to a seprate file and secrets object. - added .Values.concourse.worker.tsa.port and utilised .Values.concourse.worker.host in order to alow the user to set the `CONCOURSE_TSA_HOST` easily. - removed the template `concourse.concourse.fullname` as it is not used anymore. - bumped the chart version to 6.0.0, as this adds new ways to use the chart. - added the new variables to the README doc. for the issue: helm#11280 Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io> Signed-off-by: Taylor Silva <tsilva@pivotal.io> * [stable/concourse] bumping chart version and updating the Readme Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io> * Adds a required check yaml for having either the web or worker enabled Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * Adds required for concourse.worker.tsa in case of worker only deployments Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * updates readme Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * adds changelog file Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * Apply suggestions from code review Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

* spinnaker-additional-configmaps: (158 commits) [stable/spinnaker] Bump chart version [stable/spinnaker] Allow to use existing additionalConfigMaps objects [stable/instana-agent] Add instana-agent chart to stable (helm#12799) [stable/spring-cloud-data-flow] apiGroup extension does not have permissions over Jobs (helm#12174) Fluentd - Add option to add environment variables from secrets (helm#12565) Fluentd - Allow ingress path to be configurable (helm#12561) [stable/openebs]: update NDM image tag to 0.3.5 (helm#13282) stable/phabricator: update to 2019.16.0 (helm#13307) [stable/jenkins] Add support for idleMinutes and serviceAccount (helm#13263) [stable/gocd] Bump up k8 elastic agent to latest and bump up GoCD to v19.3.0 (helm#13301) [stable/atlantis] Add `--default-tf-version=` and `--allow-fork-prs` flag (helm#13299) stackdriver-exporter: allow google service account (helm#13214) SC-4435 Do not start the container if particular token is not provided (helm#13304) [stable/spring-cloud-data-flow] Update to new SCDF version 2.0.2 (helm#12951) allow to set COCKROACH_ENGINE_MAX_SYNC_DURATION (helm#13244) Use same JENKINS_URL no matter if slaves use different namespace (helm#12564) stable/concourse: separate worker, web deployments (helm#12920) [ci] Upgrade to chart-testing v2.3.3 (helm#13294) fixes incompatibility with 1.11 (helm#13261) Detect current network and netmask (helm#13250) ... # Conflicts: # stable/spinnaker/Chart.yaml

* stable/concourse: separate worker, web deployments - Added .Values.Web.enabled and .Values.worker.enabled (with default to true). - if only .Values.web.enable is enabled: only web resources are going to be created, as well as secrets namespace. - if only .Values.worker.enable is enabled: only worker resources are going to be created. - moved the worker specific and web specific secrets each to a seprate file and secrets object. - added .Values.concourse.worker.tsa.port and utilised .Values.concourse.worker.host in order to alow the user to set the `CONCOURSE_TSA_HOST` easily. - removed the template `concourse.concourse.fullname` as it is not used anymore. - bumped the chart version to 6.0.0, as this adds new ways to use the chart. - added the new variables to the README doc. for the issue: helm#11280 Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io> Signed-off-by: Taylor Silva <tsilva@pivotal.io> * [stable/concourse] bumping chart version and updating the Readme Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io> * Adds a required check yaml for having either the web or worker enabled Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * Adds required for concourse.worker.tsa in case of worker only deployments Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * updates readme Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * adds changelog file Signed-off-by: Bishoy Youssef <byoussef@pivotal.io> * Apply suggestions from code review Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 8, 2019

k8s-ci-robot requested review from cirocosta and william-tran April 8, 2019 19:38

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 8, 2019

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 8, 2019

k8s-ci-robot assigned cirocosta Apr 8, 2019

cirocosta mentioned this pull request Apr 9, 2019

topgun/k8s: external worker test to use separate deployments concourse/concourse#3705

Merged

YoussB mentioned this pull request Apr 11, 2019

Road to a supported installation concourse/hush-house#27

Closed

17 tasks

YoussB mentioned this pull request Apr 12, 2019

stable/concourse support multiple TSA hosts #13025

Closed

cirocosta suggested changes Apr 17, 2019

View reviewed changes

jonninen50 approved these changes Apr 22, 2019

View reviewed changes

taylorsilva and others added 7 commits April 25, 2019 16:18

[stable/concourse] bumping chart version and updating the Readme

9f7fd20

Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Bishoy Youssef <byoussef@pivotal.io>

Adds a required check yaml for having either the web or worker enabled

be97867

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

Adds required for concourse.worker.tsa in case of worker only deploym…

37e4445

…ents Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

updates readme

0e28e52

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

adds changelog file

f658974

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

Apply suggestions from code review

7161d08

Signed-off-by: Bishoy Youssef <byoussef@pivotal.io>

cirocosta approved these changes Apr 25, 2019

View reviewed changes

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2019

k8s-ci-robot merged commit 8e93db6 into helm:master Apr 25, 2019

cirocosta deleted the 11280-worker-only-deployments branch April 25, 2019 20:32

william-tran mentioned this pull request Apr 29, 2019

[stable/concourse] Updating maintainers #13366

Merged

2 tasks

cirocosta mentioned this pull request May 6, 2019

[stable/concourse] Provide a changelog that communicate our changes #11512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stable/concourse: separate worker, web deployments #12920

stable/concourse: separate worker, web deployments #12920

YoussB commented Apr 8, 2019

k8s-ci-robot commented Apr 8, 2019

cirocosta commented Apr 8, 2019

cirocosta commented Apr 8, 2019

taylorsilva commented Apr 9, 2019

YoussB commented Apr 9, 2019

cirocosta commented Apr 9, 2019

YoussB commented Apr 12, 2019 •

edited

Loading

YoussB commented Apr 12, 2019

cirocosta commented Apr 15, 2019

cirocosta left a comment

cirocosta Apr 17, 2019

YoussB commented Apr 22, 2019

cirocosta left a comment

cirocosta commented Apr 25, 2019

k8s-ci-robot commented Apr 25, 2019

	## This can allow users to create web only releases by setting this to false
	## This allows the creation of web-only releases by setting this to false.

stable/concourse: separate worker, web deployments #12920

stable/concourse: separate worker, web deployments #12920

Conversation

YoussB commented Apr 8, 2019

What this PR does / why we need it:

Which issue this PR fixes

Checklist

k8s-ci-robot commented Apr 8, 2019

cirocosta commented Apr 8, 2019

cirocosta commented Apr 8, 2019

taylorsilva commented Apr 9, 2019

YoussB commented Apr 9, 2019

cirocosta commented Apr 9, 2019

YoussB commented Apr 12, 2019 • edited Loading

YoussB commented Apr 12, 2019

cirocosta commented Apr 15, 2019

cirocosta left a comment

Choose a reason for hiding this comment

cirocosta Apr 17, 2019

Choose a reason for hiding this comment

YoussB commented Apr 22, 2019

cirocosta left a comment

Choose a reason for hiding this comment

cirocosta commented Apr 25, 2019

k8s-ci-robot commented Apr 25, 2019

YoussB commented Apr 12, 2019 •

edited

Loading