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

Docker provider #983

Closed
wants to merge 1 commit into from
Closed

Docker provider #983

wants to merge 1 commit into from

Conversation

jefferai
Copy link
Member

Please read this merge request message carefully; it is not expected that this be merged. This is being created for initial feedback.

Initial commit. This adds the initial bits of a Docker provider.
Docker's API is huge and only a small subset is currently implemented,
but this is expected to grow over time. This is enough to satisfy the
use cases of probably 95% of Docker users.

A few things are not yet tested, and are clearly marked with TODO. These
are very high on the priority list.

I'm preparing this initial pull request as a preview step for feedback.
My ideal scenario would be to develop this within a branch in the main
repository; the more eyes and testing and pitching in on the code, the
better (this would avoid a merge request-to-the-merge-request scenario,
as I figure this will be built up over the longer term, even before
a merge into master).

Unit tests do not exist yet. I really need
#950 and
#952 fixed before I can
properly instrument Docker at all, let alone trust the results of unit
tests. Working around this to the best of my abilities is also a reason
that each resource setting has a computed equivalent.

I also added a "make quickdev" option to skip the fetching step, which
speeds up builds significantly when you've just updated all of the
dependencies.

This code is copyright 2014 Akamai Technologies, Inc. opensource@akamai.com

@mitchellh
Copy link
Contributor

Note: we're doing a bunch of merges for 0.3.7 right now and we won't get to this until 0.4.0 (which will start this week).

@jefferai
Copy link
Member Author

@mitchellh Not a problem. While I'm fixing up the TODOs and bringing in the fixes you've been working on I've been rebasing. When you guys start taking a look I'll switch to merging if you wish.

@jefferai jefferai force-pushed the master branch 3 times, most recently from e9dc0f7 to eb371e1 Compare February 18, 2015 16:33
@jefferai jefferai force-pushed the master branch 5 times, most recently from c8dc21b to edb77af Compare March 5, 2015 17:51
"image": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment says "ForceNew is not true" but it is set. Which one is correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Will take it out.

@jefferai jefferai force-pushed the master branch 4 times, most recently from 62f736c to fb1fa73 Compare March 6, 2015 19:49
Docker's API is huge and only a small subset is currently implemented,
but this is expected to grow over time. Currently it's enough to
satisfy the use cases of probably 95% of Docker users.

I'm preparing this initial pull request as a preview step for feedback.
My ideal scenario would be to develop this within a branch in the main
repository; the more eyes and testing and pitching in on the code, the
better (this would avoid a merge request-to-the-merge-request scenario,
as I figure this will be built up over the longer term, even before
a merge into master).

Unit tests do not exist yet. Right now I've just been focused on getting
initial functionality ported over. I've been testing each option
extensively via the Docker inspect capabilities.

This code (C)2014-2015 Akamai Technologies, Inc. <opensource@akamai.com>
@phinze
Copy link
Contributor

phinze commented Mar 13, 2015

@jefferai seems like this might be nearly ready to merge, yeah? Just need to get some acceptance tests in there. 👍

@mitchellh
Copy link
Contributor

Starting to review and get this going for 0.4

@@ -0,0 +1,24 @@
package docker

import dc "github.com/fsouza/go-dockerclient"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick I usually just do multiline imports from the get-go cause there is usually some future that requires more and it makes it easier to add.

client, err := config.NewClient()
if err != nil {
return fmt.Errorf("Unable to connect to Docker: %s", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason the client can't be initialized in the provider configuration function?

@mitchellh
Copy link
Contributor

@jefferai This looks good, I can add the tests if you're averse to it. Is this ready to go? I noticed you haven't comitted in awhile but I think in IRC you mentioned you were doing more work on it.

@mitchellh
Copy link
Contributor

Closing for #1329 which is just your commit + some test commits.

@mitchellh mitchellh closed this Mar 27, 2015
@ghost
Copy link

ghost commented May 3, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants