-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add Ambassador Ingress controller as an addon #8161
Conversation
Hi @concaf. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
Welcome @concaf! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: concaf The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can one of the admins verify this patch? |
@concaf this looks great ! thank you for adding this addon. do you mind sharing which driver and os did you try this on ? have you tried it on the docker driver ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @concaf -- thanks for submitting this PR! Any update on medya's questions?
94c49c9
to
a45fcbf
Compare
Codecov Report
@@ Coverage Diff @@
## master #8161 +/- ##
==========================================
- Coverage 34.60% 34.59% -0.02%
==========================================
Files 147 147
Lines 9378 9381 +3
==========================================
Hits 3245 3245
- Misses 5736 5739 +3
Partials 397 397
|
@medyagh @priyawadhwa sorry for the late reply!
I've tested this on the docker driver, on Fedora 30.
Nope, no such requirement. This pretty much requires the same stuff as the ingress addons. (The force pushes are due to a rebase and updating the image, nothing major) |
a45fcbf
to
81dabc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@concaf thanks! I think this change is big enough to warrant an integration test -- would you mind writing one?
The test would:
- Start a minikube cluster, and enable the addon by adding the
--addons=<addon name>
flag (Sample code here) - Makes sure the installation was successful (as in, some expected pod came up as
Running
) (sample code here) - Try to configure Ambassador via an Ingress resource and make sure it works
You can add TestAmbassadorAddon
to our addons_test.go file.
A lot of the code from TestAddons can be copied into TestAmbassadorAddon
(I linked some key lines above).
Docs for quickly running an integration test locally can be found here. Please let me know if you have any questions!
This PR adds Ambassador (https://www.getambassador.io/) as an addon to Minikube. Also, a tutorial has been added on how to use the same.
The motivation behind adding Ambassador as an addon is mostly around improving the UX around installing and configuring Ambassador. Ambassador is installed via our operator which makes sure than the Ambassador installation is healthy and up to date at all times.
CC: @brucehorn @inercia