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

Get rid of aws-codedeploy-api and asCodeDeployLoadBalancer #2449

Closed
eladb opened this issue May 1, 2019 · 3 comments · Fixed by #2548 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Assignees
Labels
@aws-cdk/aws-codedeploy Related to AWS CodeDeploy feature-request A feature should be added or improved.

Comments

@eladb
Copy link
Contributor

eladb commented May 1, 2019

I don't see a problem for the aws-codedeploy module to take a dependency on aws-elbv2 and aws-elb, eliminating the need for this intricate type hierarchy.

@skinny85
Copy link
Contributor

skinny85 commented May 1, 2019

A little bit of an explanation why it was modeled this way.

We wanted to pass either a Classic ELB, or an ALB/NLB Target Group directly to the loadBalancer property of a CodeDeploy Deployment Group when creating it:

new codedeploy.DeploymentGroup(this, 'DG', {
  // ...
  loadBalancer: new elasticloadbalancing.LoadBalancer(this, 'ELB', {
    // ...
  }),
});

Because CodeDeploy depends on aws-autoscaling (for AutoScalingGroup), it has a transitive dependency on both aws-elasticloadbalancing and aws-elasticloadbalancingv2, and so none of those packages could have a type from aws-codedeploy that could be implemented by both elasticloadbalancing.LoadBalancer and elasticloadbalancingv2.TargetGroup (as that would be a cycle).

If you want to get rid of this dependency, we need to change the current API of DeploymentGroup.

@eladb
Copy link
Contributor Author

eladb commented May 1, 2019

Yes, let's change the API.

Would it make sense to use "union-like classes":

loadBalancer: codedeploy.LoadBalancer.classic(elb)
// or
loadBalancer: codedeploy.LoadBalancer.targetGroup(group)

@skinny85
Copy link
Contributor

skinny85 commented May 1, 2019

Yeah, that's probably the only sensible way to go here.

@skinny85 skinny85 added @aws-cdk/aws-codedeploy Related to AWS CodeDeploy feature-request A feature should be added or improved. labels May 7, 2019
@eladb eladb added the may-15 label May 14, 2019
skinny85 added a commit to skinny85/aws-cdk that referenced this issue May 15, 2019
skinny85 added a commit to skinny85/aws-cdk that referenced this issue May 15, 2019
…oup.

BREAKING CHANGE: the type of the `loadBalancer` property in ServerDeploymentGroupProps has been changed.

Fixes aws#2449
skinny85 added a commit to skinny85/aws-cdk that referenced this issue May 15, 2019
…oup.

BREAKING CHANGE: the type of the `loadBalancer` property in ServerDeploymentGroupProps has been changed.

Fixes aws#2449
skinny85 added a commit that referenced this issue May 16, 2019
…oup. (#2548)

BREAKING CHANGE: the type of the `loadBalancer` property in ServerDeploymentGroupProps has been changed.

Fixes #2449
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment