Always apply schema level secrets #286
Conversation
| @@ -933,7 +942,6 @@ func callCustomCheck[T any]( | |||
| // | |||
| // It also adds defaults to inputs as necessary, as defined by [Annotator.SetDefault]. | |||
| func DefaultCheck[I any](ctx context.Context, inputs resource.PropertyMap) (I, []p.CheckFailure, error) { | |||
| inputs = applySecrets[I](inputs) | |||
There was a problem hiding this comment.
Thoughts on maybe leaving this as is? Doing it twice is no harm?
Perhaps as an undocumented behavior of the exported DefaultCheck it's ok to remove it though.
There was a problem hiding this comment.
The caller of DefaultCheck only witnesses the effects of applySecrets[I] here when they see values that were previously secret show up as secret again (in the returned value of Check).
This doesn't change observable behavior from the user's perspective (except that applySecrets[I] covers values added in CustomCheck.Check, the goal of the PR).
infer/resource.go
Outdated
| var r R | ||
| if r, ok := ((interface{})(r)).(CustomCheck[I]); ok { | ||
| // The user implemented check manually, so call that. | ||
| // | ||
| // We do not apply defaults or secrets if the user has implemented Check | ||
| // themselves. Defaults and secrets are applied by [DefaultCheck]. | ||
|
|
||
| defCheckEnc, i, failures, err := callCustomCheck(ctx, r, req.Urn.Name(), req.Olds, req.News) | ||
| backupEncoder, _, _, _ := decodeCheckingMapErrors[I](req.News) |
There was a problem hiding this comment.
Are you sure failures, err need no handling here?
What if the backupEncoder moves under this if block:
if encoder == nil {
encoder = &backupEncoder
}
So as to compute it lazily if really needed, and it that case still may handle the error?
There was a problem hiding this comment.
+1 - it may be correct now but doesn't look robust
There was a problem hiding this comment.
Are you sure failures, err need no handling here?
I'll explain why in a comment. The tldr is: the backup encoder might not be derived from a shape that matches the shape of I, so it might not fit correctly. It will encode correctly without prior information (excluding marker types).
infer/resource.go
Outdated
| var r R | ||
| if r, ok := ((interface{})(r)).(CustomCheck[I]); ok { | ||
| // The user implemented check manually, so call that. | ||
| // | ||
| // We do not apply defaults or secrets if the user has implemented Check | ||
| // themselves. Defaults and secrets are applied by [DefaultCheck]. | ||
|
|
||
| defCheckEnc, i, failures, err := callCustomCheck(ctx, r, req.Urn.Name(), req.Olds, req.News) | ||
| backupEncoder, _, _, _ := decodeCheckingMapErrors[I](req.News) |
There was a problem hiding this comment.
+1 - it may be correct now but doesn't look robust
This will resolve pulumi/pulumi-docker-build#403 when pulumi-docker-build upgrades to this version.
iwahbe
force-pushed
the
iwahbe/always-apply-schema-level-secrets
branch
from
1401699 to
768c3fc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=======================================
Coverage 69.17% 69.17%
=======================================
Files 37 37
Lines 5109 5116 +7
=======================================
+ Hits 3534 3539 +5
- Misses 1306 1308 +2
Partials 269 269 ☔ View full report in Codecov by Sentry. |
|
This PR has been shipped in release v0.24.1. |
This will resolve pulumi/pulumi-docker-build#403 when pulumi-docker-build upgrades to this version.