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.

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

Use Helm for installing AES #2492

Merged
merged 1 commit into from Apr 10, 2020
Merged

Use Helm for installing AES #2492

merged 1 commit into from Apr 10, 2020

Conversation

inercia
Copy link
Contributor

@inercia inercia commented Mar 26, 2020

Description

Use the Helm repo for running edgectl install

Related Issues

#2466

Testing

1 - Basic installation

  • install with edgectl install
  • After a successful installation, check that the Helm release is there:
$ helm list -n ambassador 
NAME      	NAMESPACE 	REVISION	UPDATED                                 	STATUS  	CHART           	APP VERSION
ambassador	ambassador	1       	2020-04-08 14:03:12.893838399 +0200 CEST	deployed	ambassador-6.2.4	1.3.2      
  • Check that the app.kubernetes.io/managed-by label is properly set:
$ kubectl get deployments -n ambassador ambassador -o yaml | grep managed-by
    app.kubernetes.io/managed-by: edgectl
        app.kubernetes.io/managed-by: edgectl

2 - Installation after Helm

  • Install AES with Helm. For example (in local env, with NodePort) with :
$ kubectl create namespace ambassador
namespace/ambassador created
$ helm install ambassador --namespace ambassador datawire/ambassador --set service.type=NodePort --wait
# ...
  • Check that edgectl install detects the Helm installation and continues with the rest of the setup:
 $ ./edgectl install                

Installing the Ambassador Edge Stack

Please enter an email address for us to notify you before your TLS certificate
and domain name expire. In order to acquire the TLS certificate, we share this
email with Let’s Encrypt.
Email address [alvaro.saurin@*******]: 

========================================================================
Beginning Ambassador Edge Stack Installation

-> Checking latest version of Ambassador Edge Stack available...
-> Found an existing installation of Ambassador Edge Stack 1.3.2 [helm].
-> Ambassador was installed with Helm: will continue setup...
-> Checking the AES pod deployment
...

3 - Installation after an old Helm chart

  • Same as previous scenario. Check that edgectl install aborts with a message saying it does not support upgrades.

4 - Installation after applying manifests from https://www.getambassador.io/yaml/*.yaml

  • Same as previous scenario. Check that edgectl install aborts with a message saying it does not support upgrades.

cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
@inercia inercia changed the base branch from release/v1.3.2 to release/v1.3 March 27, 2020 16:29
@inercia inercia changed the title [WIP] Use Helm for installing AES Use Helm for installing AES Apr 1, 2020
@inercia inercia requested a review from ark3 April 1, 2020 08:51
@alexgervais alexgervais self-requested a review April 1, 2020 13:20
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
ark3
ark3 previously requested changes Apr 1, 2020
Copy link
Contributor

@ark3 ark3 left a comment

Choose a reason for hiding this comment

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

Looks good overall, but we need to be careful with a few details.

cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
@inercia inercia requested a review from ark3 April 2, 2020 12:30
@inercia inercia dismissed stale reviews from alexgervais and ark3 April 2, 2020 12:31

Alread addressed comments. Thanks!

@inercia inercia force-pushed the inercia/edgectl_helm branch 3 times, most recently from 58db0b9 to 81e538a Compare April 3, 2020 11:13
@inercia inercia changed the base branch from release/v1.3 to master April 3, 2020 11:13
@inercia inercia force-pushed the inercia/edgectl_helm branch 5 times, most recently from b1f15bd to d943ac9 Compare April 3, 2020 14:18
Copy link
Contributor

@alexgervais alexgervais left a comment

Choose a reason for hiding this comment

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

Just a small change and some questions:

  • Can (and where) we set the app.kubernetes.io/managed-by=edgectl label for each install?
  • There are a few newly introduced or leftover TODOs. Do we want to address them in this PR?

cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
@inercia inercia force-pushed the inercia/edgectl_helm branch 3 times, most recently from 2602768 to dae17c2 Compare April 4, 2020 16:53
ark3
ark3 previously approved these changes Apr 6, 2020
cmd/edgectl/aes_install.go Outdated Show resolved Hide resolved
@inercia inercia force-pushed the inercia/edgectl_helm branch 5 times, most recently from d8187a2 to 7ced5c2 Compare April 8, 2020 20:19
@inercia inercia requested a review from ark3 April 8, 2020 20:29
@inercia inercia changed the base branch from master to release/v1.4.1 April 9, 2020 16:03
@khussey
Copy link
Contributor

khussey commented Apr 9, 2020

@inercia it seems to me that we should be adding new "help" pages in conjunction with these changes, to cover the new Helm-specific failure cases. These cases should have new documentation URLs assigned to them (rather than repurposing existing ones), so that users who have downloaded the existing non-Helm installer can still find help for the obsolete failure cases.

We'll also need to think about the best way to deprecate the obsolete help pages that no longer apply (perhaps by adding a message at the top of them indicating that a newer version of the installer is available and that they should download it for an improved experience)...

Please coordinate with @brucehorn and @ark3 on this to account for the changes they are making to the error messages and documentation pages.

@khussey khussey requested a review from brucehorn April 9, 2020 20:37
@inercia inercia dismissed alexgervais’s stale review April 10, 2020 10:38

Already addressed comments.

Copy link
Contributor

@khussey khussey left a comment

Choose a reason for hiding this comment

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

@inercia please see comments, thanks.

cmd/edgectl/aes_install_errors.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install_errors.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install_errors.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install_errors.go Outdated Show resolved Hide resolved
cmd/edgectl/aes_install_errors.go Show resolved Hide resolved
docs/topics/install/help/index.md Outdated Show resolved Hide resolved
Signed-off-by: Alvaro Saurin <alvaro.saurin@gmail.com>
@inercia inercia requested a review from khussey April 10, 2020 14:30
@inercia inercia dismissed khussey’s stale review April 10, 2020 14:30

I think I have addressed all the comments...

@inercia inercia merged commit cc93b4f into release/v1.4.1 Apr 10, 2020
@inercia inercia deleted the inercia/edgectl_helm branch April 10, 2020 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants