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.network resource #477

Merged
merged 14 commits into from Nov 9, 2016

Conversation

Projects
None yet
2 participants
@ryane
Copy link
Contributor

ryane commented Nov 4, 2016

fixes #298

@ryane ryane added the enhancement label Nov 4, 2016

@ryane ryane added this to the 0.4.0 milestone Nov 4, 2016

@ryane ryane force-pushed the feature/docker.network branch from 2af7f77 to 2a769da Nov 7, 2016

@arichardet
Copy link
Contributor

arichardet left a comment

Mostly nitpicks on adjusting comments and could change "present"/"absent" to the StatePresent/StateAbsent

@@ -66,6 +66,12 @@ type Preparer struct {
// list of DNS servers for the container to use
DNS []string `hcl:"dns"`

// the mode of the container network

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

should you indicate the default if not specified?


// Prepare a docker network
func (p *Preparer) Prepare(ctx context.Context, render resource.Renderer) (resource.Task, error) {
if p.Name == "" {

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

This is handled for you by the preparer with required

This comment has been minimized.

@ryane

ryane Nov 8, 2016

Contributor

I think this is needed to prevent someone from manually specifying name = "" in the hcl

This comment has been minimized.

@arichardet

arichardet Nov 9, 2016

Contributor

yes, that is correct

// driver specific options
Options map[string]interface{} `hcl:"options"`

// ip address management driver

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

maybe want to indicate the default in comments

// enable ipv6 networking
IPv6 bool `hcl:"ipv6"`

// indicates whether the volume should exist.

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

add that "present" is default

// ip address management driver
IPAMDriver string `hcl:"ipam_driver"`

// custom IPAM configuration. multiple IPAM configurations are permitted. Each

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

What if no configurations are specified? Probably good to add something to the comments.

DefaultIPAMDriver = "default"
)

// Network is responsible for managing docker volumes

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

managing docker networks

type IPAMConfigs []dc.IPAMConfig

// Len implements the sort interface for IPAMConfigs
func (ic IPAMConfigs) Len() int { return len(ic) }

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

It looks like Len, Swap, Less, and String (and ipamConfigString) are not used

This comment has been minimized.

@ryane

ryane Nov 8, 2016

Contributor

these are used to implement the sort interface: https://golang.org/pkg/sort/

This comment has been minimized.

@arichardet

arichardet Nov 9, 2016

Contributor

I see


// TestPreparerPrepare tests the Prepare function
func TestPreparerPrepare(t *testing.T) {
t.Run("name is required", func(t *testing.T) {

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

can remove this test with change in network/preparer.go (handled in resource/preparer.go)

This comment has been minimized.

@ryane

This comment has been minimized.

@arichardet

arichardet Nov 9, 2016

Contributor

yes, this is ok

}

if p.State == "" {
p.State = "present"

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

could use StatePresent instead of "present"

require.NoError(t, err)
assert.True(t, status.HasChanges())
assert.Equal(t, 1, len(status.Diffs()))
comparison.AssertDiff(t, status.Diffs(), nwName, "present", "absent")

This comment has been minimized.

@arichardet

arichardet Nov 7, 2016

Contributor

could use StatePresent and StateAbsent throughout

This comment has been minimized.

@ryane

ryane Nov 8, 2016

Contributor

true but it requires a cast (otherwise, I get cannot use network.StatePresent (type network.State) as type string in argument to comparison.AssertDiff). makes the lines really long and harder to read (imo). worth it?

This comment has been minimized.

@ryane

ryane Nov 9, 2016

Contributor

actually wasn't so bad - went ahead and fixed these

ryane added some commits Nov 3, 2016

docker.network: core implementation
missing some options

@ryane ryane force-pushed the feature/docker.network branch from ac8ff25 to fe6a2a9 Nov 8, 2016

@arichardet

This comment has been minimized.

Copy link
Contributor

arichardet commented Nov 9, 2016

LGTM!

@arichardet arichardet merged commit b904201 into master Nov 9, 2016

2 checks passed

codeclimate Code Climate found 19 fixed issues.
Details
wercker/build Wercker pipeline passed
Details

@arichardet arichardet deleted the feature/docker.network branch Nov 9, 2016

BrianHicks pushed a commit that referenced this pull request Dec 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment