-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix tf sdk plugin upgrade #242
Conversation
@@ -1021,7 +1021,9 @@ var planPluginSDKUpgrade = stepv2.Func12E("Planning Plugin SDK Upgrade", func( | |||
currentBranch, ok := refs.labelOf(currentRef) | |||
if !ok { | |||
// use latest versioned branch |
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.
Looks like this comment was inaccurate - it would previously return "" which got picked up on
upgrade-provider/upgrade/upgrade_provider.go
Line 213 in 1b9fe00
if tfSDKTargetSHA == "" { |
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 strikes me as very approximate that this code reaches out to terraform-plugin-sdk Git metadata. Why does it need to do that? Is it trying to install the forked terraform-plugin-sdk ahead of the bridge for some reason? Is it possible to instead propagate the replace version from pulumi-terraform-bridge so that the forked terraform-plugin-sdk tracks the bridge it was tested with?
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.
Is it trying to install the forked terraform-plugin-sdk ahead of the bridge for some reason
This is exactly what it does but that seems to be intended IIUC.
I'm happy to look into fixing it but I'd rather ship this first to unblock bridge upgrades.
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 that's fair we can backlog this. I'm not too comfortable reviewing this codebase but was hoping for some test to match the code changes I thought we're getting more serious about testing this as it is a little hairy? much appreciated.
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 strikes me as very approximate that this code reaches out to terraform-plugin-sdk Git metadata. Why does it need to do that? Is it trying to install the forked terraform-plugin-sdk ahead of the bridge for some reason? Is it possible to instead propagate the replace version from pulumi-terraform-bridge so that the forked terraform-plugin-sdk tracks the bridge it was tested with?
It does this because it is updating the replace
d directive. It can't get picked up by go mod tidy
because replace
s are not transitive.
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.
We could do some regex magic to get the right version from tf-bridge
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.
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.
Let's put a test in, but otherwise this looks good to merge.
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
fixes pulumi/pulumi-keycloak#379 and similar - bridge upgrades which require a tf plugin sdk upgrade.
example: https://github.com/pulumi/pulumi-keycloak/pull/381/files