-
Notifications
You must be signed in to change notification settings - Fork 45
Update pulumi/pulumi to v3.158.0 #2971
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2971 +/- ##
==========================================
+ Coverage 68.30% 68.49% +0.18%
==========================================
Files 330 330
Lines 42082 42105 +23
==========================================
+ Hits 28746 28839 +93
+ Misses 11757 11689 -68
+ Partials 1579 1577 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
70bfa95
to
3c45352
Compare
3c45352
to
8823d77
Compare
Makefile
Outdated
@@ -6,7 +6,7 @@ export PULUMI_DISABLE_AUTOMATIC_PLUGIN_ACQUISITION := true | |||
PROJECT_DIR := $(patsubst %/,%,$(dir $(abspath $(lastword $(MAKEFILE_LIST))))) | |||
|
|||
install_plugins:: | |||
pulumi plugin install converter terraform 1.0.20 | |||
pulumi plugin install converter terraform 1.1.0 |
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.
Why is this necessary, curious? Adds to the scope of the change. Perhaps separate PR?
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.
Local test runs do not enforce this constraint unless you run make
, which is difficult to do for an individual test. A lot of the pain here comes from different environments doing different things. :/
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.
Good point on enforcement, let us fix this. This kind of change needs to be a separate PR otherwise it is hard to see which change comes from what IMO.
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.
Yeah, completely agreed.
Name: a.Name, | ||
Project: a.Project, | ||
Type: a.Type, | ||
Type: *a.Type, |
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 can needlessly panic. A bit dangerous.
} | ||
p := tfbridge.ProviderInfo{ | ||
Name: "simple", |
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.
Why change from simple? Actually the delta on the "simple" provider is interesting to investigate.
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.
It is because of a change in the converter that doesn't allow for "fake" providers. Since this code path calls out to the command line, the converter attempts to download a provider called "simple", and it results in breaking Java codegen in this test.
The delta is not really all that interesting, because it is caused by the fact that this provider doesn't exist in real life.
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.
@lunaris and I did investigate for quite a while on what happened here on Friday.
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.
The challenge here now is that the converter will call out to Terraform's registry in order to try and resolve versions for providers that it thinks are outside the Pulumiverse. It's always done this, but this check is now earlier in the call chain in order to facilitate converting "any TF provider". We do this by dynamically bridging providers not in the Pulumiverse to e.g. facilitate generating mappings. This test now fails because "simple" is not in the Pulumiverse, but it is also not a "real" TF provider, so version resolution (and consequently producing a hint for mapping, which powers part of this test case) fails. There are several options here, none of them (on the surface) ideal, but enumerating in case people have thoughts/better ideas than I:
- Don't fail mapping hint generation for TF providers we can't resolve. I think this is fine, but it does make it trickier for
convert
to produce errors when e.g. the user has genuinely mentioned a provider that (as far as we're concerned) doesn't exist. That said, I think fixing that gets us further in this test (though it may fail later on due to missing/incompletely-mocked schema, as discussed elsewhere e.g. in the case of Java imports) - More completely mock out the processes that power this test -- we already mock out mappings (passing a JSON file on the CLI to the converter, so that it can just use those mappings as-is without remote calls), but we don't mock out the TF registry. We could look at doing that somehow here (likely not pretty, but possible)?
- Use a "real" provider so that the TF registry lookups succeed. This is the approach currently in this PR I believe. Again, not ideal, but I think (?) the simplest change?
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.
Reasonable but maybe a different PR? Too many moving parts at once. Also it needs to pin a version of openstack such as in here or why not use one that's already there perhaps.
pulumi-terraform-bridge/Makefile
Line 8 in 4c35e78
install_plugins:: |
I'm rebuilding this change #2973 as requested through ops handoff FWIW. |
Landed #2973 but there might be some interesting residual things in this PR that are worth merging up. There is in particular an upgrade to the TF converter version we use in tests. Note that it is not currently coupled with the converter version deployed in the bridged providers (pinned in .ci-mgmt.yaml) but both could use updating I guess. |
This skips one version of pulumi.
Also updates pulumi-yaml and pulumi-java to their respective
latest
.Small adjustments to GetPluginPath and ResolvePlugin.
Fixes #2958.