Feat: Create Plugin updates as migrations #1479
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
leventebalogh
force-pushed
the
updates-as-migrations-pt1
branch
from
6b6203b to
a0b402b
Compare
leventebalogh
added
minor
There was a problem hiding this comment.
Great work on this! Looks really great 🙌🏻 I have some thoughts follow up questions.
- Have you considered having two functions as part of the migration script. One that does the migration and another one that checks if the migration is applicable. Then in the migration manager we could check if a migration is applicable prior to running it. Benefit would be so that you can skip a migration e.g. if the current plugin doesn't have/use that part of the scaffolded plugin.
- Instead of having the version could we just have an array of migrations? Then they will automatically be ordered and you can use the function that checks if a migration is applicable to check version compatibility.
- What is the thoughts regarding applying changes to the create-plugin. Are we planning on using migrations to change the create-plugin project as well?
| content: `(This check is necessary to make sure that the updates are easy to revert and don't mess with any changes you currently have. | ||
| if (!(await isGitDirectoryClean()) && !argv.force) { | ||
| printRedBox({ | ||
| title: 'Please clean your repository working tree before updating.', |
There was a problem hiding this comment.
Nice addition to this feature!
jackw
requested review from
a team and
academo
and removed request for
a team
jackw
force-pushed
the
updates-as-migrations-pt1
branch
2 times, most recently
from
5d7dbed to
5091bfd
Compare
|
|
||
| #### How to add a migration? | ||
|
|
||
| 1. Create a new migration script file with a descriptive name (e.g. `add-webpack-profile.ts`) |
There was a problem hiding this comment.
let's make it a convention to numeric-name them too, like db migrations
There was a problem hiding this comment.
Yeah I like it to. 👍
From my experience with db migrations they tend to prefix with timestamp. Do we wanna go for that or shall we keep it simple with 000- or maybe the targeted create plugin version? Not really sure what the benefits are over keeping it simple and incrementing from 000. 🤷
| @@ -37,7 +37,7 @@ | |||
| "generate-datasource-backend": "tsc && npm run clean-generated && CREATE_PLUGIN_DEV=true node ./dist/bin/run.js --pluginName='Sample datasource' --orgName='sample-org' --pluginType='datasource' --hasBackend", | |||
| "lint": "eslint --cache ./src", | |||
| "lint:fix": "npm run lint -- --fix", | |||
| "test": "vitest", | |||
| "test": "vitest --passWithNoTests", | |||
There was a problem hiding this comment.
can we instead make it ignore the particular example test?
There was a problem hiding this comment.
Thanks for pointing this out. I should have noted that it is temporary and will be removed when we add the first migration to the codebase.
The issue isn't the example test it's that right now we have no migrations which causes the migrations.test.ts that validates migrationScript paths are valid fails with no test found in suite. I can ignore that file temporarily but it felt like the lesser of two evils to use this flag instead given what "temporary" means. 😄
| printHeader('the update command completed successfully.'); | ||
| console.log(''); | ||
| } catch (error) { | ||
| printHeader('the update command encountered an error.'); |
There was a problem hiding this comment.
add a note here to instruct the user they can run git reset hard --origin [ref] and git clean -fd (calculate the ref using git) so users don't get lost in the case of error.
Maybe even add a link to our github repository for them to report the issue?
Co-authored-by: Jack Westbrook <[email protected]> Co-authored-by: Levente Balogh <[email protected]>
jackw
force-pushed
the
updates-as-migrations-pt1
branch
from
19d9b43 to
9accd00
Compare
|
🚀 PR was released in |
grafana-plugins-platform-bot
bot
added
the
released
What this PR does / why we need it:
This PR introduces the building blocks to allow developers to write migrations, focused on a specific update task, which are run sequentially via the
create-pluginupdate command. This is hidden behind a flag--experimental-updateswhich should allow us to iterate further on this without impacting current usage.See epic for more info and design doc for even more more info.
Read more about how to add migrations in the CONTRIBUTION guide →
Which issue(s) this PR fixes:
Related #1254
📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via: