feat(ecs): Return application load balancer info from LB provider #4526
Conversation
|
related: spinnaker/deck#8186 |
|
CC @clareliguori + @pkandasamy91 for review |
There was a problem hiding this comment.
@allisaurus This is awesome! I have a couple minor comments but overall it looks great!
...main/java/com/netflix/spinnaker/clouddriver/ecs/cache/client/EcsLoadbalancerCacheClient.java
Show resolved
Hide resolved
...main/java/com/netflix/spinnaker/clouddriver/ecs/cache/client/EcsLoadbalancerCacheClient.java
Outdated
Show resolved
Hide resolved
| @@ -54,6 +54,6 @@ public String getName() { | |||
|
|
|||
| @Override | |||
| public String getType() { | |||
| return loadBalancerType; | |||
| return cloudProvider; | |||
There was a problem hiding this comment.
Is there a reason why type returns the ecscloudprovider id? Shouldn't the getCloudProvider call be used instead?
There was a problem hiding this comment.
Shouldn't the getCloudProvider call be used instead?
it should! getType was actually deprecated in favor of getCloudProvider, however it's still a part of the the LoadBalancer interface which is why the method is still here.
Returning cloudProvider instead of loadBalancerType I think aligns it better to it's intended purpose (plus loadBalancerType should always be "application" in the case of ECS, vs. AWS LBs, which is where we're sourcing the cache data), however @clareliguori might know more.
There was a problem hiding this comment.
The @DaTa annotation on the class should already provide the default setters and getters. In this case, I don't think you'll need to override any of the accessors (as long as you update the methods calling getType to call getCloudProvider.
There was a problem hiding this comment.
As discussed offline, this makes sense in order to maintain consistency with the other providers. Since the interface only has a getName and getType method, it would potentially break in many places if we add logic to create a new getProvider method.
.../src/main/java/com/netflix/spinnaker/clouddriver/ecs/model/loadbalancer/EcsLoadBalancer.java
Show resolved
Hide resolved
| @@ -92,27 +106,116 @@ public String getCloudProvider() { | |||
|
|
|||
| @Override | |||
| public Item get(String name) { | |||
| return null; // TODO - Implement this. | |||
| // unused method | |||
| throw new UnsupportedOperationException("Not implemented."); | |||
There was a problem hiding this comment.
Not sure if we want to throw an exception here. Previously, this would just return null but allow users to continue, where as this would now throw an exception and potetially break and implementations that use this.
There was a problem hiding this comment.
My concern with always returning null is the client will proceed like they have correct information vs. understanding that this method should not be used. But I understand the concern about breaking folks; I'll take another look at where this (and the other such method) may cause problems and add more info here.
There was a problem hiding this comment.
Update: going back to returning null but adding more descriptive comments. Other providers vary between returning null and OperationNotSupportedException, indicating they are not required by deck, however hitting the associated endpoints directly now results in a 500. Would rather leave existing behavior intact if both have precedence.
There was a problem hiding this comment.
I'm not opposed to returning the OperationNotSupportedException, but we may want to open an issue, and let the community know prior to making the change.
| return null; // TODO - Implement this. This is used to show the details view of a load balancer | ||
| // which is not even implemented yet | ||
| // unused method | ||
| throw new UnsupportedOperationException("Not implemented."); |
...c/main/java/com/netflix/spinnaker/clouddriver/ecs/provider/view/EcsLoadBalancerProvider.java
Outdated
Show resolved
Hide resolved
...roovy/com/netflix/spinnaker/clouddriver/ecs/provider/view/EcsLoadBalancerProviderSpec.groovy
Outdated
Show resolved
Hide resolved
allisaurus
force-pushed
the
ecs-lbs
branch
2 times, most recently
from
d066776
to
d155c9b
Compare
There was a problem hiding this comment.
Ship it! 💯 This is awesome! Thanks for fixing this @allisaurus!
Implements spinnaker/spinnaker#4797
Provides application load balancer and target group information for ECS server groups.