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.

Sign up for GitHub

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

Jump to bottom

Restructure account commands to account subcommand #984

Merged
merged 9 commits into from
May 17, 2023
Merged

Restructure account commands to account subcommand #984

merged 9 commits into from
May 17, 2023

Conversation

ellie
Copy link
Member

@ellie ellie commented May 16, 2023

Also edit the history count trigger so it only runs on insert, and not on delete. This will make sure that deletes run super fast 🚀

@vercel
Copy link

vercel bot commented May 16, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2023 8:10pm

conradludgate
conradludgate reviewed May 17, 2023
View reviewed changes
Copy link
Collaborator

@conradludgate conradludgate left a comment

Choose a reason for hiding this comment

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

While this works as it did before, I realise that we don't count actual history delete requests here. Should we add a trigger on update? I think users might be confused if the remote count doesn't decrease when they delete

atuin-server/migrations/20230515221038_trigger-delete-only.sql Show resolved Hide resolved
@ellie ellie marked this pull request as ready for review May 17, 2023 19:58
@ellie
Copy link
Member Author

ellie commented May 17, 2023

While this works as it did before, I realise that we don't count actual history delete requests here. Should we add a trigger on update? I think users might be confused if the remote count doesn't decrease when they delete

I wish :( If we actually update the remote count when rows are soft deleted, sync won't work. It's append-only, afterall, and expects counts to always increase

@ellie
Copy link
Member Author

ellie commented May 17, 2023

We should perhaps offset the deleted items from the remote count before displaying it though

@ellie ellie requested a review from conradludgate May 17, 2023 20:09
@ellie ellie merged commit ca26383 into main May 17, 2023
9 checks passed
@ellie ellie deleted the ellie/account-management branch May 17, 2023 20:28
ealap pushed a commit to ealap/atuin that referenced this pull request May 21, 2023
@ellie @ealap
Restructure account commands to account subcommand (atuinsh#984)
25718fb 
* Stop running triggers on history delete

* Move to account management dir

* Alter trigger function to only run for inserts

* wip

* Add atuin account subcommands, and re-org delete

* Clarify docs

* Delete silly dupe migration

* Um where did this come from

* Oops, insert only plz
@razvanazamfirei razvanazamfirei mentioned this pull request May 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conradludgate conradludgate conradludgate approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ellie @conradludgate