fix MockMonitor reporting DeletedWith wasn't supported #14118
Conversation
Changelog[uncommitted] (2023-10-09)Bug Fixes
|
sdk/nodejs/runtime/mocks.ts
Outdated
| @@ -227,7 +227,7 @@ export class MockMonitor { | |||
|
|
|||
| // Support for "outputValues" is deliberately disabled for the mock monitor so | |||
| // instances of `Output` don't show up in `MockResourceArgs` inputs. | |||
| const hasSupport = id === "secrets" || id === "resourceReferences"; | |||
| const hasSupport = process.env.PULUMI_DISABLE_OUTPUT_VALUES != "true" || id !== "outputValues"; | |||
There was a problem hiding this comment.
I'm not sure anything actually needs this env var anymore, but doesn't look like it'll hurt to check it.
Given we've got no complaints from dotnet about this it's probably fine to leave that as not checking the env var.
cc @justinvp as he might have a better insights to this setting
|
I opened pulumi/pulumi-dotnet#184 to make the .NET SDK consistent with the changes here. |
We should support all features in mocks that we are supporting otherwise. We did a similar thing in pulumi-dotnet: pulumi/pulumi-dotnet#93. However here we're not simply changing hasSupport to true. 1e09626 has a good explanation of why we only support specific values here. However really we only need to disable support for `outputValues`, and that only if output values are disabled. Originally this was true if `PULUMI_ENABLE_OUTPUT_VALUES` was set, but that environment variable was later inverted: 3027d01.
tgummerer
force-pushed
the
tg/support-deleted-with
branch
from
5186906
to
e5d620e
Compare
| @@ -494,10 +494,10 @@ func TestRegisterResource_aliasesSpecs(t *testing.T) { | |||
| // alias specs. | |||
| // So if that's needed, wrap the monitor to claim it | |||
| // does. | |||
| if tt.supportsAliasSpecs { | |||
| if !tt.supportsAliasSpecs { | |||
There was a problem hiding this comment.
The changes necessary in this test made me a little bit worried that we'd be breaking some backwards compatibility here, but we're testing internal things here, so maybe it's okay? I'd appreciate some extra eyes here.
sdk/nodejs/tests/provider.spec.ts
Outdated
| @@ -495,7 +495,7 @@ describe("provider", () => { | |||
| it(`deserializes '${test.name}' correctly`, async () => { | |||
| pulumi.runtime.setMocks(new TestMocks(), "project", "stack", true); | |||
| pulumi.runtime.registerResourceModule("test", "index", new TestModule()); | |||
| new TestResource("name"); // Create an instance so it can be deserialized. | |||
| await new TestResource("name"); // Create an instance so it can be deserialized. | |||
There was a problem hiding this comment.
We were previously not awaiting this resource, so when we started resolving aliases during the test because the mock supports that now, things started failing because the resource wasn't fully registered yet by the time we were checking it. Regardless of the other changes in this PR I believe this is the right thing to do.
There was a problem hiding this comment.
AFAIK, it is not possible to await a constructor, since it is never async and definitely doesn't return a Promise<T> 🤔
There was a problem hiding this comment.
Await the resources URN instead.
There was a problem hiding this comment.
Interesting, somehow it seemed to work in the test. I don't know a lot about typescript, I think I got it right now? Is 3a8b197 what you meant?
tgummerer
dismissed stale reviews from justinvp and Frassle
Would appreciate a double check on the things I commented on.
# Description Make sure the mockmonitor works the same way as the other SDKs as changed in pulumi/pulumi#14118 and pulumi/pulumi-dotnet#184. ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [ ] 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 updated the [CHANGELOG-PENDING](https://github.com/pulumi/pulumi/blob/master/CHANGELOG_PENDING.md) file with my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, 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 Service API version <!-- @pulumi employees: If yes, you must submit corresponding changes in the service repo. -->
We should support all features in mocks that we are supporting otherwise. We did a similar thing in pulumi-dotnet: pulumi/pulumi-dotnet#93.
However here we're not simply changing hasSupport to true. 1e09626 has a good explanation of why we only support specific values here. However really we only need to disable support for
outputValues, and that only if output values are disabled. Originally this was true ifPULUMI_ENABLE_OUTPUT_VALUESwas set, but that environment variable was later inverted: 3027d01.This ended up being a little bit more complicated than just passing true, but I think it's more correct this way. I'm also happy to change it back to just passing through
trueif that's preferred. Once we decided on whether this is the right way to do it vs. just passingtrueunconditionally I'll also updatejavaanddotnet(if we're going this way) to do the same.Fixes #13355
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change