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.

Sign up for GitHub

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

Jump to bottom

Fix tf sdk plugin upgrade #242

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Fix tf sdk plugin upgrade #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f296aa7
fix tf sdk plugin upgrade
VenelinMartinov Jan 31, 2024
2b3682f
add test
VenelinMartinov Feb 1, 2024
361fd6d
add comment about tests in readme
VenelinMartinov Feb 1, 2024
0a3dcea
add todo
VenelinMartinov Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@
vendor/
bin/
upgrade-provider
go.work
go.work.sum
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,9 @@ Values include:
- `pr-reviewers`: A comma separated list of reviewers to assign the upgrade PR to.
- `pr-assign`: A user to assign the upgrade PR to (default: `@me`).

## Writing tests
Use `PULUMI_REPLAY=logs.json upgrade-provider...` to record logs to use in replay tests like [this](https://github.com/pulumi/upgrade-provider/blob/2b3682f894e0b8d85673cee0c0f50fb25ad067b6/upgrade/steps_test.go#L287).

## Project Guidelines

### Goals
Expand Down
7 changes: 6 additions & 1 deletion upgrade/steps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,12 @@ var planPluginSDKUpgrade = stepv2.Func12E("Planning Plugin SDK Upgrade", func(
currentBranch, ok := refs.labelOf(currentRef)
if !ok {
// use latest versioned branch
Copy link
Contributor Author

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 == "" {
to mean don't upgrade.

Copy link
Member

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?

Copy link
Contributor Author

@VenelinMartinov VenelinMartinov Jan 31, 2024 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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 replaced directive. It can't get picked up by go mod tidy because replaces are not transitive.

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#245

return "", fmt.Sprintf("Could not find head branch at ref %s. Upgrading to "+
// This is not quite correct, since it could be newer than the
// version used in the bridge.
// TODO: https://github.com/pulumi/upgrade-provider/issues/245
latestSha, ok := refs.shaOf(fmt.Sprintf("refs/heads/upstream-%s", latest.Original()))
contract.Assertf(ok, "Failed to lookup sha of known tag: %q not in %#v", latest.Original(), refs.labelToRef)
return latestSha, fmt.Sprintf("Could not find head branch at ref %s. Upgrading to "+
"latest branch at %s instead.", currentRef, latest), nil
}

Expand Down
87 changes: 84 additions & 3 deletions upgrade/steps_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ func TestGetWorkingBranch(t *testing.T) {
t.Run("", testF(tt))
}
})

}

func TestHasRemoteBranch(t *testing.T) {
Expand Down Expand Up @@ -284,8 +283,8 @@ func TestParseUpstreamProviderOrgFromModVersion(t *testing.T) {
}
]`), "Get UpstreamOrg from module version", parseUpstreamProviderOrg)
}
func TestCheckMaintenancePatchWithinCadence(t *testing.T) {

func TestCheckMaintenancePatchWithinCadence(t *testing.T) {
testReplay((&Context{
GoPath: "/Users/myuser/go",
}).Wrap(context.Background()),
Expand Down Expand Up @@ -324,7 +323,6 @@ func TestCheckMaintenancePatchWithinCadence(t *testing.T) {
}

func TestCheckMaintenancePatchExpiredCadence(t *testing.T) {

testReplay((&Context{
GoPath: "/Users/myuser/go",
}).Wrap(context.Background()),
Expand Down Expand Up @@ -360,3 +358,86 @@ func TestCheckMaintenancePatchExpiredCadence(t *testing.T) {
}
]`), "Check if we should release a maintenance patch", maintenanceRelease)
}

func TestPluginSDKUpgradeLatest(t *testing.T) {
testReplay((&Context{
GoPath: "/Users/myuser/go",
}).Wrap(context.Background()),
t, jsonMarshal[[]*step.Step](t, `[
{
"name": "Planning Plugin SDK Upgrade",
"inputs": [
{
"Name": "pulumi-keycloak",
"Org": "pulumi"
}
],
"outputs": [
"74776a5cd5f9a1330c34124588e0ad800d26724d",
"Could not find head branch at ref e6d96b3b8f7e. Upgrading to latest branch at 2.29.0 instead.",
null
]
},
{
"name": "Original Go Version of",
"inputs": [
{
"Name": "pulumi-keycloak",
"Org": "pulumi"
},
"provider/go.mod",
"github.com/pulumi/terraform-plugin-sdk/v2"
],
"outputs": [
{
"Path": "github.com/pulumi/terraform-plugin-sdk/v2",
"Version": "v2.0.0-20230912190043-e6d96b3b8f7e"
},
true,
null
]
},
{
"name": "git",
"inputs": [
"git",
[
"show",
":provider/go.mod"
]
],
"outputs": [
"module github.com/pulumi/pulumi-keycloak/provider/v5\n\ngo 1.21\n\nreplace (\n\tgithub.com/hashicorp/terraform-plugin-sdk/v2 =\u003e github.com/pulumi/terraform-plugin-sdk/v2 v2.0.0-20230912190043-e6d96b3b8f7e\n\tgithub.com/hashicorp/vault =\u003e github.com/hashicorp/vault v1.2.0\n\tgithub.com/mrparkers/terraform-provider-keycloak =\u003e ../upstream\n)\n\nrequire (\n\tgithub.com/mrparkers/terraform-provider-keycloak v0.0.0-00010101000000-000000000000\n\tgithub.com/pulumi/pulumi-terraform-bridge/v3 v3.72.0\n\tgithub.com/pulumi/pulumi/sdk/v3 v3.103.1\n)\n\nrequire (\n\tcloud.google.com/go v0.110.8 // indirect\n\tcloud.google.com/go/compute v1.23.0 // indirect\n)\n",
null
],
"impure": true
},
{
"name": "git refs of",
"inputs": [
"https://github.com/pulumi/terraform-plugin-sdk.git",
"heads"
],
"outputs": [
{},
null
]
},
{
"name": "git",
"inputs": [
"git",
[
"ls-remote",
"--heads",
"https://github.com/pulumi/terraform-plugin-sdk.git"
]
],
"outputs": [
"efd600e5c6b7a15badde5c32d6b16312c8823409\trefs/heads/dependabot/go_modules/golang.org/x/crypto-0.17.0\n313a7c4cbcad744ab88abc26b42f8e526680ce49\trefs/heads/dependabot/go_modules/golang.org/x/net-0.17.0\ndacd5f7afbedcd7aeb5881b94573369385692784\trefs/heads/dependabot/go_modules/google.golang.org/grpc-1.56.3\n7ac578ce47fc07e0888beee6d4ab9db09369274f\trefs/heads/master\na81109190d574226079b2ceff408cc7bca82f6fe\trefs/heads/pgavlin/upstream-v2.12.0\n430f685de305a148a5a79d1c3959ecf109986675\trefs/heads/upstream-v2.24.1\n3fa930f865709d507154454c2af20e023d15b6e6\trefs/heads/upstream-v2.26.1\n03a71d0fca3d7d5ff24a52e334aa2e52f442fe04\trefs/heads/upstream-v2.27.0\n74776a5cd5f9a1330c34124588e0ad800d26724d\trefs/heads/upstream-v2.29.0\n",
null
],
"impure": true
}
]`), "Planning Plugin SDK Upgrade", planPluginSDKUpgrade)
}
Loading