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.

Sign up for GitHub

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

Jump to bottom

fix: when no input enableECSManagedTagsInput, not include it to request params #669

Merged
merged 9 commits into from
Nov 26, 2024
Merged

fix: when no input enableECSManagedTagsInput, not include it to request params #669

merged 9 commits into from
Nov 26, 2024

Conversation

wim-web
Copy link
Contributor

@wim-web wim-web commented Nov 1, 2024

Description of changes:

If enableECSManagedTags is set to true by IaC, such as Terraform, and enable-ecs-managed-tags is not specified in the input for this action, each time the action is run, enableECSManagedTags will rollback to false.

you would think it is good to specify enable-ecs-managed-tags, but it is need to modify both code.

So modified the code to leave it to the infrastructure side setting unless explicitly specify it in action.

ref: #622

@wim-web
Copy link
Contributor Author

wim-web commented Nov 1, 2024

@kg-aws

@wim-web wim-web changed the title when no input enableECSManagedTagsInput, not include it to request params fix: when no input enableECSManagedTagsInput, not include it to request params Nov 5, 2024
@wim-web
Copy link
Contributor Author

wim-web commented Nov 16, 2024

@kg-aws @amazreech

some CI is failed.
Semantic Pull Request is fixed by modify title.
I don’t understand the intent behind Package distribution file. Could you tell me how to fix it?

@amazreech
Copy link
Contributor

Hi @wim-web

I believe, to fix the Package distribution tests you would just need to make sure that the latest code is packaged and pushed. This can be verified/done by running the npm run package command and pushing in all changes along with the changes in dist/ folder/

@wim-web
Copy link
Contributor Author

wim-web commented Nov 18, 2024

thanks for response @amazreech

fixed it by commiting result executed npm run package!

@wim-web
handle enableECSManagedTagsInput
a392108 
when no input enableECSManagedTagsInput, not include it to request params
@wim-web
npm run package
b29b8ef
@wim-web
npm ci && npm run package
8b77a4d
@wim-web
merge master
67bbaad 
commit 0f24c17
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Nov 19 16:08:17 2024 +0000

    chore: Bump @aws-sdk/client-codedeploy from 3.687.0 to 3.693.0 (aws-actions#682)

    * chore: Bump @aws-sdk/client-codedeploy from 3.687.0 to 3.693.0

    Bumps [@aws-sdk/client-codedeploy](https://github.com/aws/aws-sdk-js-v3/tree/HEAD/clients/client-codedeploy) from 3.687.0 to 3.693.0.
    - [Release notes](https://github.com/aws/aws-sdk-js-v3/releases)
    - [Changelog](https://github.com/aws/aws-sdk-js-v3/blob/main/clients/client-codedeploy/CHANGELOG.md)
    - [Commits](https://github.com/aws/aws-sdk-js-v3/commits/v3.693.0/clients/client-codedeploy)

    ---
    updated-dependencies:
    - dependency-name: "@aws-sdk/client-codedeploy"
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

    * chore: Update dist

    ---------

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
    Co-authored-by: GitHub Actions <runner@fv-az2021-651.3wxedyhetzae5an0xyfe2w1byg.dx.internal.cloudapp.net>
    Co-authored-by: Reecha Khanal <136762730+amazreech@users.noreply.github.com>

commit 103f366
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Nov 19 05:12:34 2024 +0000

    chore: Bump @vercel/ncc from 0.38.2 to 0.38.3 (aws-actions#683)

    Bumps [@vercel/ncc](https://github.com/vercel/ncc) from 0.38.2 to 0.38.3.
    - [Release notes](https://github.com/vercel/ncc/releases)
    - [Commits](vercel/ncc@0.38.2...0.38.3)

    ---
    updated-dependencies:
    - dependency-name: "@vercel/ncc"
      dependency-type: direct:development
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 7a7648f
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Nov 19 05:10:17 2024 +0000

    chore: Bump eslint from 9.14.0 to 9.15.0 (aws-actions#681)

    Bumps [eslint](https://github.com/eslint/eslint) from 9.14.0 to 9.15.0.
    - [Release notes](https://github.com/eslint/eslint/releases)
    - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
    - [Commits](eslint/eslint@v9.14.0...v9.15.0)

    ---
    updated-dependencies:
    - dependency-name: eslint
      dependency-type: direct:development
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 38ef14b
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Nov 19 05:07:46 2024 +0000

    chore: Bump @eslint/js from 9.14.0 to 9.15.0 (aws-actions#679)

    Bumps [@eslint/js](https://github.com/eslint/eslint/tree/HEAD/packages/js) from 9.14.0 to 9.15.0.
    - [Release notes](https://github.com/eslint/eslint/releases)
    - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
    - [Commits](https://github.com/eslint/eslint/commits/v9.15.0/packages/js)

    ---
    updated-dependencies:
    - dependency-name: "@eslint/js"
      dependency-type: direct:development
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@wim-web
npm run package
8e0686f
@wim-web
npm ci & npm run package
37e54f1
@wim-web
Merge branch 'master' into fix/input_default_value
c99c574
@wim-web
Copy link
Contributor Author

wim-web commented Nov 25, 2024

@kg-aws @amazreech
could you review this?

karanbokil
karanbokil requested changes Nov 25, 2024
View reviewed changes
index.js Outdated
@@ -398,7 +401,7 @@ async function run() {
const forceNewDeployInput = core.getInput('force-new-deployment', { required: false }) || 'false';
const forceNewDeployment = forceNewDeployInput.toLowerCase() === 'true';
const desiredCount = parseInt((core.getInput('desired-count', {required: false})));
const enableECSManagedTagsInput = core.getInput('enable-ecs-managed-tags', { required: false }) || 'false';
const enableECSManagedTagsInput = core.getInput('enable-ecs-managed-tags', { required: false }) || '';
const enableECSManagedTags = enableECSManagedTagsInput.toLowerCase() === 'true';
Copy link

Choose a reason for hiding this comment

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

This is still being used in runTask - can we include the changes for the enable ecs managed tags behavior in runTask as well to keep the behavior consistent?

Copy link

Choose a reason for hiding this comment

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

In fact we can just move

  enableECSManagedTags = ""
  if (enableECSManagedTagsInput !== "") {
   enableECSManagedTags = enableECSManagedTagsInput.toLowerCase() === 'true';
  }

here and set the param in updateEcsService and runTask only if it's not set to ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when enableECSManagedTags = “”, ecs.updateService(params) get an error "SerializationException: Unexpected token received from parser"
so enableECSManagedTags is set to null by default

I actually checked with the following code in my aws

const ecs = new ECS();
  
let params = {
    cluster: "cluster-name",
    service: "servicepname",
    taskDefinition: "arn:aws:ecs:us-east-1:000000000:task-definition/some-task:1",
    enableECSManagedTags: "", // parse error
    // enableECSManagedTags: null, // ok
    desiredCount: 1,
};

await ecs.updateService(params);

Copy link

Choose a reason for hiding this comment

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

Great find!

index.test.js Outdated Show resolved Hide resolved
@wim-web
Copy link
Contributor Author

wim-web commented Nov 26, 2024

@karanbokil
tried to fix it, and add test

karanbokil
karanbokil approved these changes Nov 26, 2024
View reviewed changes
Copy link

@karanbokil karanbokil left a comment

Choose a reason for hiding this comment

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

LGTM!

@wim-web
Copy link
Contributor Author

wim-web commented Nov 26, 2024

thank you for review
what should i do next?

@karanbokil karanbokil merged commit e4558ed into aws-actions:master Nov 26, 2024
6 checks passed
@karanbokil
Copy link

Change is merged! It should be integrated in the next release this month, but till then you can also configure your action to track HEAD to use the feature early

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karanbokil karanbokil karanbokil approved these changes

@amazreech amazreech amazreech approved these changes

@omkhegde omkhegde omkhegde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wim-web @amazreech @karanbokil @omkhegde