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

fix(amazon): Fix SpEL support for load balancers #7151

Merged

Conversation

alanmquach
Copy link
Contributor

Looks like we were actually 99% of the way there for supporting SpEL load balancers.

The only thing we were missing was to append the expression'ed load balancers into the list of valid options so that they don't get filtered out when we run _.intersection to filter out invalid load balancers.

@@ -474,10 +474,10 @@ export class AwsServerGroupConfigurationService {
const vpcLoadBalancers = this.getVpcLoadBalancerNames(command);
const allTargetGroups = this.getTargetGroupNames(command);

if (currentLoadBalancers && command.loadBalancers && !currentLoadBalancers.includes('${')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

!currentLoadBalancers.includes('${') was always true because currentLoadBalancers, an array, almost never includes '${' as an item. Probably should've been something like !currentLoadBalancers.some(lb => lb.includes('${')) but I don't think we want that now either.

Copy link
Contributor

@anotherchrisberry anotherchrisberry left a comment

Choose a reason for hiding this comment

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

I don't know if this will save the world but it's worth trying.

@alanmquach alanmquach merged commit 8ee7e11 into spinnaker:master Jun 26, 2019
@alanmquach alanmquach deleted the save-the-loadbalancers-save-the-world branch June 26, 2019 23:34
anotherchrisberry added a commit that referenced this pull request Jun 27, 2019
* chore(core): Bump version to 0.0.378

058007e fix(core): properly update execution permalink on location change (#7152)
9bd9624 fix(projects): Fixed project dashboard to application link (#7154)
d6ad8c2 feat(appengine): Enable new artifacts for config artifacts in deploy SG (#7153)
a9a927b feat(appengine): Enable new artifacts workflow in deploy SG (#7147)

* chore(amazon): Bump version to 0.0.195

8ee7e11 fix(amazon): Fix SpEL support for load balancers (#7151)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants