When executing a `--target`ed operation, Pulumi considers all providers
to be "implicitly targeted", whether their URNs are mentioned or not. As
a result, *all* Pulumi operations update provider resources,
`--target`ed or not. This can lead to some tricky scenarios that Pulumi
needs to handle or detect in order not to break the user's program or
state.
One such case is where a user tries to alter the provider of an
untargeted resource. Consider the following starting program/state:
```typescript
const parent = new sdk.Component("parent")
const provider = new sdk.Provider("provider")
const resource = new sdk.Resource("resource", { provider })
```
Let us modify the program so that `provider` becomes a child of
`parent`. In doing so, `provider`'s URN will be changed on the next
update to include `parent`'s type (so e.g. `provider` will become
something akin to `parent$provider`):
```typescript
const parent = new sdk.Component("parent")
const provider = new sdk.Provider("provider", { parent })
const resource = new sdk.Resource("resource", { provider })
```
Now we run `pulumi up --target "**non-existent**"`. That is, we specify
a target glob that will not target *any* of the resources in the
program. Our update will proceed as follows:
* A resource registration for `parent` is sent. `parent` is not targeted
(it is not matched by the `--target` glob, and it is not a provider
resource), so we emit a `SameStep` for it to copy it unchanged from the
old state to the new state.
* A resource registration for `provider` is sent, with a parent of
`parent`. This resource *is* targeted since it is a provider, and thus
is implicitly targeted. Pulumi constructs the URN `parent$provider` for
the provider resource and looks to see if there is any old state for it.
* Pulumi finds no old state (the existing provider state is registered
under the URN `provider` [no `parent$`]) and so decides that `provider`
must be `Create`d. It emits a `CreateStep` and moves on.
* A resource registration for `resource` is sent. `resource` is not
targeted (it is not matched by the `--target` glob, and it is not a
provider resource), so we want to emit a `SameStep` for it to copy it
unchanged from the old state to the new state.
* *However*, as part of checking whether we can emit a `SameStep` for
resource, we find that it references a provider that we have not seen a
registration for (`provider` -- recall that we have only seen
`parent$provider`). We thus abort with an error informing the user that
they have to run an untargeted update to update their provider before
targeted updates can proceed.
If the user genuinely wants to rename the provider, and not actually
delete the old one and create a new one whose parent happens to be
`parent`, they can of course use an alias:
```typescript
const parent = new sdk.Component("parent")
const provider = new sdk.Provider(
"provider",
{
parent,
aliases: [new Alias({ parent: undefined })],
}
)
const resource = new sdk.Resource("resource", { provider })
```
At least, that's what they might expect. Presently, attempting to do
this will yield a `panic`, neither tripping the existing error
machinery, nor successfully completing the `--target`ed operation.
This commit fixes this panic by making it work. We'd previously
considered making this case trigger the existing "no provider exists"
error flow, but this is not in fact correct -- in the case an alias is
supplied, the provider *does* exist and we should be able to find it and
complete the rename. Here we make that so.
Fixes#17719
When using `--target` to target specific resources during an update, we
use the list of targets to decide which steps to generate given a set of
resource registrations. Specifically:
* If the registration event names a resource that is targeted, we
process it as usual.
* If the registration event names a resource that _is not_ targeted, we
emit a `SameStep` for it.
In the latter case, the emission of a `SameStep` means that the old
state for the resource will be copied across to the new state. This is
the desired behaviour -- the resource was not targeted and so the new
state should contain the resource exactly as it was prior to the update.
In #16247, this behaviour was extended so that any *dependencies* of a
resource being copied were *also* `SameStep`ped, to avoid copying over
states referring to resources no longer registered by the program (and
thus not already implicitly in the new state -- see #16247 for a
concrete example).
Manually traversing dependencies in this manner introduces a new bug
however -- by walking "back up" the state as step generation is
proceeding, we are able to observe certain intermediate states that we
would not be able to ordinarily. One such intermediate state is that of
resources which are marked for deletion as part of a
create-before-replace operation. Consider the following starting
program/state:
```typescript
const p = new Resource("parent")
const a = new Resource("a", { parent: p })
const b = new Resource("b", { dependency: a })
```
We have three resources in our state: `p`, `a` (whose URN will be
something like `p$a` since it has `p` as its parent and will thus embed
`p`'s type in its URN), and `b`. We change the program as follows:
```typescript
const p = new Resource("parent")
const a = new Resource("a", { aliases: [new Alias({ parent: p })] })
const b = new Resource("b", { dependency: a })
```
We have modified `a` to no longer have `p` as a parent and added an
alias to the URN it would have had when `p` *was* its parent. With this
change made, we run a *targeted* update that *only targets `a`*.
Moreover, as part of this update, the following will occur:
* `a` will be diffed and found to be needing *replacement*;
* The replacement will be a "create-before-replace", meaning that a new
A will be created *before* the old A is deleted;
* As part of this operation, the creation of the new A will succeed,
while the deletion of the old A will fail.
As a result, the operation unfolds thus:
* A resource registration is received for `p`. `p` is not targeted, so a
`SameStep` is emitted for it.
* A resource registration is received for `a`, with no parent and an
alias. The alias means that the old `a` with URN of the form `p$a` will
be found and become `a`'s `old` during step generation.
* `a` is targeted, so we diff it and find we must perform a
create-before-replace.
* A new `a`, with the parentless URN, is created successfully.
* The old `a`, with the parented (`p$a`) URN (since the alias enabled us
to find this), is marked for deletion -- its `Delete` field is set to
`true`. This permits Pulumi to retry deletions of replaced resources
should they fail.
* Indeed, this deletion fails, so the old `a` (with `Delete: true`) will
be left in the state to be cleaned up in a subsequent operation.
* A resource registration is received for `b`. `b` is not targeted, so a
`SameStep` is emitted for it. Prior to this however, `b`'s dependencies
are traversed so that `SameStep`s can be emitted for them as part of
#16247.
* `b`'s dependencies contain a reference to *`a`'s old URN* (of the form
`p$a`).
* Ordinarily, this URN would be *normalized* (rewritten to the new URN)
as part of snapshot persistence -- this happens to all resources,
targeted or not, if aliased resources are targeted as part of an
operation.
* In this case however, we are manually traversing dependencies prior to
snapshot persistence and so can observe the old resource (which is now
marked for deletion with `Delete: true`).
* We emit a `SameStep` for the old `a`, which causes a panic since
`SameStep`s are not supposed to be constructed for `Delete`d resources.
Despite the wordy explanation, the fix is (it seems) straightforward --
just as we skip `Delete`d resources when building the list of `old`s
that will take part in step generation, we skip them when we encounter
them as we manually traverse dependencies. The snapshot persistence
layer then takes care of them later on, copying them over. Note that
they are copied over *after* the resources that supposedly depend on
them, but due to URN normalization, those resources will be rewritten to
have their dependencies point to the new, renamed resources, which
*will* appear *before* them as is necessary.
Fixes#17713
It's often the case that we want to move or rename resources in our
Pulumi programs. Doing so can result in a change in the resource's URN,
which means that Pulumi will, by default, see the move as a deletion of
the old resource and a creation of the new resource. We can tell Pulumi
that a resource has been renamed by using *aliases*, whereby a resource
can be annotated with its previous URNs. Pulumi will then use these URNs
in several places:
* When looking for a resource's previous state, Pulumi will try to find
it using the new URN first and any aliases second.
* When writing out a new snapshot, Pulumi will *normalize* all URNs
(e.g. those in provider, parent and dependency references) to remove
aliases and replace them with the new URNs they resolve to.
Alas, a familiar story presents itself in the second case -- Pulumi does
not currently normalize `DeletedWith` references. This can result in
snapshot integrity errors as Pulumi leaves stale references in the
snapshot before writing it. This commit addresses this omission, using
the now-preferred `GetAllDependencies` method introduced in #17320 to
hopefully stop this from happening again in this part of the codebase.
Fixes#17614
`Create`s happen in response to new resources being present in a Pulumi
program but not in Pulumi's state snapshot. For instance, take the
following TypeScript program:
```typescript
const b = new Resource("b", { x: 2 })
const c = new Resource("c", { x: 3 }, { deletedWith: b })
```
When asked to run this program on an empty state with no arguments,
Pulumi will issue two `Create` calls -- one for `b`, and one for `c`.
The call for `c` will *depend on* `b`'s due to the need for
`deletedWith` to refer to `b`'s URN.
If instead of passing no arguments we ask Pulumi to perform a *targeted*
operation that *only targets `c`*, Pulumi will throw an error and refuse
to execute the program. This is because `c`'s `Create` would depend on
`b`'s `Create`, but since `b` has not been targeted, its `Create` will
not happen and thus `c`'s call cannot be built. Internally, we call
`b`'s omission a "skipped create", and keep track of these so that we
can trigger the error above appropriately.
So far, so good. Now, consider that we have executed the above program
with no targets, so that Pulumi's state contains both `b` and `c`, with
`c` depending on `b` via a `deletedWith` link. We now add the missing
`a` to the program and modify `b` to depend on it:
```typescript
const a = new Resource("a", { x: 1 })
const b = new Resource("b", { x: 2 }, { deletedWith: a })
const c = new Resource("c", { x: 3 }, { deletedWith: b })
```
If we were to run the program with no targets, we would expect a state
in which all of `a`, `b` and `c` existed. `c` would depend on `b` (as it
did before) and `b` would have a new dependency on `a`. Lovely.
Of course, we are not about to run the program with no targets. We are
instead interested in *targeting `b` only*. When we do, the following
sequence of events will unfold:
* `a`, not existing in the state already and not being targeted, will
yield a skipped create.
* `b` depends on a skipped create. However, its *inputs* have not
changed (`x` was `2` before, and it's still `2` after). Consequently,
`b` will have yielded a `SameStep`. We *incorrectly* assume this means
that skipped creates are not a problem (`SameStep`s mean no changes,
right?) and allow the deployment to proceed.
* We write the new `b` to the state, with a dependency on the skipped
create, yielding a snapshot integrity error.
* 💥
We might ask "why assume that `SameStep`s are safe?". From looking at
the existing test cases, it seems likely that this was designed to cover
the case where a skipped create is depended on *by another skipped
create* (which is internally represented as a `SameStep`) -- in such
cases, we don't need to error and the deployment can proceed. However,
this bug shows that there are cases where `SameStep` does not imply
skipped create. This commit thus tightens up the logic appropriately,
checking explicitly for `SameStep`s whose `IsSkippedCreate` method
returns true.
When performing a `--target`ed operation, it is possible that the new
program introduces new resources that are not targeted. In such cases,
we can't emit `CreateStep`s (since that would not respect the
`--target`s), but we need to track the creations so that e.g. later
resources that will come to depend on these would-be resources can be
handled appropriately. The step generator maintains a list of so-called
"skipped creates" to this end.
Unfortunately, when later traversing the list of skipped creates to see
if any resources depend on them, "depend on" does not take into account
property dependencies and deleted-with relationships. This results in
snapshot integrity errors as we end up writing the broken dependencies
to the state. This commit rewrites the code to use the new
`GetAllDependencies()` method on `resource.State` to take account of the
other possible relationships, adding lifecycle tests for these new cases
and cleaning up the existing tests to be more specific about the errors
they expect.
When using a `--non-interactive` renderer, it seems that we don't
respect the presence or absence of options like `--show-sames`, instead
displaying all rows. While we do correctly compute whether or not a row
should be hidden, we don't check that computed boolean later on unless
we are performing an interactive render. This commit fixes that.
Fixes#12162
This fixes a snapshot integrity issue with delete before replace, failed
creates, and multiple updates.
This was caused by https://github.com/pulumi/pulumi/pull/16510 where we
started removing pending replace resources as part of their replacement
steps. A well intentioned fix to stop trying to delete resources we'd
already deleted in a previous update.
The bug would manifest if the following happened:
1. An update goes to replace a resource with `deleteBeforeCreate` set.
It deletes the old resource (and saves the state as pending replace) but
then failed to create the new replacement resource.
2. A second update is run to try and create the replacement resource. At
this point the bug manifest and we delete the pending replace resource
from state, but then if the new resource fails to create again we end up
with an invalid snapshot.
The fix is very simple. If a resource is already pending replacement we
just don't issue a delete/remove step at all. The pending replace
resource will get cleaned up at the end by the create step creating the
new version.
Fixes#17111Fixes#17073Fixes#17070Fixes#17069Fixes#16916
When using `--target` to target specific resources during an update, we
use the list of targets to decide which steps to generate given a set of
resource registrations. Specifically, *prior to #16247*:
* If a registration event named a resource that was targeted, we would
process it as usual.
* If a registration event named a resource that was not targeted, we
would emit a SameStep for it.
In #16247, this behaviour was changed so that we'd consider the entire
dependency graph of an untargeted resource, in order to avoid missing
copying resources that had been removed from the program but not
targeted (and thus whose old states would be copied over later on,
potentially *after* the states of resources depending on them). As part
of this work, parent-child relationships were not handled. This did not
manifest in errors in the cases tested, but in the presence of aliases
and other edge cases, parent-child relationships *can* result in
snapshot integrity issues and thus must be handled too. This commit
implements those changes.
Fixes#17081 (I believe)
Provider internal state is now separated from provider user config. This
allows providers to use a config key like "pluginDownloadURL" which
previously would have conflicted with the engines internal state. It
also allows the engine to add new internal state fields without concern
for clashing with existing or future provider config keys.
"version" is maintained as a root level key because providers already
look at this field, and we use it in conformance tests. However it's
been fixed to be the package version, not the plugin version. That's the
same for normal providers, but for a parameterised provider it will be
the version of the parameterised package, not the base plugin version.
As part of this I've made schema more strict that a provider can't
define its own version field. This would have clashed with the use of
version to set the plugin version to download, and is better set by
users by using the version resource option.
Fixes https://github.com/pulumi/pulumi/issues/16757.
---------
Co-authored-by: Will Jones <will@sacharissa.co.uk>
source_eval wasn't looking at package refs for explicit provider
resources, only when building default providers. This meant that
explicit provider resources for parameterized packages would fail to
construct as they wouldn't be able to find their plugin.
This fixes source_eval to check for PackageRef and set all the required
fields from the provider request it maps to.
In `pulumi up --continue-on-error`, we have to make sure to not delete
resources that are still needed in the snapshot because an earlier step
failed. E.g. if B depends on A, and A fails to be deleted, we need to
make sure B is not deleted as well, otherwise we get a snapshot
integrity failure because B is missing A.
We do this by checking the dependencies of B in the *current program*,
e.g. only checking `dep.Res()`. However it is possible that the
dependencies in the new program are not the same.
For example, say resource A depends on provider B. Now we change the
program and resource A depends on provider C. We try to `pulumi up
--continue-on-error` the latter program, but the Update of resource A
fails. But because the new resource A only depends on C, and not on B,
we go ahead and delete B, resulting in a snapshot integrity failure,
because A has not been updated.
Fix this by also checking the `res.Old()` for dependencies, and not
deleting those either if `res` had a failure.
Note that this isn't an issue for creates/updates, because we only need
to check wheter a *new* resource is dependent on the existing resource.
Fixes https://github.com/pulumi/pulumi/issues/16720
There are a number of use cases for `Delete` steps in a Pulumi
operation. Aside from direct deletions, where a resource has been
removed from a program, they are also a key component of _replacements_,
whereby a resource has changed in a manner where it cannot simply be
updated in the provider but instead must be completely removed and
reconstructed. In such cases, Pulumi offers two options:
* "Delete after replace", in which a new resource is first created
before the old one is deleted.
* "Delete before replace", in which the old resource is first deleted
before a replacement is created.
Delete-after-replace is the default, since where possible it is
typically the preferred option by users, enabling e.g. zero-downtime
deployments. However, there are cases where delete-after-replace is not
possible (e.g. because the new and old resources would clash in some
manner), and so delete-before-replace is an option that can be opted
into.
In cases where the deletion must happen first, we must be careful how we
handle the Pulumi state. Either or both of the delete and create calls
could fail, and we always want a state file that tells us how to resume
a failed call to yield the desired outcome. In the case of
delete-before-replace operations, a key component of resumable state is
the `PendingReplacement` field on a resource. `PendingReplacement`
indicates that a resource has been deleted in the provider, but that
this deletion is part of a replacement (and thus that a create call will
subsequently occur). In this way, the deleted resource can remain in the
state file throughout the operation, meaning that e.g. resources that
depend on the deleted resource won't have their dependencies violated
(causing a snapshot integrity error).
Alas, until this point, `PendingReplacement` was set unconditionally on
the creation of a delete-before-replace step, meaning that if the
provider delete failed, we'd elide the delete on a retry and end up with
a bad state failing snapshot integrity checks. This commit fixes this by
moving the setting of `PendingReplacement` inside `DeleteStep.Apply`, so
that it occurs only if the provider delete call succeeds. It also adds a
lifecycle test to test this case and hopefully guard against
regressions.
Fixes#16597
Part of #16667
The test now checks that destroy works, and also that the parameterized
provider state is what we expect.
This required flipping around some of the state managment around reading
and writing provider inputs.
We've recently introduced resource transforms, which allow users to
update any resource options and properties for the duration of a program
using a callback. We want to introduce similar functionality for Invokes
(and eventually also StreamInvokes, Read and Calls). This can help users
e.g. set default providers through transforms consistently for all
components.
While this PR only implements the engine parts of invoke transforms, the
API for this will look very similar to what the API for resource
transforms looks like. For example in TypeScript:
```
pulumi.runtime.registerInvokeTransform(args => {
[...]
});
```
---------
Co-authored-by: Will Jones <will@sacharissa.co.uk>
This is a very similar fix to
https://github.com/pulumi/pulumi/pull/16371, but for imports instead of
create and updates.
When using the import resource option with continue-on-error, and there
is a diff in the import, the import fais, but we still also return the
completion function from the `Apply` call. This results in the engine
trying to call `Done` twice, which in turn results in it running
indefinitely, trying to write the result to a channel that's no longer
being read from.
Fix this by not returning the completion function from `Apply` for the
ImportStep when there is an error.
(I went through the other steps as well to double check we don't need a
similar fix there, and it looks like they are all fine).
Fixes: https://github.com/pulumi/pulumi/issues/16570
External resources are resources whose state is tracked but not managed.
That is, they feature in the state file, but Pulumi does not issue
create, read or update operations for them. Typically they arise as the
result of generated `.get` functions and methods in Pulumi programs
(these should not be confused with specific `getX` functions/methods,
which are backed by `Invoke`s and designed to reflect data
sources/arbitrary function calls).
In #16146, `refresh` operations were modified to use actual bonafide
`Diff` calls to compute differences in a manner that would reflect "what
will happen on the next `preview`". In general, this aligns better with
what users expect when running `refresh`, and fixes a number of issues
that have long plagued `refresh`. However, it falls down in the case of
external resources. The existing `Diff` contract takes (somewhat
strangely) _three_ key inputs:
* The old inputs ("what the program used to say")
* The old outputs ("what we got back from the last update")
* The new inputs ("what the program says now")
Intuitively, `Diff`'s job is to return the difference between the old
and new inputs. Aside from old outputs being a bit of a third wheel,
this contract makes sense for an `update` operation, where the inputs
are driving the outputs and changes there will yield changes elsewhere.
In a `refresh` operation, however, we have a different situation -- one
in which _both_ old and new inputs _and_ outputs are available. Alas, we
don't currently have a "four-argument `Diff`" (or a two-argument one we
can call twice with no fear of repercussions), and so the usage in
`refresh` is a little bit of a clever hack that ends up biasing inputs
over outputs (see #16146 or the implementation for details).
For external resources, the old behaviour (where we just compared
outputs) seems like a better fit. This commit therefore adds this
special case and a test to ensure that when we see external resources,
we simply compare outputs and don't call `Diff`.
Fixes#16401Fixes#16502
---------
Co-authored-by: Thomas Gummerer <t.gummerer@gmail.com>
When the `RetainOnDelete` resource option is set, the console should
display `[retain]` next to resources which are being deleted from the
Pulumi program to indicate that, while the Pulumi state for those
resources will be deleted, the provider will not be instructed to
actually `Delete` the resource (effectively leaving the resource
unmanaged).
Currently, Pulumi decides whether or not to add the `[retain]` text by
inspecting resource state attached to a display step. However, there are
two places in which a display step is constructed:
* As part of normal/"in-memory" execution, whereby resource state is
inherently available and attached to the display step.
* As part of deserializing an existing JSON event stream (e.g. from a
Pulumi REST API call), whereby resource state is _not_ available and so
absent (`nil`) from the display step. This is also the codepath taken by
display tests.
The absence of the state in the latter codepath means that `[retain]` is
not displayed. This needn't be the case however. The `RetainOnDelete`
option is explicitly persisted in our JSON events and copied on to the
display step, so we can just use that instead. This commit makes this
change. Additionally, the single test we have for `RetainOnDelete` is
pulled out into its own file and display testing is now enabled for that
test.
As well as indicating that a resource's state has changes, a diff can
also indicate that those changes require the _replacement_ of the
resource, meaning that it must be recreated and not just updated. In
this scenario, there are two possible ways to replace the resource -- by
first creating another new resource before deleting the old one
("create-before-replace"), or by first deleting the old resource before
creating its replacement ("delete-before-replace").
Create-before-replace is the default since generally, if possible to
implement, it should result in fewer instances of "downtime", where a
desired resource does not exist in the system.
Should delete-before-replace be chosen, Pulumi implements this under the
hood as three steps: delete for replacement, replace, and create
replacement. To track things consistently, as well as enable resumption
of an interrupted operation, Pulumi writes a flag, `PendingReplacement`
to the state of a deleted resource that will later be cleaned up by a
completed replacement.
Should an interrupted operation be resumed, Pulumi does not currently
take `PendingReplacement` into account, and always enqueues a(nother)
delete operation. This is typically fine (albeit wasteful) since deletes
are (should) be idempotent, but unnecessary. This commit adds
@jesse-triplewhale's fix for this behaviour whereby the
`PendingReplacement` flag is simply removed before the remainder of the
required steps (replace, create replacement) are actioned as normal. It
also extends this work with some lifecycle tests for this scenario and a
few others that may arise as a result of an interrupted replacement.
Fixes#16288Closes#16303
Co-authored-by: Jesse Grodman <jesse@triplewhale.com>
We're considering whether it makes sense to document setting "default
providers" through transforms. For that we want to make sure the
transforms behave exactly the way we expect before trying to document
anything, and going ahead with that plan.
Currently it is the SDKs responsibility to resolve the providers map and
to resolve the parent relationship and send the provider in a resolved
manner to the engine. For example in the NodeJS SDK this currently
happens here:
91568bc03b/sdk/nodejs/resource.ts (L445-L469)
To make sure this works in the same way in every SDK, and that we can
add an engine test to make sure this works consistently, we add the same
resolution of the provider before we pass these options to the
transform. We'll keep the resolution in the SDK, which will be preferred
(we'll choose the Provider resource option first in any case), but now
also have a fallback to the same extend in the engine.
Also add a test to show the behaviour is correct to the engine.
Presently, the behaviour of diffing during refresh steps is incomplete,
returning only an "output diff" that presents the changes in outputs.
This commit changes refresh steps so that:
* they compute a diff similar to the one that would be computed if a
`preview` were run immediately after the refresh, which is more
typically what users expect and want; and
* `IgnoreChanges` resource options are respected when performing the new
desired-state diffs, so that property additions or changes reported by a
refresh can be ignored.
In particular, `IgnoreChanges` can now be used to acknowledge that part
or all of a resource may change in the provider, but the user is OK with
this and doesn't want to be notified about it during a refresh.
Importantly, this means that the diff won't be reported, but also that
the changes won't be applied to state.
The implementation covers the following:
* A diff is computed using the inputs from the program and then
inverting the result, since in the case of a refresh the diff is being
driven by the provider side and not the program. This doesn't change
what is stored back into the state, but it does produce a diff that is
more aligned with the "true changes to the desired state".
* `IgnoreChanges` resource options are now stored in state, so that this
information can be used in refresh operations that do not have access
to/run the program.
* In the context of a refresh operation, `IgnoreChanges` applies to
*both* input and output properties. This differs from the behaviour of a
normal update operation, where `IgnoreChanges` only considers input
properties.
* The special `"*"` value for `IgnoreChanges` can be used to ignore all
properties. It _also_ ignores the case where the resource cannot be
found in the provider, and instead keeps the resource intact in state
with its existing input and output properties.
Because the program is not run for refresh operations, `IgnoreChanges`
options must be applied separately before a refresh takes place. This
can be accomplished using e.g. a `pulumi up` that applies the options
prior to a refresh. We should investigate perhaps providing a `pulumi
state set ...`-like CLI to make these sorts of changes directly to a
state.
For use cases relying on the legacy refresh diff provider, the
`PULUMI_USE_LEGACY_REFRESH_DIFF` environment variable can be set, which
will disable desired-state diff computation. We only need to perform
checks in `RefreshStep.{ResultOp,Apply}`, since downstream code will
work correctly based on the presence or absence of a `DetailedDiff` in
the step.
### Notes
- https://github.com/pulumi/pulumi/issues/16144 affects some of these
cases - though its technically orthogonal
- https://github.com/pulumi/pulumi/issues/11279 is another technically
orthogonal issue that many providers (at least TFBridge ones) - do not
report back changes to input properties on Read when the input property
(or property path) was missing on the inputs. This is again technically
orthogonal - but leads to cases that appear "wrong" in terms of what is
stored back into the state still - though the same as before this
change.
- Azure Native doesn't seem to handle `ignoreChanges` passed to Diff, so
the ability to ignore changes on refresh doesn't currently work for
Azure Native.
### Fixes
* Fixes#16072
* Fixes#16278
* Fixes#16334
* Not quite #12346, but likely replaces the need for that
Co-authored-by: Will Jones <will@sacharissa.co.uk>
<!---
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->
# Description
<!--- Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context. -->
This is the first part of paramaterized providers in the engine. This
only supports "replacement" packages, that is where we fully replace a
providers package with a new package (think how dynamic tfbridge will
work, vs how crd2pulumi will work).
I've made the decision to _not_ support using parameterised providers by
sending the parameter in the RegisterResource request. This will
necessitate some different work in how we send the parameter for
explicit providers compared to version and pluginDownloadURL, but I
think it's worth it going forward.
No changelog as this is still basically unusable without codegen support
done, and should still be considered primarily for internal experimental
use for now.
## Checklist
- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
- [ ] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
When using `--target` to target specific resources during an update, we
use the list of targets to decide which steps to generate given a set of
resource registrations. Specifically:
* If the registration event names a resource that is targeted, we
process it as usual.
* If the registration event names a resource that _is not_ targeted, we
emit a `SameStep` for it.
In the latter case, the emission of a `SameStep` means that the old
state for the resource will be copied across to the new state. This is
the desired behaviour -- the resource was not targeted and so the new
state should contain the resource exactly as it was prior to the update.
However, this presents a problem if the old state has references to
resources that either will not appear in the new state, or will appear
in the wrong place. Consider the following program in TypeScript-esque
pseudocode:
```typescript
const a = new Resource("a")
const b = new Resource("b", { dependency: a })
const c = new Resource("c")
```
Here, `b` depends on `a`, while `a` and `c` have no dependencies. We run
this program without specifying targets and obtain a state containing
`a`, `b` and `c`, with `a` appearing before `b` due to `b`'s dependency
on `a`. We now modify the program as follows:
```typescript
const b = new Resource("b")
const c = new Resource("c")
```
`a` has been removed from the program and consequently `b` no longer
depends on it. We once more run the program, this time with a `--target`
of `c`. That is to say, neither `a` nor `b` is targeted. The execution
proceeds as follows:
* `a` is not in the program, so no `RegisterResourceEvent` will be
emitted and processed for it.
* `b` is in the program, but it is not targeted. Its
`RegisterResourceEvent` will be turned into a `SameStep` and `b`'s _old
state will be copied as-is to the new state_.
* `c` is in the program and is targeted. It will be processed as normal.
At the end of execution when we come to write the snapshot, we take the
following actions:
* We first write the processed resources: `b`'s old state and `c`'s new
state.
* We then copy over any unprocessed resources from the base (previous)
snapshot. This includes `a` (which is again desirable since its deletion
should not be processed due to it not being targeted).
Our snapshot is now not topologically sorted and thus invalid: `b` has a
dependency on `a`, but `a` appears after it. Presently this bug will
manifest irrespective of the nature of the dependency: `.Dependencies`,
`.PropertyDependencies` and `.DeletedWith` are all affected.
This commit fixes this issue by traversing all untargeted resource
dependency relationships and ensuring that `SameStep`s (or better if
they have been targeted) are emitted before emitting the depending
resource's `SameStep`.
* Fixes#16052
* Fixes#15959
<!---
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->
# Description
<!--- Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context. -->
This adds support for a `RegisterProvider` method to the engine. This
allows an SDK process to send the information for a package (name,
version, url, etc, and parameter in the future) and get back a UUID for
that run of the engine that can be used to re-lookup that information.
That allows the SDK to just send the `provider` field in
`RegisterResourceRequest` instead of filling in `version`,
`pluginDownloadURL` etc (and importantly not having to fill in
`parameter` for parameterised providers, which could be a large amount
of data).
This doesn't update any of the SDKs to yet use this method. We can do
that piecemeal, but it will require core sdk and codegen changes for
each language.
## Checklist
- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
- [x] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
Refreshes have their own way of dealing with errors, which does not
match what we want to do with ContinueOnError (it's further reaching, we
still want to refresh dependents of failed resources). Therefore we made
them mutually exclusive.
The original change however missed that it's also possible for refreshes
to happen during `pulumi up` using the `--refresh` flag. This currently
results in an assert/panic.
Fix this by not passing the `ContinueOnError` option through when we
have a refresh. This still gives us the right behaviour since refresh
ignores the errors anyway.
Fixes https://github.com/pulumi/pulumi/issues/16177
## Checklist
- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
- [x] I have formatted my code using `gofumpt`
<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!---
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @Pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
We want to add more test coverage to the display code. The best way to
do that is to add it to the engine tests, that already cover most of the
pulumi functionality.
It's probably not really possible to review all of the output, but at
least it gives us a baseline, which we can work with.
There's a couple of tests that are flaky for reasons I don't quite
understand yet. I marked them as to skip and we can look at them later.
I'd rather get in the baseline tests sooner, rather than spending a
bunch of time looking at that. The output differences also seem very
minor, so not super concerning.
The biggest remaining issue is that this doesn't interact well with the
Chdir we're doing in the engine. We could either pass the CWD through,
or just try to get rid of that Chdir. So this should only be merged
after https://github.com/pulumi/pulumi/pull/15607.
I've tried to split this into a few commits, separating out adding the
testdata, so it's hopefully a little easier to review, even though the
PR is still quite large.
One other thing to note is that we're comparing that the output has all
the same lines, and not that it is exactly the same. Because of how the
engine is implemented, there's a bunch of race conditions otherwise,
that would make us have to skip a bunch of tests, just because e.g.
resource A is sometimes deleted before resource B and sometimes it's the
other way around.
The biggest downside of that is that running with `PULUMI_ACCEPT` will
produce a diff even when there are no changes. Hopefully we won't have
to run that way too often though, so it might not be a huge issue?
---------
Co-authored-by: Fraser Waters <fraser@pulumi.com>