-
Notifications
You must be signed in to change notification settings - Fork 918
add upgrade command #5882
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
add upgrade command #5882
Conversation
[ci skip] [skip ci]
[ci skip] [skip ci]
[ci skip] [skip ci]
[ci skip] [skip ci]
src/app/Console/Commands/Upgrade/Concerns/InteractsWithCrudControllers.php
Show resolved
Hide resolved
[ci skip] [skip ci]
Co-authored-by: Pedro Martins <[email protected]>
[ci skip] [skip ci]
tabacitu
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.
Love it, man! This will make it MUCH easier for people to upgrade to v7, FOR SURE!
I have some UX improvements I want us to make. So I've shot myself following the upgrade process here. It's a 20min video, but I really want you to see things from my perspective:
https://www.loom.com/share/5223c60e84784c88a3080f6a73f1e410
In short, I think we need to
- make it mandatory to already have Laravel 12 installed (both in
composer.jsonandcomposer.lock); - change the texts to highlight the order in which everything should be done - first run the command, then follow the upgrade guide manually;
- rephrase some texts and use fewer colors;
[ci skip] [skip ci]
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.
I am VERY happy with the upgrade command now. Great job Pedro - and good rephrasing, everything is much more user-friendly now.
Wrote a few thoughts down below.
src/app/Console/Commands/Upgrade/v7/Steps/EnsureLaravelVersionStep.php
Outdated
Show resolved
Hide resolved
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.
Here's what I got upgrading the Demo - why?
What will this do? Since theme-tabler config isn't published, should the default be that nothing is done? Alternatively, the default to be to publish the new config? Both of those are better than publishing the old config (which is what I understand saying YES will do).
Maybe we should have a multiple-option selection here?
- get new look, but don't publish anything
- get new look & make it permanent, by publishing the v7 config
- keep the old look & feel, by publishing the v6 config
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.
To publish v7 config and go back to old values, you need to already have the v7. this will be a step in the "after" upgrade command.
WHY
BEFORE - What was wrong? What was happening before this PR?
As me and @tabacitu discussed before, most if not all of the changes required to upgrade Backpack in major versions, are things like "check if you have X file published, if you do, check this", "check if you have version x/y/z if you have b/c/d".
All those tasks can be automated for 90% of the use cases.
AFTER - What is happening after this PR?
We should have a reliable "upgrade" command that checks what user needs to be aware given the upgrade guide, and we provide some automatically fixes, and sometimes when it's difficult/not possible to automate, we let the user know the next steps.
HOW
How did you achieve that, in technical terms?
Creating kind of a "upgrade framework", that would let us create "Steps" for each upgrade process. Each step can have the fix etc when applicable.
Is it a breaking change?
no