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: invocation of getEngineVersion returns an error #4047

Merged
merged 1 commit into from
Jun 11, 2024
Merged

fix: invocation of getEngineVersion returns an error #4047

merged 1 commit into from
Jun 11, 2024

Conversation

corymhall
Copy link
Contributor

A patch was introduced via #3757 due to a bug in
pulumi-terraform-bridge.

The underlying bridge issue has now been fixed so I am removing the patch and restoring the original terraform behavior.

fixes #4043

@corymhall
fix: invocation of getEngineVersion returns an error
1116656 
A patch was introduced via #3757 due to [a bug in
pulumi-terraform-bridge](pulumi/pulumi-terraform-bridge#1809).

The underlying bridge issue has now been fixed so I am removing the
patch and restoring the original terraform behavior.

fixes #4043
@github-actions GitHub Actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

@corymhall corymhall self-assigned this Jun 10, 2024
mjeffryes
mjeffryes approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@mjeffryes mjeffryes 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 a little unclear on the context here:

We introduced a patch to fix a panic; did that patch always produce this error? or did it start producing the error after we picked up the tfbridge change that fixed the underlying cause for the panic?

flostadler
flostadler approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

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

The change lgtm.
I think for the future we need to be more careful with adding more tests for patches and following up with removing them when the underlying issue is fixed.

In this case the bridge fix was released 3 weeks ago, but we're missing some sort of automation to make us aware of this

@corymhall
Copy link
Contributor Author

We introduced a patch to fix a panic; did that patch always produce this error? or did it start producing the error after we picked up the tfbridge change that fixed the underlying cause for the panic?

I am not 100% sure. There were a couple of bug fixes in upstream that we had to revert/incorporate into our revert/patch. We had a test for the panic that was still passing, but it wasn't testing this specific use case with other properties provided. Since the linked issue says since 6.37.0 and our patch was introduced in 6.28.2 my hunch would be that the bug was introduced attempting to rebase upstream changes into our patch during an upgrade.

In this case the bridge fix was released 3 weeks ago, but we're missing some sort of automation to make us aware of this

Yeah it would be nice to have automation to gather linked issues and label them with fixed-upstream or something.

@corymhall corymhall merged commit b1eae8a into master Jun 11, 2024
25 checks passed
@corymhall corymhall deleted the corymhall/fix-getengineversion branch June 11, 2024 12:23
@corymhall
Copy link
Contributor Author

/release patch

@github-actions github-actions bot added needs-release/patch When a PR with this label merges, it initiates a release of vX.Y.Z+1 and removed needs-release/patch When a PR with this label merges, it initiates a release of vX.Y.Z+1 labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjeffryes mjeffryes mjeffryes approved these changes

@flostadler flostadler flostadler approved these changes

Assignees

@corymhall corymhall

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

As of 6.37.0 invocation of aws:rds/getEngineVersion:getEngineVersion returned an error.
3 participants
@corymhall @mjeffryes @flostadler