Skip to content

Port ECS deploy command from SPF#1897

Merged
sathvikkumar-octo merged 18 commits intomainfrom
sk/ecs-deploy-service
Apr 30, 2026
Merged

Port ECS deploy command from SPF#1897
sathvikkumar-octo merged 18 commits intomainfrom
sk/ecs-deploy-service

Conversation

@sathvikkumar-octo
Copy link
Copy Markdown
Contributor

@sathvikkumar-octo sathvikkumar-octo commented Apr 22, 2026

ECS deploy SPF step replacement in Calamari

Ref MD-1813

  • A new Calamari command that wires up CFN deploy + ECS-specific input handling, default tag merging, and output variables matching the SPF contract.
  • A default stack-name generator (cf-ecs-{cluster}-{service}-{env}-{tenant}) used when the user doesn't supply one.
  • A 1:1 port of lodash's camelCase so Calamari and SPF produce byte-for-byte identical default stack names so that migrating from SPF to Calamari execution doesn't cause new CFN stacks to be created

CFN convention tweaks

  • Optional waitTimeout on the wait-for-stack helper; throws on elapse.
  • Drops an unused roleArnProvider parameter from the CFN convention constructor.

@sathvikkumar-octo sathvikkumar-octo marked this pull request as ready for review April 27, 2026 02:20
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

God have mercy on our souls!

Let's mark this class as [Deprecated] from the get go so there's a persistent reminder to replace it.

@zentron Flagging that we need to chat about this

TemplateFactory,
stackEventLogger,
StackProvider,
RoleArnProvider,
Copy link
Copy Markdown
Contributor

@Jtango18 Jtango18 Apr 27, 2026

Choose a reason for hiding this comment

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

🚩 This is passed down to the template and attached to the Stack. or am I missing something here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm it seemed unused to me in the convention.

Regardless I'll pull it out into a separate PR

Copy link
Copy Markdown
Contributor

@Jtango18 Jtango18 Apr 27, 2026

Choose a reason for hiding this comment

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

public override CreateStackRequest BuildCreateStackRequest()
{
    return new CreateStackRequest
    {
        StackName = stackName,
        TemplateBody = Content,
        Parameters = Inputs.ToList(),
        Capabilities = capabilities,
        DisableRollback = disableRollback,
        RoleARN = roleArn,
        Tags = tags
    };
}

public override UpdateStackRequest BuildUpdateStackRequest()
{
    return new UpdateStackRequest
    {
        StackName = stackName,
        TemplateBody = Content,
        Parameters = Inputs.ToList(),
        Capabilities = capabilities,
        RoleARN = roleArn,
        Tags = tags
    };
}

public override async Task<CreateChangeSetRequest> BuildChangesetRequest()
{
    return new CreateChangeSetRequest
    {
        StackName = stack.Value,
        TemplateBody = Content,
        Parameters = Inputs.ToList(),
        /*
         * The change set name might be passed down directly, or this variable may be
         * set as part of the deployment. Reading the value from the variables here
         * allows us to catch any deferred construction of the change stack name.
         */
        ChangeSetName = variables[AwsSpecialVariables.CloudFormation.Changesets.Name],
        ChangeSetType = await GetStackStatus() == StackStatus.DoesNotExist ? ChangeSetType.CREATE : ChangeSetType.UPDATE,
        Capabilities = capabilities,
        RoleARN = roleArn,
        Tags = tags
    };
}

It is passed to the SDK Request classes

TemplateFactory,
stackEventLogger,
StackProvider,
_ => null,
Copy link
Copy Markdown
Contributor

@Jtango18 Jtango18 Apr 27, 2026

Choose a reason for hiding this comment

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

nit: While this appears to make sense in the S3 case (vs. Below). Let's pull this out and do it as it's own little PR

Copy link
Copy Markdown
Contributor

@Jtango18 Jtango18 left a comment

Choose a reason for hiding this comment

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

Looks good. A few minor nits and concern about breaking existing functionality.

Reverts the earlier drop of the roleArnProvider parameter; existing
callers keep their previous wiring and the new ECS step passes null.
@sathvikkumar-octo sathvikkumar-octo enabled auto-merge (squash) April 29, 2026 06:49
JT's commit added [Obsolete] to CamelCase but missed using System; for the
attribute, and triggered TreatWarningsAsErrors on the two callers. Adds the
missing using and suppresses CS0618 at the call sites that intentionally
depend on the lodash port for SPF parity.
@sathvikkumar-octo sathvikkumar-octo merged commit 4da5a33 into main Apr 30, 2026
33 checks passed
@sathvikkumar-octo sathvikkumar-octo deleted the sk/ecs-deploy-service branch April 30, 2026 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants