-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: adds ability to create new task revisions, and optionally deploy #286
Conversation
e3c1dad
to
1075946
Compare
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.
Looks good.
I'm not familiar with EventBridge or System Store parameters so need to read up to know exactly what's going on there.
rule-name: | ||
required: false | ||
type: string | ||
description: | | ||
name of rule to update, if deploying |
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.
Not immediately clear what this is.
container: | ||
required: false | ||
type: string | ||
description: | | ||
name of container | ||
task-name: | ||
required: false | ||
type: string | ||
description: | | ||
name of task definition |
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 these need a , if deploying
caveat on the description like the others?
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.
no, task name / container is always required, so I'll update it to required: true
skip-workflow: | ||
required: false | ||
type: boolean | ||
default: false | ||
description: | | ||
skip this workflow |
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.
Is this used?
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.
thanks, will remove (it's a relic from the past)
type of deployment, valid values are | ||
ecs, eventbridge, or empty for no deployment | ||
deployment-tag-param-name: | ||
required: false |
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.
What happens if deploying but this is left blank?
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.
The param will have either the value of Null
, or the previous deployment information.
On the next terraform apply
, the container tag will be latest
, or the previously deployed container, respectively.
README.md
Outdated
@@ -1117,6 +1118,92 @@ jobs: | |||
``` | |||
|
|||
|
|||
### AWS deploy | |||
|
|||
STATUS: stable |
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.
Bold move.
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.
Perhaps premature. I had intended it to signify I don't expect any breaking changes to occur, once this is in production. I will mark as Beta: I consider it complete and ready to be declared as stable, subject to public testing.
8c964c0
to
da938f5
Compare
Allows a workflow to update a task definition with updated container, and deploy new task revision.
da938f5
to
f0bf4c5
Compare
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.
🚀
This PR adds a new workflow allowing for CICD-based deployments.
Typically, services using this workflow would be configured in Terraform (see https://github.com/GeoNet/terraform-aws/pull/1151).
Re: deployment generally, I see this as an option, not the option. I think Terraform-based deployments / Terraform-tracked explicit deployment tags can co-exist with CICD-based deployments.