-
Notifications
You must be signed in to change notification settings - Fork 227
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 volumes and mount points #13
Conversation
flah00
commented
Jan 21, 2016
- Define volumes the instance exports
- Define mount points the container imports
* Define volumes the instance exports * Define mount points the container imports
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
return volumes; | ||
} | ||
|
||
private Collection<Volume> getVolumeVolumes() { |
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.
Method name is a bit surprising, I suggest getVolumeSpec
or anything that make it clean what type of Volume we are talking about here
From a technical PoV, sounds to me from user point of view volume and mountpoint should be defined as a single "volume" configuration element, not as separated entities - even this is the cas on the API, but we need to consider UX here, not ECS API design. Also, I'm not convinced about the actual benefit. Persistent volumes aren't replicated by ECS around the cluster, so you can't rely on them to let build container run on arbitrary node. So you can't safely rely on this. IMHO A better approach to persistent data is to rely on a data volume container and use |
Technical critiqueI thought about limiting the input to a single entry, instead of following the API. It seemed like the best option was to follow the API, because if a user becomes confused, they can still reference ECS docs and make meaningful changes to their task template. If we stray from the API, the user will have to understand the ECS API and the way the plugin uses the API. I think that invites misunderstandings. But I don't feel super religious about it... So I'll push the change up. I'm happy to add help files, I tried adding them in Use caseWe need these options to share configuration files across workers. In our environment, we use chef to keep our configuration files updated. This includes some AWS credentials (because we use multiple AWS accounts), chef encryption keys, pem files, and berkshelf configuration flies. Generally speaking, I would never want to put these files into the image. But more importantly, if we did, we'd have to create new images each time we updated one of the configuration files. Also, putting these files into a data volume container would be cumbersome. We guarantee chef's presence, by including its installation and configuration in the |
Your use case is reasonable, but unfortunately require some additional setup for use with ECS. A docker-centric solution to inject secrets in containers would be better, and more portable. Anyway, I get your point, will maybe just add some warning in helper. |
@afejiang get development builds from https://jenkins.ci.cloudbees.com/job/plugins/job/amazon-ecs-plugin/ |