https://pulumi.com logo
Join Slack
Powered by
There need to be end-to-end tests in the bridge fo...
# package-authoring
a
There need to be end-to-end tests in the bridge for 
.MuxWith
(for sdk, pf and muxed providers). At minimum, we need to show that the new providers have their schemas added and that runtime dispatch works as expected. I think the clearest way to do that will be to write a test provider that uses 
.MuxWith
. We have a couple of these test providers already in the bridge: https://github.com/pulumi/pulumi-terraform-bridge/blob/fe7b24d6a9128e06d8c281bd0f2f089676abadc7/internal/testprovider/schema_mini_random.go#L21-L23 https://github.com/pulumi/pulumi-terraform-bridge/blob/fe7b24d6a9128e06d8c281bd0f2f089676abadc7/pkg/tfgen/internal/testprovider/minirandom.go#L[…]28 For documentation, I meant that 
muxer.Provider
and 
tfbridge.ProviderInfo.MuxWith
need doc comments explaining what they do.
b
@ancient-policeman-24615 @limited-rainbow-51650 opened a PR https://github.com/pulumi/pulumi-terraform-bridge/pull/1605 I was able to implement tests for the schema generation of a muxed provider. Not sure how to implement the validation that a muxed provider resource or function is correctly called. Will try to dig down in the code of the bridge to find a way how to do that. Of course I'll be happy about a hint!
@ancient-policeman-24615 I saw that PF in the Bridge uses a completely different approach to mux providers. (AugmentShimWithPF). The code is used at least in the Pulumi Cloudflare, Exoscale and MongoDBAtlas providers. So we can't introduce a breaking change here and align the code with what I created for the TF SDK V1 and V2 providers. Because we have no muxing for TF SDK providers yet we should align here with the code used for PF based providers. CC @limited-rainbow-51650 https://github.com/search?q=MuxShimWithPF&type=code
@ancient-policeman-24615 Main issue with this approach will be that the required interface 
shim.Provider
does not align with the interface exposed by the GO Provider Framework 
provider.Provider
. So you can't use the GO Provider Framework to create a mux provider. At least inconvenient. However, I would go so far as to say that all these interfaces for providers make the Pulumi ecosystem look chaotic in this area.
l
@big-architect-71258 I guess this is related: https://github.com/pulumi/pulumi-terraform-bridge/issues/1487
b
@limited-rainbow-51650 not only the entry points got proliferated but as well the interfaces that you have to implement for a provider. This may sound egocentric, but compared to "my" code with a MuxWith array in the ProviderInfo struct, using the AugmentShimWithPF function and the necessary code to distinguish which provider (upstream or mux) is executing a resource or function is much more complex. @ancient-policeman-24615
a
I saw that PF in the Bridge uses a completely different approach to mux providers. (AugmentShimWithPF). The code is used at least in the Pulumi Cloudflare, Exoscale and MongoDBAtlas providers. So we can’t introduce a breaking change here and align the code with what I created for the TF SDK V1 and V2 providers. Because we have no muxing for TF SDK providers yet we should align here with the code used for PF based providers.
We will need to keep that functionality, since it is deployed in many of our providers. That said, I don’t think we need to align muxing a generic pulumi provider with the 
AugmentShimWithPF
call. The two are meaningfully different. For example, 
tfbridge.ProviderInfo
mappings apply to the SDK V{1,2} and PF providers. They will not apply to 
MuxWith
addins. I think the 
.MuxWith
design makes sense for adding in a generic provider.
Main issue with this approach will be that the required interface 
shim.Provider
does not align with the interface exposed by the GO Provider Framework 
provider.Provider
. So you can’t use the GO Provider Framework to create a mux provider. At least inconvenient. However, I would go so far as to say that all these interfaces for providers make the Pulumi ecosystem look chaotic in this area.
shim.Provider
is an abstraction over a TF provider. 
provider.Provider
is an abstraction over a Pulumi provider. The interfaces are very different, but I agree that the work provider is overused here. --- @big-architect-71258 I’m reviewing your PR now.
b
@ancient-policeman-24615 so are the two types of muxing disjunct? I.e. you can't (won't) use 
AugmentShimWithPF
at the same time with 
MuxWith
? Next possible issue: 
pf
has a different entry point as 
tfbridge
for SDK V{1,2} providers (as mentioned by @enough-garden-22763 in https://github.com/pulumi/pulumi-terraform-bridge/issues/1487). Currently I only added the code to evaluate 
MuxWith
to 
tfbridge
. Does the code in PF also need to be changed? And if so, how do we prevent a collision with 
AugmentShimWithPF
?
@ancient-policeman-24615 pushed new commits according to your first review
a
@big-architect-71258 The two types of muxing can be disjoint in the initial PR, but long term they need to be composable. Right now, can you insert a check into 
pf/tfbridge.serve
and 
pf/tfbridge.MainWithMuxer
that errors if 
len(ProviderInfo.MuxWith) > 0
. Eventually, we will want you to be able to combine arbitrary Terraform providers (served from 
ProviderInfo.P
) and arbitrary Pulumi providers (served from 
ProviderInfo.MuxWith
). I don’t think we need that all in 1 PR. A series of PRs is much more likely to merge then one monster PR.
I’m taking a look at the new changes now.
b
@ancient-policeman-24615 Then I understood correctly that the PF implementation also needs to be changed and that one challenge is to combine 
MuxWith
with 
AugmentShimWithPF
. I totally agree with you that it is not a good idea to create a monster PR to squeeze the PF customizations into the current PR. However, I will add a 
len(ProviderInfo.MuxWith) > 0
with an 
error
to 
pf/tfbridge.MainWithMuxer
to avoid problems with unadapted code there.
a
Then I understood correctly that the PF implementation also needs to be changed and that one challenge is to combine 
MuxWith
with 
AugmentShimWithPF
.
Yep
However, I will add a 
len(ProviderInfo.MuxWith) > 0
with an 
error
to 
pf/tfbridge.MainWithMuxer
to avoid problems with unadapted code there.
Great
b
@ancient-policeman-24615 There should be an issue for having the pf code changed for MuxWith. Shall I create one? Or is there already an issue that could be changed?
a
I don’t think there is an existing issue. Please feel free to create one.
b
On my list 😁
@ancient-policeman-24615 pushed the code for 
len(ProviderInfo.MuxWith) > 0
Had to use 
panic
because the methods 
MainWithMuxer
for 
pf/tfgen
and 
pf/tfbridge
do not return an 
error
.
@ancient-policeman-24615 @enough-garden-22763 added tests for provider muxing. Issue right now: 
liniting
fails. I local `make lint`is successful though.
image.png
Cleaning the local cache didn't make any difference.
l
@big-architect-71258 line 15: no go files to analyze. Are there any source files that weren't pushed to the branch perhaps?
b
@limited-rainbow-51650 nope branch is clean. And I can't reproduce the error locally. Running the same command 
make lint
and having the the same version of 
golangci-lint
installed.
a
make lint
fails locally for me on your PR.
b
@ancient-policeman-24615 WT*!?!?! 🙄 How could that be??
a
$ golangci-lint --version
golangci-lint has version 1.55.2 built with go1.21.3 from e3c2265 on 2023-11-02T214002Z
Could there be some ignored files that are linked? 
git clean -fxd
is my go to hammer for this (but it will removed everything not checked into git).
b
@ancient-policeman-24615 my version of 
golangci-lint
is slightly different. Hard to spot the difference golangci-lint has version 1.55.2 built with go1.21.3 from e3c2265*f* on 2023-11-03T125925Z
Let me try the hammer 😄
a
@big-architect-71258 I think you are missing a 
go mod tidy
in 
pkg/tests
.
Running that got the linter working for me.
b
@ancient-policeman-24615 after cleaning 
golanglint-ci
fails.
l
Progress! Now it fails for everybody. 🤣
b
After 
go mod tidy
linting is successful. Pushing the changes now.
@ancient-policeman-24615 Thanks for helping!!
a
No problem.
b
Something learned again. 👍🏻 But I've to admin the error message of 
golanglint-ci
could be improved. Changes pushed BTW
@ancient-policeman-24615 @enough-garden-22763 do we need another review for the PR, or could it be merged now?
e
Yeah I'll review again tomorrow-ish. Thank you!
2 Views
Open in Slack
PreviousNext
Powered by