Skip to content
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

chore: vendor more OpenTOFU code #2718

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

chore: vendor more OpenTOFU code #2718

wants to merge 3 commits into from

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Dec 11, 2024

Changes:

  • dynamic/internal/shim is gone in favor of more files in pkg/vendored/opentofu
  • vendored more files from opentofu to accomplish this
  • simpler structure of go modules under dynamic/* - two modules removed
  • is dynamic/go.mod still needed as a separate module?

I think it fixes #2668 or it should do that because the resulting solution bundles vendored opentofu code that rewrites the generated protobuf files to use the Pulumi bridge specific copy.

Fixes #2668

Concerns:

  • do acceptance tests on dynamic bridged providers pass
  • looks like dynamic still depends on opentofu indirectly, which has a different version from the vendored copy; perhaps benign but may be suspicious
  • vendored code is patched to remove provisioners; this is likely not yet used in the bridge, but maybe causes trobule

@t0yv0 t0yv0 requested review from VenelinMartinov, iwahbe and blampe and removed request for VenelinMartinov December 11, 2024 01:31
@t0yv0 t0yv0 force-pushed the t0yv0/vendor-opentofu branch from a72637c to 6bbab4a Compare December 11, 2024 01:36
go.mod Outdated
@@ -40,6 +41,8 @@ require (
github.com/mitchellh/mapstructure v1.5.0
github.com/mitchellh/reflectwalk v1.0.2
github.com/olekukonko/tablewriter v0.0.5
github.com/opentofu/opentofu v1.8.7
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is a little suspect, likely we're not shadow-rewriting all imports in pkg/vendored/opentofu.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this. Fully using vendored (renamed) packages. No direct dependency on opentofu.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 11, 2024

Hmm this is interesting, this approach is not quite right, deep inside debugging TestLoadProvider I get an issue.

Deep inside the call stack, gRPC is unhappy with:

    Message "unknown service tfplugin5_pulumi.Provider"

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 11, 2024

I think I figured that one out, we need to be more careful with how protoc is invoked.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with vendoring, but I'd like us to re-examine the process by which we vendor. I have two specific questions:

  • Can we link the new code in the pkg/vendored/README.md?
  • Can we standardize on a single generate.go script (instead of one per vendored directory)?

Once we do this, I don't see any reason to keep the dynamic/go.mod anymore.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 11, 2024

Streamlined vendor folder layout a bit. Single generate.go now.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 11, 2024

One reason to keep the module around is scoping this redirect:

	// Use OpenTofu's fork of hcl (copied from OpenTofu).
	github.com/hashicorp/hcl/v2 => github.com/opentofu/hcl/v2 v2.0.0-20240416130056-03228b26f391

I think hashicorp/hcl is used in non-dynamic bridge code already and I wonder if the replacement will cause any unwanted changes there, e.g. does it need to be there. We'll need test covering why it's there.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 11, 2024

I think I'll postpone removing dynamic/go.mod to a subsequent PR. Focusing on getting tests to run here for now.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 11, 2024

make: *** [test] Error 1
        	Error Trace:	/Users/runner/work/pulumi-terraform-bridge/pulumi-terraform-bridge/dynamic/internal/shim/run/loader_test.go:42
        	Error:      	Received unexpected error:
        	            	provider registry.opentofu.org/hashicorp/tls 4.0.4 is not available for darwin_arm64
        	Test:       	TestLoadProvider/registry

Surprising because I think I can pass it locally? Perhaps platform specific issues?

In particular this achieves using Pulumi-specific proto generated Go code avoiding errors of double-loading copies of
the same Go proto code in the same process.
@t0yv0 t0yv0 force-pushed the t0yv0/vendor-opentofu branch from ed1b2b6 to 478f42b Compare December 12, 2024 20:43
@t0yv0
Copy link
Member Author

t0yv0 commented Dec 12, 2024

Fixed an issue with an over-eager patch that disabled code when vendoring. Let us see if this passes now.

@t0yv0 t0yv0 requested a review from iwahbe December 12, 2024 20:46
Copy link
Member Author

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Parallel() is not compatible with t.Setenv

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make dynamic/ tests runnable without special command
2 participants