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.

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

added task definition validation. #28

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

reiki4040
Copy link
Contributor

Added validation for Task Definition.

Background

Scheduled Task is fail if specified not defined Task Definition or Revision. And it is need to effort for catch this problem. (no ECS error, no Task log..., see CloudWatchMetrics FailedInvocations)

ecschedule is able to set undefined Task Definition and Revision. On AWS management console, Task Definition is able to select only from defined Task Definitions and Revisions. However Scheduled Task API(CloudWatchEvent) does not check defined Task Definition.

Solution

If specified undefined Task Definition and Revision, then error and exit command.

sample validation error message

[ecschedule] applying the rule "ScheduleTask1"
[ecschedule] 💢 task definition sample-task:5 is not defined: ClientException: Unable to describe task definition.

(ClientException: Unable to describe task definition. is return from ECS DescribeTaskDefinition API)

Breaking change in specific case.

I chose default do validate, because I guess almost case is not affected.

It will not be setable ScheduledTask that use undefined Task Definition after this change. Would you like to need Opt-Out option? (ex. -without-taskdef-validation) or change to Opt-In option? (ex. -validate-task-definition)

@Songmu
Copy link
Owner

Songmu commented Dec 28, 2021

Thanks for the detailed explanation.
I think this is appropriate as a guardrail, although it will increase the number of requests.
Also, you are concerned about the incompatibility change, but as you said, we don't expect to specify task definitions that don't exist, so I think we can keep this change without unnecessarily adding more options.
Thank you again.

@Songmu Songmu merged commit 78f3636 into Songmu:main Dec 28, 2021
Songmu added a commit that referenced this pull request Dec 28, 2021
## [v0.3.2](v0.3.1...v0.3.2) (2021-12-28)

* Go 1.17 and follow it in toolchains [#29](#29) ([Songmu](https://github.com/Songmu))
* added task definition validation. [#28](#28) ([reiki4040](https://github.com/reiki4040))
* Add procedure of execute `run` subcommand to README [#27](#27) ([gotoeveryone](https://github.com/gotoeveryone))
* Add "rules:" to the sample configuration [#25](#25) ([yuu26jp](https://github.com/yuu26jp))
* Add action.yml for GitHub Actions [#24](#24) ([mokichi](https://github.com/mokichi))
@reiki4040
Copy link
Contributor Author

thanks!!

@reiki4040 reiki4040 deleted the add/check-exists-task-definition branch December 28, 2021 07:49
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