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

Track default population in inputs #349

Merged
merged 6 commits into from
Mar 12, 2019
Merged

Track default population in inputs #349

merged 6 commits into from
Mar 12, 2019

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Mar 9, 2019

Add a reserved property to each map value present in an input bag that
tracks which keys in the map were populated using default values. When
performing default application, only use a value from an old input bag
if it was itself a default value.

This allows the following sequence of events:

  • Create a resource with a value explicitly specified for property P
  • Remove the value for property P and update the resource. The new value
    for property P will be whatever its default value is, if any.

Fixes pulumi/pulumi-aws#439.

Add a reserved property to each map value present in an input bag that
tracks which keys in the map were populated using default values. When
performing default application, only use a value from an old input bag
if it was itself a default value.

This allows the following sequence of events:
- Create a resource with a value explicitly specified for property P
- Remove the value for property P and update the resource. The new value
  for property P will be whatever its default value is, if any.

Fixes pulumi/pulumi-aws#439.
@@ -843,7 +883,42 @@ func CoerceTerraformString(schType schema.ValueType, stringValue string) (interf
return stringValue, nil
}

func extractInputsFromOutputs(urn resource.URN, outs resource.PropertyMap,
func propagateDefaultAnnotations(oldInput, newInput resource.PropertyValue) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is latent, and will become active once pulumi/pulumi#2531 and an as-of-yet-unpublished PR land.

In essence: once we begin refreshing inputs, doing so will destroy the __defaults annotations this PR adds. In order to avoid that, we need to hand Read the current inputs as well as the current state. We can then propagate the __defaults annotations for properties whose values have not changed.

pgavlin added a commit to pulumi/pulumi that referenced this pull request Mar 11, 2019
These changes expose a resource's old inputs to the provider when
performing a refresh. The provider can take advantage of this to
maintain the accuracy of any additional data or metadata in the
resource's inputs that may need to be updated during the refresh.

This is required for the complete implementation of
pulumi/pulumi-terraform#349. Without access to
the old inputs for a resource, TF-based providers would lose all
information about default population during a refresh.
pgavlin added a commit to pulumi/pulumi that referenced this pull request Mar 11, 2019
These changes expose a resource's old inputs to the provider when
performing a refresh. The provider can take advantage of this to
maintain the accuracy of any additional data or metadata in the
resource's inputs that may need to be updated during the refresh.

This is required for the complete implementation of
pulumi/pulumi-terraform#349. Without access to
the old inputs for a resource, TF-based providers would lose all
information about default population during a refresh.
pgavlin added a commit to pulumi/pulumi that referenced this pull request Mar 11, 2019
These changes take advantage of the newly-added support for returning
inputs from Read to update a resource's inputs as part of a refresh.
As a consequence, the Pulumi engine will now properly detect drift
between the actual state of a resource and the desired state described
by the program and generate appropriate update or replace steps.

As part of these changes, a resource's old inputs are now passed to the
provider when performing a refresh. The provider can take advantage of
this to maintain the accuracy of any additional data or metadata in the
resource's inputs that may need to be updated during the refresh.

This is required for the complete implementation of
pulumi/pulumi-terraform#349. Without access to
the old inputs for a resource, TF-based providers would lose all
information about default population during a refresh.
this makes it possible to retain default lists during refresh
@pgavlin
Copy link
Member Author

pgavlin commented Mar 11, 2019

Ping.

@pgavlin
Copy link
Member Author

pgavlin commented Mar 11, 2019

Code-gen tests will fail here b/c of the update to pulumi/pulumi. These changes do not affect codegen.

Copy link
Contributor

@swgillespie swgillespie left a comment

Choose a reason for hiding this comment

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

Took me a little bit to wrap my head around __defaults, but I get it now and LGTM. Only thing I'd suggest is an additional doc comment on __defaults, something like this:

"__defaults is the set of input property keys whose values were not present in the original user input but instead were derived from provider defaults. A key's absence in __defaults implies that the old state's value for that key should not be used when applying defaults, since the old state's value was itself not a default."

pkg/tfbridge/provider.go Outdated Show resolved Hide resolved
Co-Authored-By: pgavlin <pgavlin@gmail.com>
@pgavlin
Copy link
Member Author

pgavlin commented Mar 11, 2019

A demonstration of the effects of this change may prove useful. Below is a before/after of a stack that creates a versioned S3 bucket and then attempts to disable versioning. In the before case, pulumi-terraform picks up the old value of true and reuses it when the property is removed, so no diff is present and no update occurs. In the after case, pulumi-terraform is able to detect that the old value for enabled was not a default and should not be used as one, so a diff is present and the update occurs.

Before: https://asciinema.org/a/Drf7Mx4QLRd4fIZeL6UjbgKLY
After: https://asciinema.org/a/VbJSbHU3775nSoaxwqRnYSyGq

@pgavlin pgavlin merged commit b9d202e into master Mar 12, 2019
@pulumi-bot pulumi-bot deleted the pgavlin/defaults branch March 12, 2019 01:32
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.

None yet

2 participants