-
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
Cross-tests for PF's Configure #2467
Conversation
d02c353
to
775b13a
Compare
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.
LGTM, thanks for adding this! Out of scope here, but might be nice to add a simple resource test to make sure the library interface fits that nicely as well before adopting it more.
3063401
to
daa1350
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2467 +/- ##
==========================================
+ Coverage 57.13% 57.19% +0.06%
==========================================
Files 370 372 +2
Lines 51037 51243 +206
==========================================
+ Hits 29160 29310 +150
- Misses 20288 20336 +48
- Partials 1589 1597 +8 ☔ View full report in Codecov by Sentry. |
This is a pre-requisite for #2440.
daa1350
to
f4038d0
Compare
// The idea is that the "Configured Provider" should not be able to tell if it was configured | ||
// via HCL or Pulumi YAML: | ||
// | ||
// +--------------------+ +---------------------+ |
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.
Nice. Category theory.
|
||
func (w PFWriter) Provider(sch pfproviderschema.Schema, providerName string, config map[string]cty.Value) error { | ||
if !cty.ObjectVal(config).IsWhollyKnown() { | ||
return fmt.Errorf("WriteHCL cannot yet write unknowns") |
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.
Definitely out of scope but could be very nice. It could reference some proxy resource.
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 the framework bits right, not the coverage yet? If it adds coverage can you comment in-line to facilitate review? Appreciated!
t.Run("secret-string", crosstests.MakeConfigure( | ||
schema.Schema{Attributes: map[string]schema.Attribute{ | ||
"k": schema.StringAttribute{Optional: true}, | ||
}}, | ||
map[string]cty.Value{"k": cty.StringVal("foo")}, | ||
resource.PropertyMap{"k": resource.MakeSecret(resource.NewProperty("foo"))}, | ||
)) |
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 the framework bits right, not the coverage yet? If it adds coverage can you comment in-line to facilitate review? Appreciated!
There is some coverage here, but it is not extensive. We have coverage for:
- A plain string property
- A secret string property
- A plain bool property
- A string (
"false"
) passed to a bool property.
A follow-up PR will add actual coverage.
This PR has been shipped in release v3.92.0. |
The interface for configure cross-tests is:
Because the mapping between
tfConfig
andpuConfig
is non-trivial, and to allow for secret injection, I have chosen to allow settingtfConfig
andpuConfig
separately.This is a pre-requisite for #2440.
When the underlying provider configuration doesn't match between Pulumi and TF, the error looks like this: