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

Adopt changes to Read and Diff #477

Merged
merged 3 commits into from
Jun 26, 2019
Merged

Adopt changes to Read and Diff #477

merged 3 commits into from
Jun 26, 2019

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Mar 7, 2019

  1. Return resource inputs as well as resource state from Read()
  2. Return a list of properties that changed from Diff()

We implement the former by reading the last applied configuration
from the kubectl.kubernetes.io/last-applied-configuration
annotation in the live object state. If this key is not present, no
inputs are populated and the old inputs are retained. These changes
also update the provider to set this field during Create and
Update.

We implement the latter by scanning the JSON diff and recording the
names of the top-level properties that changed. The engine uses this
information to filter diffs to only those that are semantically
meaningful.

These changes required a couple of bugfixes:

  • Old names are only adopted if the old resource was auto-named. This
    ensures that a name must be specified when importing a resource that
    was not autonamed.
  • URN to GVK conversion was fixed for resources in the "core" group.
    These resources have no group part in the GVK. Parsing was also
    simplified through the use of pulumi/pulumi's token manipulation
    functions.
  • When reading a resource, the GVK for the resource to read is now
    pulled from the URN if it is absent from the inputs.

@pgavlin
Copy link
Member Author

pgavlin commented Mar 7, 2019

This turns out to have some issues.

Because the set of inputs is pretty broadly overestimated, refreshing the inputs leads to spurious replaces due to apparent changes to auto-populated fields. Here is a simple example: https://asciinema.org/a/e700R9RYcK9pzevwLoXomS4CL

@pgavlin
Copy link
Member Author

pgavlin commented Jun 20, 2019

I believe that these changes are good to go.

There is a follow-up set of changes that I will make to switch the provider's Diff function over to use openapi.PatchForResourceUpdate to compute diffs. These changes will be necessary for a good import experience in the face of resources that are missing the last-applied-configuration annotation.

@pgavlin pgavlin force-pushed the pgavlin/import branch 2 times, most recently from 158d54e to a3f8c2a Compare June 24, 2019 19:25
1. Return resource inputs as well as resource state from Read()
2. Return a list of properties that changed from Diff()

We implement the former by reading the last applied configuration
from the `kubectl.kubernetes.io/last-applied-configuration`
annotation in the live object state. If this key is not present, no
inputs are populated and the old inputs are retained. These changes
also update the provider to set this field during `Create` and
`Update`.

We implement the latter by scanning the JSON diff and recording the
names of the top-level properties that changed. The engine uses this
information to filter diffs to only those that are semantically
meaningful.

These changes required a couple of bugfixes:
- Old names are only adopted if the old resource was auto-named. This
  ensures that a name must be specified when importing a resource that
  was not autonamed.
- URN to GVK conversion was fixed for resources in the "core" group.
  These resources have no group part in the GVK. Parsing was also
  simplified through the use of pulumi/pulumi's token manipulation
  functions.
- When reading a resource, the GVK for the resource to read is now
  pulled from the URN if it is absent from the inputs.
@pgavlin
Copy link
Member Author

pgavlin commented Jun 26, 2019

Ping.

@pgavlin
Copy link
Member Author

pgavlin commented Jun 26, 2019

I have verified that refreshing resources that were created with prior versions of @pulumi/kubernetes does not cause unexpected diffs despite the lack of the last applied configuration annotation. In these cases, the old inputs are retained as-is and the new behavior remains consistent with the old behavior.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Code changes LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@pgavlin pgavlin merged commit 52b44f3 into master Jun 26, 2019
@pulumi-bot pulumi-bot deleted the pgavlin/import branch June 26, 2019 23:26
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