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

Update to Plugin Framework #190

Merged
merged 21 commits into from
Mar 9, 2023
Merged

Update to Plugin Framework #190

merged 21 commits into from
Mar 9, 2023

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Mar 3, 2023

This is the third attempt to upgrade to Plugin Framework.

Requires: pulumi/pulumi-terraform-bridge#860

Fixes: #149

PreStateUpgradeHook is added to ensure that despite breaking changes upstream and the fact that upstream does not properly upgrade state, the problem #173 does not recur and simple programs with self-signed certs generate update, not replace plans when upgrading the provider.

Upstream crosses a major version boundary and we do not have capacity to completely compensate to make it non-breaking, therefore it's a new major release (v5.x.x.). The PR includes changes to move code to v5 (especially affecting Go).

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
No new resources/functions.

1 similar comment
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
No new resources/functions.

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.

It looks like schema changes broke one of our tests.

@t0yv0 t0yv0 force-pushed the plugin-framework-3 branch from 7639a99 to 38d707e Compare March 8, 2023 16:21
@t0yv0
Copy link
Member Author

t0yv0 commented Mar 8, 2023

Yes that's a breaking change from upstream I don't know how to compensate for, so editing out the examples for now.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
No new resources/functions.

@t0yv0 t0yv0 requested review from iwahbe and a team March 8, 2023 22:45
@@ -0,0 +1,103 @@
// go run scripts/new_major_version.go -ver v5 -name tls goes part of the way to udpate all Go source references to a
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling this out. Doesn't really belong here, but I was trying to see if it can automate part of the edits for a major version release via the "gofmt" route. This is the "regex based" alternative to pulumi/ci-mgmt#319

)

replace github.com/pulumi/pulumi-tls/sdk/v5 => ../../../sdk
Copy link
Member Author

Choose a reason for hiding this comment

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

Having this replace in makes it compilable before a 5.x release, though I think ProgramTest makes it possible to fix it up in flight.

@@ -1,18 +1,24 @@
# NOTE: temporary hand edited to add --version-prefix.
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling out this temporary edit. VERSION_PREFIX makes CI generate 5.x.x versions before an actual 5.x.x release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to make the PR to ci-mgmt first to edit major-version here:

https://github.com/pulumi/ci-mgmt/blob/master/provider-ci/providers/tls/config.yaml

And rebase on top of that? Might reduce the size of the diff, maybe obviate some of these changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather follow up with a PR to clean up. I've put the comments in pulumi/ci-mgmt#318 and sounds like we're doing that next iteration - once that's in this dance won't be needed anymore.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 9, 2023

Dropping the scripts code, taking it to pulumi/upgrade-provider#9

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
No new resources/functions.

VERSION_PATH := $(PROVIDER_PATH)/pkg/version.Version
TFGEN := pulumi-tfgen-$(PACK)
PROVIDER := pulumi-resource-$(PACK)
VERSION := $(shell pulumictl get version)
VERSION := $(shell pulumictl get version --version-prefix 5.0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I run this locally, I get the correct behavior without this change.

$ git tag v5.0.0
$ pulumictl get version --language generic
5.0.0

That should cover releases produced via tags. I tried this with a few different tags as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes to get this behavior in CI you need to publish the tag, which might trigger a release :) And if CI-local fidelity is lost then some tests break.

Comment on lines +63 to +69
"response": {
"changes": "DIFF_SOME",
"diffs": [
"privateKeyPem",
"subject"
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Note that this test is checking that setAuthorityKeyId, setSubjectKeyId, allowedUses is not present by implementing state upgrade?

Copy link
Contributor

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

This should prepare for a v5 release without issue, and we've already discussed fixes to ci-mgmt afterward to decouple major releases from ci-mgmt.

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

Does the PR have any schema changes?

Found 3 breaking changes:
Resource "tls:index/certRequest:CertRequest" missing input "keyAlgorithm"
Resource "tls:index/locallySignedCert:LocallySignedCert" missing input "caKeyAlgorithm"
Resource "tls:index/selfSignedCert:SelfSignedCert" missing input "keyAlgorithm"
No new resources/functions.

@t0yv0 t0yv0 merged commit 7c9d3d4 into master Mar 9, 2023
@t0yv0 t0yv0 deleted the plugin-framework-3 branch March 9, 2023 23:00
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.

Upgrade terraform-provider-tls to v4.0.4
3 participants