provider/docker: Add support for a list of pull_triggers within the docker_image resource. #10845

hmcgonig · 2016-12-19T20:40:43Z

This PR is in response to an issue I opened earlier today: #10843

Changes included are:

Modify docker_image.pull_trigger (string) to now be a list of strings called docker_image.pull_triggers.
Always pull images regardless of if they are found by searchLocalImages()
Fixed acceptance test which used pull_trigger to now use the pull_triggers list.
Updated relevant documentation

Why the PR?: When using the terraform provider to orchestrate a docker swarm cluster there are various times when you need to invoke a docker image pull outside of just the registry image sha256 changing. Take for example, adding a swarm node to the cluster. Once the new swarm node is up and accessible, an image pull must be triggered so that images are pulled onto the brand new node. To accommodate this I have changed pull_trigger into a list so that image pulls can be invoked by any number of things.

When testing this new functionality I found that images still were not being pulled onto the new host in the swarm. The reasoning behind this was because the provider was checking if an image was already found in the docker images list. When using docker swarm, 'docker images' will return an image if it is found in any node in the swarm, so in the case of a brand new swarm node, it will have no images and never be able to receive any images since the provider thinks the swarm already has the image. The best fix was to remove the check which sees if the image is already pulled and always attempt a pull operation. During this pull operation, nodes that already have the required docker image result in a no-op while nodes that need the image pull the image.

Example usage of pull_triggers attribute:

data "docker_registry_image" "elasticsearch" {
  name = "elasticsearch:latest"
}

resource "docker_image" "elasticsearch" {
  name = "${data.docker_registry_image.elasticsearch.name}"
  pull_triggers = ["${data.docker_registry_image.elasticsearch.sha256_digest}", "${var.swarm_cluster_size}"]
}

Thanks for taking a look, hopefully this is useful to others!

stack72 · 2017-01-03T11:50:59Z

Hi @hmcgonig

Thanks for the work here - unfortunately, this is a backwards incompatibility that will affect our users. The correct way to handle this would be to mark the old parameter as deprecated and then have it conflicting with the new way of working

"pull_trigger": &schema.Schema{
				Type:          schema.TypeString,
				Optional:      true,
				ConflictsWith: []string{"pull_triggers"},
				Deprecated:    "Use field pull_triggers instead",
			},

Then in the code itself, we would need to cater for both pull_trigger and pull_triggers as we need to be able to support both sets of parameters

Sound ok?

Thanks

Paul

stack72

Backwards incompatible - changes requested

hmcgonig · 2017-01-03T13:58:57Z

@stack72 Sure, that sounds great, I can make these changes sometime today. Thanks for the review.

stack72 · 2017-01-03T14:00:11Z

👍

hmcgonig · 2017-01-03T14:48:31Z

Hey @stack72, I just reverted the removal of pull_trigger, marked it as a deprecated field in the schema+docs, then squashed the commit. Is there anything else I should do?

stack72

Left some comments inline

stack72 · 2017-01-03T14:49:52Z

builtin/providers/docker/resource_docker_image_funcs.go

@@ -188,15 +188,11 @@ func findImage(d *schema.ResourceData, client *dc.Client) (*dc.APIImages, error)
 		return nil, fmt.Errorf("Empty image name is not allowed")
 	}

-	foundImage := searchLocalImages(data, imageName)


Why are we deleting the searchLocalImages check?

This was removed for two reasons, first because we do not need to care about if the image is found locally or not. A docker pull on an image that you already have just results in a no-op. Second, this is actually incorrect in the case of someone using a docker-swarm. If you search local images in the swarm, it will return any image on any host in the swarm. Because of this behavior, it was previously possible for terraform to try and start a container on a swarm host that did not have the image pulled, which is incorrect.

stack72 · 2017-01-03T14:50:18Z

builtin/providers/docker/resource_docker_image_funcs.go

-		if err := pullImage(&data, client, imageName); err != nil {
-			return nil, fmt.Errorf("Unable to pull image %s: %s", imageName, err)
-		}
+	if err := pullImage(&data, client, imageName); err != nil {


does this pullImage work for both pull_trigger and pull_triggers?

Yes, this functionality is correct in either case.

stack72 · 2017-01-03T14:50:48Z

builtin/providers/docker/resource_docker_image_test.go

@@ -131,6 +131,6 @@ data "docker_registry_image" "foobarbaz" {
 }
 resource "docker_image" "foobarbaz" {
 	name = "${data.docker_registry_image.foobarbaz.name}"


I think it would be useful to have 2 tests - 1 for pull_trigger and 1 for pull_triggers

I can do this, sure.

hmcgonig · 2017-01-03T15:25:57Z

@stack72 I left the existing "TestAccDockerImage_data" using the pull_triggers list since that is the new functionality going forward, but added in a test which uses pull_trigger which will need to be deleted when we remove that functionality.

…ocker_image resource.

stack72 · 2017-01-03T16:10:20Z

Hi @hmcgonig

This looks much better now :) A nice upgrade path for users - we can remove the deprecation for 0.9 :)

