-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add resource option PreCheckCallback #1310
Conversation
Diff for pulumi-azuread with merge commit 539459f |
Diff for pulumi-random with merge commit 539459f |
https://go.dev/play/p/-eNaTRqOVhu perhaps consider a way to override it without making changes to this codebase? It seems Go language has the facility to do this via embedded structs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it not be overridden in Go by other means outside of making edits to this codebase?
IN addition, FWIW there's PreConfigureCallback machinery that is allowed to modify the data at Pulumi layer before it drops to TF; not sure if that is sufficient for the use case you have. |
I don't see how this is applicable to this case. Where would this come into play? Which interface would I be hijacking here?
|
Ok, my bad you're right about PreConfigureCallback belonging to a different part of the system. Regarding overriding via Go embedding I guess it can be too tricky to be useful if we rely on calling into Main/ having multiple layers. So let's discuss making a change to this codebase to support customizing Check. One sec. |
Giving it a second read, I think this can be useful to have, I wanted to reach out for something like this in other work. Perhaps let's call it PreCheckCallback? Add some quick tests to make sure it's covered, similar to PreConfigureCallback. |
Diff for pulumi-random with merge commit b97edc4 |
Diff for pulumi-azuread with merge commit b97edc4 |
pkg/tfbridge/provider_test.go
Outdated
callback := func(_ context.Context, config, _ resource.PropertyMap) (resource.PropertyMap, error) { | ||
delete(config, "conflictingProperty2") | ||
config["arrayPropertyValues"] = resource.NewArrayProperty([]resource.PropertyValue{ | ||
resource.NewStringProperty("v1"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tough to follow, do we have an assert somewhere (where)? that observes that the effect of this callback has been actually performed? resource.NewStringProperty("v1")
I looked around for "v1" but cannot find it easily.
For an example of how to make asserts more obviously visually correlated to the property being tested, have a look at TestPreConfigureCallback
in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testCheckFailures
returns no errors. Without the callback, it returns 3 errors. We don't have a direct assertion that the callback run, but we observe its results.
This is great just needs a little more test TLC. |
a90f474
to
0d807d9
Compare
Diff for pulumi-random with merge commit 011045c |
Diff for pulumi-azuread with merge commit 011045c |
0d807d9
to
e10adff
Compare
Diff for pulumi-azuread with merge commit 6266eaa |
Diff for pulumi-random with merge commit 6266eaa |
e10adff
to
272e881
Compare
Diff for pulumi-azuread with merge commit ea090d2 |
return config, nil | ||
} | ||
|
||
for _, f := range factories { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By-reference loop variable capture :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test isn't parallelized. I can change that (272e881), but the by-ref isn't actually a risk here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wanted to enable a linter that flags these but we can deal with it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen golang/go#56010?
@@ -550,6 +521,104 @@ func TestProviderCheckV2(t *testing.T) { | |||
assert.Equal(t, "", failures[2].Property) | |||
} | |||
|
|||
func TestProviderCheck(t *testing.T) { | |||
testFailures := map[string]func(*testing.T, []*pulumirpc.CheckFailure){ | |||
"v1": testCheckFailuresV1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I think V1 is not in prod anymore so in the future we can avoid any work pertaining to testing it, and start deprecating/removing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
272e881
to
b26fd13
Compare
Diff for pulumi-azuread with merge commit 3b96d9e |
* Add GRPC test to replicate 2633 * Test that we set "tags" on creation TF does, and if we assume that "tags" is set correctly, we then get the expected diff. * Ensure valid diffs by overriding check * Remove `.XComputeInput` This fix takes effect during Check, which occurs before Diff. We don't need to do anything special for Diff now. * Depend on pulumi/pulumi-terraform-bridge#1310 * Turn tests back on * Consume upstream changes The previous fix works on all resources... except our s3legacy bucket because it was not updated to use upstream's new tags strategy. I have changed our fork to use the new strategy. The change is here: https://github.com/pulumi/terraform-provider-aws/compare/patched-v5.9.0...patched-v5.9.0-with-modern-s3legacy-tags?expand=1 * Explain .Mappable() has the same shape as NewTagConfig expected * Template out YAML Tests * Accommodate aws:cognito:UserPool * Switch to environment for pf * Add a test for aws:s3:BucketV2 We are seeing tests fail for aws:cognito:UserPool but I know this works for aws:s3:BucketV2. These are both SDKv2 resources. Since they work differently I have ensured that both are tested. * Skip aws:cognito:UserPool test This resource is broken upstream, so we skip the test here. * Remove outdated tests I have left skeleton code for GRPC based testing, as I often find it useful when debugging locally. I have noted that it does not run in CI, and is just there for local development. * Don't pass empty tags properties
Add the ability to override the check behavior for resources.