chore(task): implement task package that runs one-off tasks #1139
Conversation
internal/pkg/task/task.go
Outdated
| return nil, fmt.Errorf("get security groups: %w", err) | ||
| } | ||
|
|
||
| arns, err := t.starter.RunTask(ecs.RunTaskInput{ |
There was a problem hiding this comment.
I'll add more comments later but you'll need to use the env role session here.
Louise15926
force-pushed
the
milestone-1-task
branch
from
3b0ded3 to
ecf0f26
Compare
internal/pkg/task/task.go
Outdated
| // NewTaskConfig contains fields required to initialize a task struct. | ||
| type NewTaskConfig struct { | ||
| // Count is the count of the tasks to be run | ||
| Count int | ||
| // GroupName is the name of the task group | ||
| GroupName string | ||
| } | ||
|
|
||
| // NewTask initializes a new task struct. | ||
| func NewTask(config NewTaskConfig) (*Task, error) { |
There was a problem hiding this comment.
I'm thinking we should consider stopping with constructors that initializes sessions and then our dependencies. Instead, we can do dependency injection from the "cli" pkg.
I’ve found that just writing code and just instantiating things, and just passing them in inside of main tends to be the best, most straightforward dependency injection I’ve seen. That’s all I do.
https://changelog.com/gotime/102
So the fields in the Runner struct would be public as well as the interfaces in this package. So the cli pkg is the only package that creates a session and the aws clients, then passes them down to the task.Runner struct and all of our other structs.
Does that make sense to you? What do you think?
There was a problem hiding this comment.
Yeah I can see the merit of it. That'd mean if I want to use Runner, I'd need to be aware of its dependencies. Is that correct? Would that result in too much complexity in the caller, e.g. say cli package (having to know all the dependencies of its dependencies)?
There was a problem hiding this comment.
Yeah good question, I agree that in this situation the CLI needs to ensure that all the dependencies are set that's why I am also torn lol
I think the right answer is probably copying what the standard library does. For example, the http.Server struct has lots of configuration similar to ours. However, if a user does not provide a handler than they set default ones (https://golang.org/src/net/http/server.go?#L2800) or panic. Maybe in our situation, we can check if they're nil and return an error 🤔
There was a problem hiding this comment.
Sounds good. The merit of returning an error over having a default behavior is that the behavior of, say Runner is more predictable right? Because the caller knows what the fields are set to, otherwise they would have received an error?
I have another question about having all fields public. Why would that be necessary in the case of dependency injection? Do we want to allow the caller to be able to later alter the fields after the instance is initiated?
There was a problem hiding this comment.
Because the caller knows what the fields are set to, otherwise they would have received an error?
I think the std lib panics if there is no good default behavior that can be set.
OK how about this, we don't do any validation within Run and assume that these fields need to be set. We can add a comment to make that explicit.
To guarantee that our code handles that, we can add an integ test to the cli pkg to makes sure the task run command works and initializes the dependencies properly.
I have another question about having all fields public. Why would that be necessary in the case of dependency injection?
I might be misunderstanding but how else could the cli package set the field for the task.DefaultVPCConfig struct if it's not public? I meant that they should be public so that the clients can set them (not to alter)
Co-authored-by: Efe Karakus <[email protected]>
| arns, err := r.Starter.RunTask(ecs.RunTaskInput{ | ||
| Cluster: cluster, | ||
| Count: r.Count, | ||
| Subnets: subnets, | ||
| TaskFamilyName: taskFamilyName(r.GroupName), | ||
| StartedBy: startedBy, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("run task %s: %w", r.GroupName, err) | ||
| } |
There was a problem hiding this comment.
If the task fails due to a missing NAT/Internet gateway, is there an error we can detect and provide the user with a better messaging so that they don't need to do all the research you did to figure out what was wrong?
There was a problem hiding this comment.
If the task fails to start due to a missing NAT/Internet gateway, it won't be able to pull the container image, which results in a ResourceNotReady error returned from the waiter (the doc says ResourceNotReady is returned when the waiter's max attempts have been exhausted, but from my observation it was also returned right after the container fails to pull image). Anyways, it is very generic, so i'm not sure if we could diagnose anything from the error. Maybe we could suggest the user to take a look at the console.
Louise15926
force-pushed
the
milestone-1-task
branch
from
7fe5237 to
16d8a40
Compare
This PR adds a task package that runs one-off tasks in different configurations, including running in the default cluster and subnets, running in a Copilot environment, and running in specified subnets and security groups.
Related #702
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.