% make testacc TEST=./builtin/providers/docker                                                                                ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/01/03 16:06:41 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/docker -v  -timeout 120m
=== RUN   TestAccDockerRegistryImage_basic
--- PASS: TestAccDockerRegistryImage_basic (6.08s)
=== RUN   TestAccDockerRegistryImage_private
--- PASS: TestAccDockerRegistryImage_private (2.02s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDockerContainer_basic
--- PASS: TestAccDockerContainer_basic (20.17s)
=== RUN   TestAccDockerContainer_volume
--- PASS: TestAccDockerContainer_volume (21.19s)
=== RUN   TestAccDockerContainer_customized
--- PASS: TestAccDockerContainer_customized (31.77s)
=== RUN   TestAccDockerContainer_upload
--- PASS: TestAccDockerContainer_upload (18.97s)
=== RUN   TestAccDockerImage_basic
--- PASS: TestAccDockerImage_basic (4.05s)
=== RUN   TestAccDockerImage_private
--- PASS: TestAccDockerImage_private (2.16s)
=== RUN   TestAccDockerImage_destroy
--- PASS: TestAccDockerImage_destroy (2.41s)
=== RUN   TestAccDockerImage_data
--- PASS: TestAccDockerImage_data (8.50s)
=== RUN   TestAccDockerImage_data_pull_trigger
--- PASS: TestAccDockerImage_data_pull_trigger (8.13s)
=== RUN   TestAccDockerNetwork_basic
--- PASS: TestAccDockerNetwork_basic (0.43s)
=== RUN   TestAccDockerNetwork_internal
--- PASS: TestAccDockerNetwork_internal (1.03s)
=== RUN   TestAccDockerVolume_basic
--- PASS: TestAccDockerVolume_basic (0.23s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/docker	127.160s

Thanks for the work here

Paul

hmcgonig · 2017-01-03T16:15:17Z

Woo! Thanks for guiding me through my first official terraform PR! Looking forward to more in the future. 👍

stack72 · 2017-01-03T16:17:26Z

Please continue to submit them :)

ghost · 2020-04-18T02:08:25Z

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.

stack72 suggested changes Jan 3, 2017

View reviewed changes

stack72 added enhancement provider/docker waiting-response An issue/pull request is waiting for a response from the community labels Jan 3, 2017

hmcgonig force-pushed the master branch from 91f1e00 to b5f4880 Compare January 3, 2017 14:27

stack72 reviewed Jan 3, 2017

View reviewed changes

hmcgonig force-pushed the master branch 2 times, most recently from 1a1dcef to 0cdbb57 Compare January 3, 2017 15:20

provider/docker: Add support for a list of pull_triggers within the d…

c521d0f

…ocker_image resource.

hmcgonig force-pushed the master branch from 0cdbb57 to c521d0f Compare January 3, 2017 15:28

stack72 approved these changes Jan 3, 2017

View reviewed changes

stack72 merged commit 78e7d20 into hashicorp:master Jan 3, 2017

stack72 mentioned this pull request Jan 3, 2017

provider/docker: Add support for multiple docker_image pull_triggers #10843

Closed

ghost locked and limited conversation to collaborators Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/docker: Add support for a list of pull_triggers within the docker_image resource. #10845

provider/docker: Add support for a list of pull_triggers within the docker_image resource. #10845

hmcgonig commented Dec 19, 2016

stack72 commented Jan 3, 2017

stack72 left a comment

hmcgonig commented Jan 3, 2017

stack72 commented Jan 3, 2017

hmcgonig commented Jan 3, 2017

stack72 left a comment

stack72 Jan 3, 2017

hmcgonig Jan 3, 2017 •

edited

Loading

stack72 Jan 3, 2017

hmcgonig Jan 3, 2017

stack72 Jan 3, 2017

hmcgonig Jan 3, 2017

hmcgonig commented Jan 3, 2017

stack72 commented Jan 3, 2017

hmcgonig commented Jan 3, 2017

stack72 commented Jan 3, 2017

ghost commented Apr 18, 2020

provider/docker: Add support for a list of pull_triggers within the docker_image resource. #10845

provider/docker: Add support for a list of pull_triggers within the docker_image resource. #10845

Conversation

hmcgonig commented Dec 19, 2016

stack72 commented Jan 3, 2017

stack72 left a comment

Choose a reason for hiding this comment

hmcgonig commented Jan 3, 2017

stack72 commented Jan 3, 2017

hmcgonig commented Jan 3, 2017

stack72 left a comment

Choose a reason for hiding this comment

stack72 Jan 3, 2017

Choose a reason for hiding this comment

hmcgonig Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

stack72 Jan 3, 2017

Choose a reason for hiding this comment

hmcgonig Jan 3, 2017

Choose a reason for hiding this comment

stack72 Jan 3, 2017

Choose a reason for hiding this comment

hmcgonig Jan 3, 2017

Choose a reason for hiding this comment

hmcgonig commented Jan 3, 2017

stack72 commented Jan 3, 2017

hmcgonig commented Jan 3, 2017

stack72 commented Jan 3, 2017

ghost commented Apr 18, 2020

hmcgonig Jan 3, 2017 •

edited

Loading