feat: ECS Cluster Target Discovery #2013
Conversation
Jtango18
force-pushed
the
jt/spf-deprecation-replace-ecs-target-discovery
branch
2 times, most recently
from
5d67b08 to
b8c1e1a
Compare
Jtango18
force-pushed
the
jt/spf-deprecation-replace-ecs-target-discovery
branch
from
5482c90 to
0bdd0a1
Compare
| namespace Calamari.Aws.Integration.Ecs; | ||
|
|
||
| // TODO: Replace Static Helper with concrete class/interface to enable better testing | ||
| public static class EcsClientFactoryHelper |
There was a problem hiding this comment.
is this class adding value - could people just go direct to the factory?
There was a problem hiding this comment.
They could, but I'm focused addition with this PR, avoiding modifying other functionality until a future PR.
| var environment = AwsEnvironmentGeneration.Create(log, variables).GetAwaiter().GetResult(); | ||
|
|
||
| using var ecsClient = EcsClientFactory.Create(environment); | ||
| using var ecsClient = EcsClientFactoryHelper.Create(environment); |
There was a problem hiding this comment.
you can probably inject this as an autofac delegate factory (EcsClientFactory.Factory)
There was a problem hiding this comment.
I got chipped last time I did that :-(
Also, note from the above, that this will disappear in a future PR anyway - there's a lot of love for static in the AWS related code for some reason.
| { | ||
| discoveredTargetCount++; | ||
| log.Info($"Discovered matching ECS cluster '{cluster.ClusterName}' in {region}."); | ||
| clusterDiscoveryWriter.WriteTargetCreationServiceMessage(region, cluster, authentication, scope, matchResult); |
There was a problem hiding this comment.
standard question - IF we failed finding the 3rd cluster - should we have reported the first 2?
Eg: should this logic create a list of discovered clusters - then turn them into service msgs at the very end (Rather than after each discovery)?
There was a problem hiding this comment.
@zentron This is an intersting talking point. I've matched the existing behaviour, but it's worthy of further exploration moving forward (outside the confines of the SPF Deprecation Project)
There was a problem hiding this comment.
| public static readonly string Manifest = "Octopus.Steps.Manifest"; | ||
| } | ||
|
|
||
| public const string TargetDiscoveryContext = "Octopus.TargetDiscovery.Context"; |
There was a problem hiding this comment.
nit: is this the right place?
| await sut.Execute(deployment); | ||
|
|
||
| // Assert | ||
| fakeWriter.DidNotReceive() |
There was a problem hiding this comment.
nit: fake log to show no clusters found
zentron
left a comment
zentron
left a comment
There was a problem hiding this comment.
Feel free to merge when ready 👍
| { | ||
| discoveredTargetCount++; | ||
| log.Info($"Discovered matching ECS cluster '{cluster.ClusterName}' in {region}."); | ||
| clusterDiscoveryWriter.WriteTargetCreationServiceMessage(region, cluster, authentication, scope, matchResult); |
There was a problem hiding this comment.
3 participants
Implements Target Discovery for Conventional ECS Clusters as part of SPF Deprecation.
Centralises some AWS Credentials handling use by AKS for simplicity - more cleanup in a later PR
Server Change is staged here