[stable/ambassador] Major Release v3.0.0 #15538

iNoahNothing · 2019-07-15T17:57:02Z

What this PR does / why we need it:

This is a major chart release with a couple of breaking changes.

Notable changes include:

Replacing service.http, service.https, and additionalTCPPort with service.ports.
renaming the admin service to match the default YAML install
Removing the default Module annotation
RBAC fixes for CRDs

All changes are documented in the CHANGELOG and README.

Which issue this PR fixes

fixes [stable/ambassador] AMBASSADOR_ID not set on ambassador-pro containers #15025
fixes Allow ambassador Helm chart to use CRDS when namespaced emissary-ingress/emissary#1576
fixes Multiple Ambassador deployments fails with CRDs emissary-ingress/emissary#1679
fixes Helm chart does not have option to open additional ports for TCPMapping emissary-ingress/emissary#1672
fixes [stable/ambassador] CRDs not accessible #14243

Special notes for your reviewer:

If there are any other breaking changes that had been discussed that I missed please comment the same.

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
Title of the PR starts with chart name (e.g. [stable/chart])

Signed-off-by: Noah Krause <krausenoah@gmail.com>

k8s-ci-robot · 2019-07-15T17:57:17Z

Hi @nbkrause. 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.

Signed-off-by: Noah Krause <krausenoah@gmail.com>

…amespace Signed-off-by: Noah Krause <krausenoah@gmail.com>

Signed-off-by: Noah Krause <krausenoah@gmail.com>

…ut not necessarily created Signed-off-by: Noah Krause <krausenoah@gmail.com>

Signed-off-by: Noah Krause <krausenoah@gmail.com>

Flydiverny · 2019-07-15T18:54:08Z

/assign
Will take a more detailed look at this when I return from vacation. (flying back tomorrow so in the coming days!)

iNoahNothing · 2019-07-16T13:05:34Z

/hold

Pending internal review this afternoon

…is as redis server Signed-off-by: Noah Krause <krausenoah@gmail.com>

stable/ambassador/templates/deployment.yaml

stable/ambassador/templates/service.yaml

stable/ambassador/values.yaml

Flydiverny · 2019-07-18T01:13:51Z

Another potential improvment, that I think can wait. Would be to remove the ambassador-pro-redis.yaml and add a dependency on the redis chart instead, to allow all the configurability the redis chart offers :)
Can be done with helm dependencies (for example ghost chart depends on mariadb https://github.com/helm/charts/blob/master/stable/ghost/requirements.yaml) and they allow conditionals.

Signed-off-by: Noah Krause <krausenoah@gmail.com>

iNoahNothing · 2019-07-19T18:59:39Z

Another potential improvment, that I think can wait. Would be to remove the ambassador-pro-redis.yaml and add a dependency on the redis chart instead, to allow all the configurability the redis chart offers :)
Can be done with helm dependencies (for example ghost chart depends on mariadb https://github.com/helm/charts/blob/master/stable/ghost/requirements.yaml) and they allow conditionals.

This is not a breaking change so I would rather put it off to a later date. I'll create an issue to track and see how much of an effort this would be as well as potential trade offs

iNoahNothing · 2019-07-19T19:05:04Z

/hold cancel

The PR looks sane to members of the Datawire team.

@Flydiverny @kflynn Do either of you wanna take a final pass at it?

Flydiverny · 2019-07-19T21:57:25Z

/ok-to-test

Flydiverny · 2019-07-19T21:57:58Z

Looks good to me, havent tested it but should be ok if test passes :)
Seems to need a rebase tho as Chart.yaml is conflicting.

Flydiverny · 2019-07-19T21:58:50Z

/lgtm

iNoahNothing · 2019-07-22T14:33:24Z

/retest

Signed-off-by: Noah Krause <krausenoah@gmail.com>

iNoahNothing · 2019-07-25T14:12:57Z

/retest

Flydiverny · 2019-07-25T15:11:39Z

