-
Notifications
You must be signed in to change notification settings - Fork 7
Update command #272
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
Update command #272
Conversation
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 think we need to iterate a little on the app:update command. I would expect it to just update everything (migrations and templates). How many confirmations we prompt for i don't know. Could be just at first, could be individually for migrations and for templates.
See https://github.com/kimai/kimai/blob/main/src/Command/InstallCommand.php for inspiration
src/Command/TemplatesListCommand.php
Outdated
| } else { | ||
| $io->warning($text); | ||
|
|
||
| return Command::FAILURE; |
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.
Semantics, but not sure I think a status can fail. Even warning might be excessive. I would prefer that we either:
- Keep the
--statusoption but change toinfo/success - Change to
--validate-installand usewarning/error
src/Command/UpdateCommand.php
Outdated
| // TODO: Update existing templates. | ||
| protected function configure(): void | ||
| { | ||
| $this->addOption('force', 'f', InputOption::VALUE_NONE, 'Update all requirements. Otherwise, update command only checks requirements.'); |
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 don't like the naming combination here. --force means something else for me. If we need a status command I would make a dedicated app:status.
For aap:update we could follow doctrine migrations:
- no option: Prompt for "are yoy sure?"
--no-interaction: just run
src/Command/UpdateCommand.php
Outdated
| '--update' => true, | ||
| ]); | ||
| $migrationsCommand->setInteractive(false); | ||
| $application->doRun($migrationsCommand, $output); |
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.
If the called command fail we should also fail.
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.
Approved with one minor suggestion
src/Command/UpdateCommand.php
Outdated
| // If no installed templates, we assume that this is a new installation and offer to install all templates. | ||
| if ($isInteractive && 0 === count($installedTemplates)) { | ||
| $question = new Question('No templates are installed. Install all '.count($allTemplates).'?', 'yes'); | ||
| $question->setAutocompleterValues(['yes', 'no']); |
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.
Symfony has a ConfirmationQuestion https://symfony.com/doc/current/components/console/helpers/questionhelper.html#asking-the-user-for-confirmation
Link to issue
#249
Description
Added update command.
Checklist