Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

[D2IQ-71860] Ambassador Preview #524

Merged
merged 1 commit into from
Oct 2, 2020
Merged

[D2IQ-71860] Ambassador Preview #524

merged 1 commit into from
Oct 2, 2020

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Sep 21, 2020 •
edited
Loading

What type of PR is this?
Feature

What this PR does/ why we need it:
Adds Ambassador as an addon in KBA.

Which issue(s) this PR fixes:
https://jira.d2iq.com/browse/D2IQ-71860

Does this PR introduce a user-facing change?:

* added Ambassador addon

Checklist

  • The commit message explains the changes and why are needed.
  • The code builds and passes lint/style checks locally.
  • The relevant subset of integration tests pass locally.
  • The core changes are covered by tests.
  • The documentation is updated where needed.

@shaneutt shaneutt requested a review from a team as a code owner September 21, 2020 16:32
@shaneutt shaneutt self-assigned this Sep 21, 2020
@shaneutt shaneutt requested a review from branden September 21, 2020 16:32
@shaneutt shaneutt added enhancement New feature or request ready and removed dont-review-yet wip labels Sep 21, 2020
GoelDeepak
GoelDeepak suggested changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@GoelDeepak GoelDeepak left a comment

Choose a reason for hiding this comment

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

Preview isn't in the list of requirements. Ambassador will be a supported addon

@shaneutt shaneutt force-pushed the shaneutt/ambassador-addon branch from aadc910 to a701db0 Compare September 22, 2020 12:16
@shaneutt
Copy link
Contributor Author

Preview isn't in the list of requirements. Ambassador will be a supported addon

This has been removed, ready for re-review. I pinged you in slack as I want to discuss this some before we merge.

@shaneutt shaneutt requested a review from GoelDeepak September 22, 2020 12:17
@shaneutt shaneutt requested a review from a team September 22, 2020 12:28
@shaneutt
Copy link
Contributor Author

We're going to need to include cert-manager updates which are inbound, so for the moment I'm going to close this but keep the branch, the intention being to re-open with cert-manager tests once we're unblocked.

@shaneutt shaneutt closed this Sep 22, 2020
@shaneutt shaneutt reopened this Oct 2, 2020
@shaneutt
Copy link
Contributor Author

shaneutt commented Oct 2, 2020

We've decided to do a "Preview" version after all, sans cert-manager integration for the time being.

@shaneutt shaneutt force-pushed the shaneutt/ambassador-addon branch from 980147d to bf2e345 Compare October 2, 2020 17:41
@shaneutt shaneutt requested a review from vespian October 2, 2020 17:41
@shaneutt shaneutt merged commit 4622b26 into master Oct 2, 2020
@shaneutt shaneutt deleted the shaneutt/ambassador-addon branch October 2, 2020 21:54
vespian
vespian reviewed Oct 4, 2020
View reviewed changes
test/ambassador_test.go
Name: "http",
ContainerPort: 8080,
}}}}}}}}
deployment, err := cluster.Client().AppsV1().Deployments(ns).Create(context.TODO(), &testDeployment, metav1.CreateOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to wait for deployment to finish before we continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right that we probably do want to do this, I'll make an update for it.

@dkoshkin
Copy link
Contributor

dkoshkin commented Oct 5, 2020

We probably need a better method, but for Calico we should atleast add this similar to istio https://github.com/mesosphere/konvoy/blob/master/clientapis/pkg/kubernetes/encoder/encoders.go#L20

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vespian vespian vespian left review comments

@GoelDeepak GoelDeepak GoelDeepak approved these changes

@branden branden branden approved these changes

@tillt tillt Awaiting requested review from tillt

@alejandroEsc alejandroEsc Awaiting requested review from alejandroEsc

@armandgrillet armandgrillet Awaiting requested review from armandgrillet

@joejulian joejulian Awaiting requested review from joejulian

@makkes makkes Awaiting requested review from makkes

Assignees

@shaneutt shaneutt

Labels
enhancement New feature or request LetMeMerge ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shaneutt @dkoshkin @branden @GoelDeepak @vespian