/retest

Signed-off-by: Noah Krause <krausenoah@gmail.com>

iNoahNothing · 2019-07-29T15:05:04Z

/test pull-charts-e2e

Flydiverny · 2019-07-29T16:05:36Z

I think the connection test thing is probably failing. Think it can be tested by using helm test after installing the chart. Havent had the time to check it :) @nbkrause

iNoahNothing · 2019-07-29T18:52:04Z

@Flydiverny Thank you! I had no clue what was causing this and don't yet fully grok the test infrastructure.

Signed-off-by: Noah Krause <krausenoah@gmail.com>

Flydiverny · 2019-07-30T02:23:06Z

Nice!

/lgtm

k8s-ci-robot · 2019-07-30T02:23:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Flydiverny, nbkrause

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/ambassador/OWNERS~~ [Flydiverny,nbkrause]

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

* Make ports configuration flexible to allow for TCPMapping ports Signed-off-by: Noah Krause <krausenoah@gmail.com> * bump chart version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Update README and bump major version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Rename admin service to match YAML templates Signed-off-by: Noah Krause <krausenoah@gmail.com> * Remove default annotation Signed-off-by: Noah Krause <krausenoah@gmail.com> * .service.ports eliminates the need to `.additionalTCPPorts` as well Signed-off-by: Noah Krause <krausenoah@gmail.com> * Create clusterrole for crds to solve issue with singlenamespaced config Signed-off-by: Noah Krause <krausenoah@gmail.com> * rbac.namespaced was extraneous and has been replaced by scope.singleNamespace Signed-off-by: Noah Krause <krausenoah@gmail.com> * Ambassador Pro will notice `AMBASSADOR_ID` in `.Values.env` Signed-off-by: Noah Krause <krausenoah@gmail.com> * `crds.enabled` will flag that crds should be enabled in the release but not necessarily created Signed-off-by: Noah Krause <krausenoah@gmail.com> * Update changelog Signed-off-by: Noah Krause <krausenoah@gmail.com> * Fix trailing spaces Signed-off-by: Noah Krause <krausenoah@gmail.com> * Bump pro version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Ambassador Pro will use {{ include "ambassador.fullname" . }}-pro-redis as redis server Signed-off-by: Noah Krause <krausenoah@gmail.com> * Requested fixes Signed-off-by: Noah Krause <krausenoah@gmail.com> * Forgot port on redis url change Signed-off-by: Noah Krause <krausenoah@gmail.com> * fix test Signed-off-by: Noah Krause <krausenoah@gmail.com>

* Make ports configuration flexible to allow for TCPMapping ports Signed-off-by: Noah Krause <krausenoah@gmail.com> * bump chart version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Update README and bump major version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Rename admin service to match YAML templates Signed-off-by: Noah Krause <krausenoah@gmail.com> * Remove default annotation Signed-off-by: Noah Krause <krausenoah@gmail.com> * .service.ports eliminates the need to `.additionalTCPPorts` as well Signed-off-by: Noah Krause <krausenoah@gmail.com> * Create clusterrole for crds to solve issue with singlenamespaced config Signed-off-by: Noah Krause <krausenoah@gmail.com> * rbac.namespaced was extraneous and has been replaced by scope.singleNamespace Signed-off-by: Noah Krause <krausenoah@gmail.com> * Ambassador Pro will notice `AMBASSADOR_ID` in `.Values.env` Signed-off-by: Noah Krause <krausenoah@gmail.com> * `crds.enabled` will flag that crds should be enabled in the release but not necessarily created Signed-off-by: Noah Krause <krausenoah@gmail.com> * Update changelog Signed-off-by: Noah Krause <krausenoah@gmail.com> * Fix trailing spaces Signed-off-by: Noah Krause <krausenoah@gmail.com> * Bump pro version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Ambassador Pro will use {{ include "ambassador.fullname" . }}-pro-redis as redis server Signed-off-by: Noah Krause <krausenoah@gmail.com> * Requested fixes Signed-off-by: Noah Krause <krausenoah@gmail.com> * Forgot port on redis url change Signed-off-by: Noah Krause <krausenoah@gmail.com> * fix test Signed-off-by: Noah Krause <krausenoah@gmail.com> Signed-off-by: Roland Gritzer <gritzer.roland@gmail.com>

* Make ports configuration flexible to allow for TCPMapping ports Signed-off-by: Noah Krause <krausenoah@gmail.com> * bump chart version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Update README and bump major version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Rename admin service to match YAML templates Signed-off-by: Noah Krause <krausenoah@gmail.com> * Remove default annotation Signed-off-by: Noah Krause <krausenoah@gmail.com> * .service.ports eliminates the need to `.additionalTCPPorts` as well Signed-off-by: Noah Krause <krausenoah@gmail.com> * Create clusterrole for crds to solve issue with singlenamespaced config Signed-off-by: Noah Krause <krausenoah@gmail.com> * rbac.namespaced was extraneous and has been replaced by scope.singleNamespace Signed-off-by: Noah Krause <krausenoah@gmail.com> * Ambassador Pro will notice `AMBASSADOR_ID` in `.Values.env` Signed-off-by: Noah Krause <krausenoah@gmail.com> * `crds.enabled` will flag that crds should be enabled in the release but not necessarily created Signed-off-by: Noah Krause <krausenoah@gmail.com> * Update changelog Signed-off-by: Noah Krause <krausenoah@gmail.com> * Fix trailing spaces Signed-off-by: Noah Krause <krausenoah@gmail.com> * Bump pro version Signed-off-by: Noah Krause <krausenoah@gmail.com> * Ambassador Pro will use {{ include "ambassador.fullname" . }}-pro-redis as redis server Signed-off-by: Noah Krause <krausenoah@gmail.com> * Requested fixes Signed-off-by: Noah Krause <krausenoah@gmail.com> * Forgot port on redis url change Signed-off-by: Noah Krause <krausenoah@gmail.com> * fix test Signed-off-by: Noah Krause <krausenoah@gmail.com>

iNoahNothing added 6 commits July 12, 2019 16:46

Make ports configuration flexible to allow for TCPMapping ports

39aa065

Signed-off-by: Noah Krause <krausenoah@gmail.com>

bump chart version

9cc2704

Signed-off-by: Noah Krause <krausenoah@gmail.com>

Update README and bump major version

0373d02

Signed-off-by: Noah Krause <krausenoah@gmail.com>

Rename admin service to match YAML templates

8842837

Signed-off-by: Noah Krause <krausenoah@gmail.com>

Remove default annotation

7702c31

Signed-off-by: Noah Krause <krausenoah@gmail.com>

.service.ports eliminates the need to .additionalTCPPorts as well

fe22db7

Signed-off-by: Noah Krause <krausenoah@gmail.com>

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2019

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 Jul 15, 2019

k8s-ci-robot requested review from Flydiverny and kflynn July 15, 2019 17:57

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

iNoahNothing added 5 commits July 15, 2019 13:59

Create clusterrole for crds to solve issue with singlenamespaced config

af3c156

Signed-off-by: Noah Krause <krausenoah@gmail.com>

rbac.namespaced was extraneous and has been replaced by scope.singleN…

3f18fc9

…amespace Signed-off-by: Noah Krause <krausenoah@gmail.com>

Ambassador Pro will notice AMBASSADOR_ID in .Values.env

163b984

Signed-off-by: Noah Krause <krausenoah@gmail.com>

crds.enabled will flag that crds should be enabled in the release b…

e530d2b

…ut not necessarily created Signed-off-by: Noah Krause <krausenoah@gmail.com>

Update changelog

b299493

Signed-off-by: Noah Krause <krausenoah@gmail.com>

iNoahNothing force-pushed the chart/v3.0.0 branch from 35682b8 to b299493 Compare July 15, 2019 17:59

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Jul 15, 2019

iNoahNothing added 2 commits July 15, 2019 14:02

Fix trailing spaces

08c9eeb

Signed-off-by: Noah Krause <krausenoah@gmail.com>

Bump pro version

1bd78cb

Signed-off-by: Noah Krause <krausenoah@gmail.com>

k8s-ci-robot assigned Flydiverny Jul 15, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2019

Ambassador Pro will use {{ include "ambassador.fullname" . }}-pro-red…

0503f45

…is as redis server Signed-off-by: Noah Krause <krausenoah@gmail.com>

Flydiverny reviewed Jul 18, 2019

View reviewed changes

stable/ambassador/templates/deployment.yaml Show resolved Hide resolved

stable/ambassador/templates/service.yaml Outdated Show resolved Hide resolved

stable/ambassador/values.yaml Outdated Show resolved Hide resolved

stable/ambassador/values.yaml Outdated Show resolved Hide resolved

Requested fixes

f23be7c

Signed-off-by: Noah Krause <krausenoah@gmail.com>

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 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 Jul 19, 2019

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

This was referenced Jul 19, 2019

[stable/ambassador] adding annotation support on the admin service #15562

Merged

ambassador: add podSecurityPolicies #15407

Merged

Merge branch 'master' of github.com:helm/charts into chart/v3.0.0

a206723

Signed-off-by: Noah Krause <krausenoah@gmail.com>

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

Forgot port on redis url change

096f80f

Signed-off-by: Noah Krause <krausenoah@gmail.com>

fix test

d6968ba

Signed-off-by: Noah Krause <krausenoah@gmail.com>

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

k8s-ci-robot merged commit f5a544a into helm:master Jul 30, 2019

Flydiverny mentioned this pull request Aug 9, 2019

[stable/ambassador] Remove default ambassador Module annotation #14944

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/ambassador] Major Release v3.0.0 #15538

[stable/ambassador] Major Release v3.0.0 #15538

iNoahNothing commented Jul 15, 2019

k8s-ci-robot commented Jul 15, 2019

Flydiverny commented Jul 15, 2019

iNoahNothing commented Jul 16, 2019

Flydiverny commented Jul 18, 2019

iNoahNothing commented Jul 19, 2019

iNoahNothing commented Jul 19, 2019

Flydiverny commented Jul 19, 2019

Flydiverny commented Jul 19, 2019

Flydiverny commented Jul 19, 2019

iNoahNothing commented Jul 22, 2019

iNoahNothing commented Jul 25, 2019

Flydiverny commented Jul 25, 2019

iNoahNothing commented Jul 29, 2019

Flydiverny commented Jul 29, 2019 •

edited

Loading

iNoahNothing commented Jul 29, 2019

Flydiverny commented Jul 30, 2019

k8s-ci-robot commented Jul 30, 2019

[stable/ambassador] Major Release v3.0.0 #15538

[stable/ambassador] Major Release v3.0.0 #15538

Conversation

iNoahNothing commented Jul 15, 2019

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Jul 15, 2019

Flydiverny commented Jul 15, 2019

iNoahNothing commented Jul 16, 2019

Flydiverny commented Jul 18, 2019

iNoahNothing commented Jul 19, 2019

iNoahNothing commented Jul 19, 2019

Flydiverny commented Jul 19, 2019

Flydiverny commented Jul 19, 2019

Flydiverny commented Jul 19, 2019

iNoahNothing commented Jul 22, 2019

iNoahNothing commented Jul 25, 2019

Flydiverny commented Jul 25, 2019

iNoahNothing commented Jul 29, 2019

Flydiverny commented Jul 29, 2019 • edited Loading

iNoahNothing commented Jul 29, 2019

Flydiverny commented Jul 30, 2019

k8s-ci-robot commented Jul 30, 2019

Flydiverny commented Jul 29, 2019 •

edited

Loading