Skip to content
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

FEATURE: cr:list command #4900

Open
wants to merge 4 commits into
base: 9.0
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Feb 19, 2024

image

advanced / verbose output (using internal stuff)

image

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign force-pushed the feature/crListCommand branch from 26fff3a to 4cb1889 Compare June 22, 2024 09:56
@mhsdesign mhsdesign changed the title WIP: cr:list command FEATURE: cr:list command Aug 30, 2024
@mhsdesign mhsdesign marked this pull request as ready for review August 30, 2024 10:18
Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly more changes than indicated in the title. 🤨

@crydotsnake crydotsnake self-requested a review August 30, 2024 11:45
@@ -33,6 +33,11 @@ class ContentStreamCommandController extends CommandController
*/
public function pruneCommand(string $contentRepository = 'default', bool $removeTemporary = false): void
{
if (!$this->output->askConfirmation(sprintf('> This operation in "%s" cannot be reverted. Are you sure to proceed? (y/n) ', $contentRepository), false)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhsdesign What was the intention on adding this unskippable confirmation? I remember using the pruneCommand and pruneRemovedFromEventStreamCommand regularly to cleanup the database.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a safety measurement we already have in place in other locations but forgot about this one ... like cr:prune and cr:projectionReplay etc

Copy link
Member

@ahaeslich ahaeslich Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. But do we really assume that this commands will only ever have the need to be triggered manually? Cleaning up the contentstream tables of junk data is a bit of a different use case then cr:prune and cr:projectionReplay.

And I'm worried about adding this change now in a feature PR without addressing it. This might be a change someone running an active Neos 9 project would like to see in the Logs.

Copy link
Member

@nezaniel nezaniel Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho this must be able to be part of a deployment process, so at least a confirmation parameter must be added to the CLI call.

@mhsdesign
Copy link
Member Author

future idea from christian: show also if there is a mismatch between actual dimensions and configured ones ... that will make this clearly a cr:debug command rather

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants