-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 error when adding revisions to existing APIs #22380
Conversation
I noticed the checks were cancelled for this PR, when would be a good time to have them run? |
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.
@kevjallen Thanks for this PR. I've re-triggered the checks, they probably failed due to an Actions outage.
I'm attempting to check out your changes locally to review them, but it looks like the source branch has patched some of the imports? I'm not sure why these aren't showing up in the PR diff, but I'm not confident to merge this - would you be able to commit these changes to a separate branch rather than your fork's main branch, and re-open the PR from that separate branch? Ideally if you can rather use a personal fork rather than an organization fork, this will help us to potentially push changes to the PR branch as needed. Thanks!
@manicminer Hi, thanks for the response! The repo I am trying to merge from is called Please let me know if that is acceptable, thanks |
Aha, so it is! It'd be great if GitHub surfaced this detail without making you click through on the diff page 😅 I'll take another look today, thanks! |
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.
@kevjallen This is mostly looking good. Unfortunately, changing the ID means that we'll need a state migration here.
This involves bumping the SchemaVersion
and configuring a StateUpgrader
for the Resource
struct instance. You can find straightforward examples of this in the various record resources in the dns
package, and a reference guide in the plugin SDK documentation.
Please reach out if you need any further pointers!
@manicminer Please review the state migration when you have a moment, thanks! |
I believe this is ready for another review, the changes from the SDK migration were incorporated here |
Apologies for the delayed response @kevjallen. We've been fixing some broken APIM state-migrations that recently got into the provider. In the process of this we've opened #23031 which incorporates your changes to address one of the fixes we believe is required to bring these into a workable state again. Before merging #23031 we need an internal discussion to resolve concerns we have around a State-migrations become a permanent component of a resource and need to be treated with appropriate caution. For this reason, as well as to avoid duplicate PRs, I'm going to close this PR in favour of #23031. Thank you so much for your time and effort on this! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
fix for the issues described here:
#12720
#18093
sample terraform code:
trying to create a revision of an existing API using the above code currently results in this error: