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

Add optional name for tasks and docker containers #22

Closed
wants to merge 1 commit into from
Closed

Add optional name for tasks and docker containers #22

wants to merge 1 commit into from

Conversation

wheresalice
Copy link

I'm pretty new to both working with Go and working on this codebase, but this works for me as a way of optionally setting a container name as referenced in issue #4

Usage:

	task := command.Task("shaynativ/redis-ml")
	task.Name = "bob"

Any advice on better ways of doing it would be appreciated.

@wheresalice
Copy link
Author

wheresalice commented Feb 19, 2018

This doesn't cover the requirements in #4 but it gives me enough to do what I need to do for datali.

@lucymhdavies
Copy link

lucymhdavies commented Feb 19, 2018

This would work under some circumstances.

However if both of the Task's TaskFuncs are docker image names, you'd end up with both of them running with the same name. This isn't necessarily an issue, as both would not exist at the same time.

It would however mean that you cannot run two instances of your Task at the same time though, unless your app was very careful about ensuring that t.Name was unique.

I'll have a think.

@lucymhdavies
Copy link

Ah. I think I've figured out why this doesn't feel right.

It's the assumption of a 1-to-1 mapping of Task to Docker container, which is sorta an assumed thing, and basically how most Cali Commands would work, but technically you can have two Docker containers in your Task.
And perhaps it should be more like a Command can have multiple Tasks, rather than 1 Command = 1 Task = 2 TaskFuncs.

I need to think this through, and make some breaking changes to Cali at some point, when I figure out how I think this should work.

@wheresalice wheresalice closed this Apr 7, 2018
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