-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Require service-name as input #3
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
Conversation
Move service-name to the top of required input validations since it's now a required parameter. This provides faster feedback to users if the service name is missing. Changes: - Read and validate service-name first among required inputs - Remove duplicate service-name declarations - Fix test mocks that had duplicate service-name entries - All 17 tests passing
Move service-name to be the first input in action.yml since it's now a required parameter. This makes the action.yml structure consistent with the validation order in index.js. Changes: - Move service-name from 'Service identification' section to top - Place it as the first required input - Simplify description to match index.js comment - All 17 tests passing
| core.info('Checking if service exists...'); | ||
| const describeCommand = new DescribeServicesCommand({ | ||
| cluster: clusterName, | ||
| services: [serviceName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still trim() here?
| core.info('Service does not exist, will create new service'); | ||
| } | ||
| } catch (error) { | ||
| if (error.name === 'ServiceNotFoundException' || error.name === 'ClusterNotFoundException') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like ServiceNotFoundException is ever thrown: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DescribeServices.html
| } else { | ||
| throw error; | ||
| core.info('Service exists but is INACTIVE, will create new service'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to handle the case where service is in DRAINING state here? Prolly an error if a service is DRAINING
https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_Service.html#ECS-Type-Service-status
In this PR:
Requiring the serviceName on the input since the GitHub Action does not store state, and would break if we had to re-use it across requests
In this PR: