-
Notifications
You must be signed in to change notification settings - Fork 913
[MAINT] Convert legacy Schema Migrations to use StateUpgraders #3065
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
stevehipwell
left a comment
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.
Could we move the migration code into the resource files instead of having it in separate files which makes it significantly harder to reason about?
We can definitely do that - I'd recommend that we follow up with changing the existing ones as well so that we have a consistent pattern. |
|
@nickfloyd I think this PR is modifying all of the existing ones, so I was meaning for all migrations to be located next to the relevant schemas. |
|
@stevehipwell That makes so much more sense... thank you for the clarity. |
…ers` for schema migrations Signed-off-by: Timo Sand <[email protected]>
…ema migrations Signed-off-by: Timo Sand <[email protected]>
…tionWebhook` schema migrations to use StateUpgraders Signed-off-by: Timo Sand <[email protected]>
…ders Signed-off-by: Timo Sand <[email protected]>
|
@nickfloyd @stevehipwell I'm honestly not sure about merging the migration files with the resource files Some of these migration files are very long and since they contain Schemas at specific points in time it could make it confusing to have multiple schema in the same file. Or would we keep the schemas still in separate files and only move the state upgrade functions to the resource files? |
|
What about adding schema files with the same name as the primary files and then adding a schema suffix so they're at least located next to each other? For example github_resource_foo.go & github_resource_foo_schema.go |
Signed-off-by: Timo Sand <[email protected]>
|
@stevehipwell I'm wondering if this would be a good move for 7.x. I really like the idea of a related grouping so for instance our protocol would be:
So, given something like:
Is that what you were thinking? If so I can write up an issue for us to rework all of that. |
6fc64f6 to
710d9b8
Compare
|
@stevehipwell Something like this? |
|
@deiga do you mean the code or file paths? @nickfloyd for v7 I'd suggest aligning to a more idiomatic approach like below. But for now I'd really like to see the migration code moved as it's all being modified.
|
|
@stevehipwell Both? Just wanted to double check if this close enough to what you had in mind, before I do the same change for every migration file |
I was on my phone so it was hard to figure out what had changed. OK, how about github/resource_github_actions_secret_migration.go (& github/resource_github_actions_secret_migration_test.go) with the migration functions and the schemas in the one file? I think this will meet my desire for keeping the code together without adding additional complexity to the main files. Also after thinking it through carefully I realized that once the migration has happened the code is effectively meaningless so should be kept out of the way for general readability. |
Supersedes #3018
Before the change?
MigrateStatewhich is from 0.11 of the SDKgithub_actions_organization_secretgithub_actions_secretgithub_repository_webhookgithub_repositoryAfter the change?
StateUpgraderswhich is the default since 0.12 of the SDKPull request checklist
Schema migrations have been created if needed (example)Does this introduce a breaking change?
Please see our docs on breaking changes to help!