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

Soft delete #306

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Soft delete #306

wants to merge 13 commits into from

Conversation

mngan
Copy link
Contributor

@mngan mngan commented Oct 18, 2020

Fixes: #301

  • BGPGroup, InternetExchange, DirectPeeringSession, InternetPeeringSession, RoutingPolicy and Community Objects all now support soft deletion. This means that they will not be deleted in the database immediately. Instead, a deleted field will record the timestamp of the deletion, which allows for multiple version of a deleted object to exist.
  • New example templates for JUNOS and EOS using the deleted attribute to clean up existing configured objects
  • New management tool to purge soft deleted objects

Closes: #86

- Enable Soft Delete for
  - BGP Groups
  - IX
  - IX Peering Sessions
  - Direct Peering Sessions
  - Routing Policies
  - Communities
- Fix views with counts of child objects to not include deleted items
- Add manager that combines NetManager and SafeDeleteManager
- Make DirectPeeringSessions cascade delete when BGPGroup is deleted
- Add migrations
- Create SoftDeleteModel and CascadingSoftDeleteModel to abstract away the SafeDeleteModel
- Use a conditional unique constraint on the slug field to allow for deleleted objects with the same unique slug
- Inherit Meta class from Abstract Model class

*Views*
- Use Count with filter instead of Case/When to filter related deleted objects in annotate()
- Fix ReturnURLMixin to use return_url instead of default_return_url, since everything seemed to be setting return_url
- Fix ReturnURLMixin to ignore deleted objects and fall through to the default return_url
- Display validation error messages in BulkDeleteView if we fail validation
- Set model_field to None in exception handler for BulkdEditView.post() to ensure the variable is bound.
- Add Soft Delete Admin page
- Add link to admin page to menu
- Patch SafeDeleteQuerySet.delete() so that it returns the number of deleted objects (compat with standard Queryset)
- Add Soft Delete API entry point
…on template objects.

- Fix the preview for configuration templates so that it generates a context in the same format as Router.get_configuration_context()
- Add a hack in TemplateModel.to_dict() to make it work properly with the template preview.
@mngan
Copy link
Contributor Author

mngan commented Oct 18, 2020

i was just thinking i should have this be a feature you need to enable. otherwise the new functionality will potentially upset people’s existing templates.

@gmazoyer gmazoyer added the type: feature request Issue is an accepted feature request label Oct 18, 2020
@gmazoyer gmazoyer self-assigned this Oct 18, 2020
- Add retention period for soft delete, default to changelog retention period
- Make command to purge soft deleted object default use the configured retention period
@mngan
Copy link
Contributor Author

mngan commented Oct 25, 2020

Added the settings to enable the feature and configure the retention period. I'll go take a look at the docker setup to make it line up. It also looks like we have a number of variables missing in the docker environment

Base automatically changed from master to main January 16, 2021 14:25
@jamesditrapani jamesditrapani added the status: do not merge Do not merge the code specified for now label Apr 29, 2021
@jamesditrapani
Copy link
Contributor

Delayed until a further major release

@stale stale bot added the status: wontfix Not an issue, no fix to be done, no code to be changed label Jul 12, 2021
@peering-manager peering-manager deleted a comment from stale bot Jul 12, 2021
@stale stale bot removed the status: wontfix Not an issue, no fix to be done, no code to be changed label Jul 12, 2021
@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@gmazoyer
Copy link
Member

Keep for tracking purpose

@Paktosan
Copy link
Contributor

How high in the priority list is this? I think this is a quite important maintenance feature to keep routers clean.

@forkwhilefork
Copy link
Contributor

This would be great to have. It's unclear why it was never merged, but I assume it'll require more work to get ready to merge now that main has progressed so much. Can anyone confirm?

@gmazoyer
Copy link
Member

This has been delayed until we have proper heritage between models and be able to define common behaviour across the code base. This work is in progress, but takes time (a lot of time). Once ready, which should happen for 1.8, we will engage a major rewrite of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted status: do not merge Do not merge the code specified for now type: feature request Issue is an accepted feature request
Projects
None yet
5 participants