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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

chore: vendor more OpenTOFU code #2718

wants to merge 7 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 - yes they do
  • looks like dynamic still depends on opentofu indirectly, which has a different version from the vendored copy; perhaps benign but may be suspicious - this has been fixed so the code does not depend on opentofu proper yet
  • vendored code is patched to remove provisioners; this is likely not yet used in the bridge

@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?

@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

@VenelinMartinov
Copy link
Contributor

I think it's fine to remove t.Parallel from tests which need t.Setenv but you need to ignore the linter with //nolint:paralleltest

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.64%. Comparing base (917f85c) to head (456ab96).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2718   +/-   ##
=======================================
  Coverage   69.64%   69.64%           
=======================================
  Files         301      301           
  Lines       38726    38726           
=======================================
  Hits        26969    26969           
  Misses      10240    10240           
  Partials     1517     1517           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 13, 2024

The linter rule you mention @VenelinMartinov is not enabled as yet.

@t0yv0 t0yv0 marked this pull request as ready for review December 13, 2024 16:27
@t0yv0
Copy link
Member Author

t0yv0 commented Dec 13, 2024

Unfortunately Windows is not happy:

=== NAME  TestLoadProvider
    testing.go:1231: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestLoadProvider3788457442\001\registry.opentofu.org\hashicorp\tls\4.0.4\windows_amd64\terraform-provider-tls.exe: Access is denied.
--- FAIL: TestLoadProvider (4.84s)

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 13, 2024

Ah no the linter is there. Just it does not fire under make lint for some reason.

Error: dynamic/internal/shim/run/loader_test.go:35:1: Function TestLoadProvider missing the call to method parallel (paralleltest)
func TestLoadProvider(t *testing.T) {
^

@VenelinMartinov
Copy link
Contributor

Could it be a difference in local tool versions? Mikhail hit this recently. AFAIK CI runs make lint

@t0yv0
Copy link
Member Author

t0yv0 commented Dec 16, 2024

Worked this morning. Fixing.

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 1e64d87 to 456ab96 Compare December 16, 2024 15:46
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
3 participants