org-formation / org-formation-cli

Better than landingzones!
MIT License
1.41k stars 131 forks source link

feat: support sls v3 with custom task parameter #468

Closed davidvpe closed 1 year ago

davidvpe commented 1 year ago

This PR aims to fix https://github.com/org-formation/org-formation-cli/issues/303

At the moment, parameters in update-serverless task are being rendered as --paramName="paramValue" but since serverless v3 this has changed to --param="paramName=paramValue"

In order to support both sls v2 and sls v3, a new task parameter for update-serverless.com has been introduced.

SLSVersion lets you specify the sls version to use when rendering parameters. If SLSVersion is not specified it will default to v2 as it currently is.

OlafConijn commented 1 year ago

@davidvpe thanks for putting this together, I'll make sure to look at it within the next day or so.

OlafConijn commented 1 year ago

thanks! added an extra test on the differences in version. looks good. I might add an integration test later, but for now, published this as 1.0.9-beta.1.

you can download and run it after installing this version: npm i aws-organization-formation@1.0.9-beta.1.

approved the PR.

davidvpe commented 1 year ago

Thanks @OlafConijn we'll give it a spin on our end

OlafConijn commented 1 year ago

hi @davidvpe, did you manage to give this a try? looks relatively risk free. was planning to bundle a couple of fixes/features in a new release. let me know!

davidvpe commented 1 year ago

Hi @OlafConijn yes I gave it a try locally and made a deployment in sls v3 with no issues, however whoever uses this with CustomDeployCommand, will need to make sure to use also "Config" task parameter, since this cannot be passed in the Parameters section of the task anymore (our case) same applies for "Region" and "Stage" as these are special SLS params (org-formation allows to specify these as Task Parameters), but this is not a breaking change, if anything just part of the migration to sls v3.

OlafConijn commented 1 year ago

Hi @OlafConijn yes I gave it a try locally and made a deployment in sls v3 with no issues, however whoever uses this with CustomDeployCommand, will need to make sure to use also "Config" task parameter, since this cannot be passed in the Parameters section of the task anymore (our case) same applies for "Region" and "Stage" as these are special SLS params (org-formation allows to specify these as Task Parameters), but this is not a breaking change, if anything just part of the migration to sls v3.

going off the serverless.com documentation here. I dont believe the --stage, --config or --region options have changed from v1 to v2 - did they?

thanks.

davidvpe commented 1 year ago

Hi @OlafConijn yes I gave it a try locally and made a deployment in sls v3 with no issues, however whoever uses this with CustomDeployCommand, will need to make sure to use also "Config" task parameter, since this cannot be passed in the Parameters section of the task anymore (our case) same applies for "Region" and "Stage" as these are special SLS params (org-formation allows to specify these as Task Parameters), but this is not a breaking change, if anything just part of the migration to sls v3.

going off the serverless.com documentation here. I dont believe the --stage, --config or --region options have changed from v1 to v2 - did they?

thanks.

Hi @OlafConijn

To give you some context, this is one of our tasks:

TestTask:
    Type: update-serverless.com
    Path: ./serverless
    OrganizationBinding:
        Account:
            - !Ref TestAccount
        Region: !Ref testRegion
    RunNpmInstall: false
    Config: serverless-test.yml
    CustomDeployCommand: !Sub 'npx sls deploy ${CurrentTask.Parameters} --verbose'
    CustomRemoveCommand: !Sub 'npx sls remove ${CurrentTask.Parameters} --verbose'
    Parameters:
        config: serverless-test.yml
        stage: tst

As you can see we are using CustomDeployCommand and not using ${CurrentTask.Stage} nor ${CurrentTask.Config} but passing them on as Parameters (which worked just fine), but in v3, if we left that the way it was then the task would fail, since stage and config won't be transformed to --config or --stage but --param config=something --param stage=something.

TLDR;

If someone else is doing the same, then they will need to update their task and start using Task's Stage and Config for the CustomDeployCommand

OlafConijn commented 1 year ago

Hi @OlafConijn yes I gave it a try locally and made a deployment in sls v3 with no issues, however whoever uses this with CustomDeployCommand, will need to make sure to use also "Config" task parameter, since this cannot be passed in the Parameters section of the task anymore (our case) same applies for "Region" and "Stage" as these are special SLS params (org-formation allows to specify these as Task Parameters), but this is not a breaking change, if anything just part of the migration to sls v3.

going off the serverless.com documentation here. I dont believe the --stage, --config or --region options have changed from v1 to v2 - did they? thanks.

Hi @OlafConijn

To give you some context, this is one of our tasks:

TestTask:
    Type: update-serverless.com
    Path: ./serverless
    OrganizationBinding:
        Account:
            - !Ref TestAccount
        Region: !Ref testRegion
    RunNpmInstall: false
    Config: serverless-test.yml
    CustomDeployCommand: !Sub 'npx sls deploy ${CurrentTask.Parameters} --verbose'
    CustomRemoveCommand: !Sub 'npx sls remove ${CurrentTask.Parameters} --verbose'
    Parameters:
        config: serverless-test.yml
        stage: tst

As you can see we are using CustomDeployCommand and not using ${CurrentTask.Stage} nor ${CurrentTask.Config} but passing them on as Parameters (which worked just fine), but in v3, if we left that the way it was then the task would fail, since stage and config won't be transformed to --config or --stage but --param config=something --param stage=something.

TLDR;

If someone else is doing the same, then they will need to update their task and start using Task's Stage and Config for the CustomDeployCommand

gotcha, seems fair enough! thanks for clarifying