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

Support for a peer-review workflow #11942

Closed
rmanyari opened this issue Mar 9, 2023 · 9 comments
Closed

Support for a peer-review workflow #11942

rmanyari opened this issue Mar 9, 2023 · 9 comments
Labels
complexity: high Expected to require a large amont of time and effort to implement relative to other tasks needs milestone Awaiting prioritization for inclusion with a future NetBox release status: backlog Awaiting selection for work type: feature Introduction of new functionality to the application

Comments

@rmanyari
Copy link
Contributor

rmanyari commented Mar 9, 2023

NetBox version

v3.4.5

Feature type

Data model extension

Proposed functionality

Extend the staging mechanism to support a peer-review workflow.

The idea here is to provide users with a way of proposing changes, both programmatically and through the UI, and have another team member with more elevated permissions review, approve and apply them.

To support this, we'd need changes across different components and I'd suggest to implement them parts. Phases 1 through 6 are all backend work that can be done incrementally in different MRs. Phases 7 through 9 are all UI work that I believe can be released independently.

Phase 1 - Scaffolding for notifications
  • Add a new model to represent notifications.
  • Extend API (REST and GraphQL) to read, update and delete notifications. Note that notifications are created by the system.
Phase 2 - Introduce the ReviewRequest model.
  • Introduce a new model that represents a ReviewRequest. At a minimum this should be associated to a Branch
  • Extend the API (REST and GraphQL) to CRUD ReviewRequest
Phase 3 - Enhance Branch to identify stale changes.
  • Enhance the apply function of the Branch model to support applying a change on a stale model. To keep things simple, reject stale changes.
Phase 4 - Extend the permissions model to support the creation of review requests by non power users.
  • Add a new permission type to give users the capability to create review requests.
Phase 5 - Extend the ObjectChange model to support review/reviewer information.
  • Update the model to include review/reviewer information if applicable.
Phase 6 - E2E test suite to go through the peer-review workflow programmatically
  • At this stage of the implementation, we should have all the backend components to test the workflow end-to-end. Implement a test suite that describes the canonical way of using these new APIs and demonstrates the workflow.
Phase 7 - Update the UI to support notifications.
  • Add bell style notifications in the top bar.
  • Add view to show all notifications, and support actions such as mark as read, unread and delete.
Phase 8 - Update the UI to support review requests.
  • Add new button type (something analog to the edit button) to support creating a review request.
  • Add view to read, diff, update and delete review requests.
Phase 9 - Update the change log in the UI.
  • Update the UI include reviewer/review information (if applicable).

Use case

Netbox users with create/edit/delete permissions can independently change the state of an instance.

While convenient from a velocity point of view, the current model doesn't support organizations with strict change management processes that require all production changes to be peer-reviewed. This feature request aim to add that functionality to Netbox.

Database changes

New Notifications model (minimum fields required):
  • pk
  • user (fk to user)
  • title
  • content
  • read flag
New ReviewRequest model (minimum fields required):
  • pk
  • owner (fk to user)
  • reviewer (fk to user)
  • branch
  • state (in review, approved, denied)
  • status (open, closed)
Update ObjectChange model (add optional field):
  • Review Request (fk to ReviewRequest)

External dependencies

None that I can think of.

@rmanyari rmanyari added the type: feature Introduction of new functionality to the application label Mar 9, 2023
@jsenecal
Copy link
Contributor

jsenecal commented Mar 9, 2023

Very well written and thought of FR. Much appreciated.

The only concern we have is that this is in fact two features in one request : Notifications and Review functionalities.

We could decouple them, or handle them together as the notifications system wouldn't serve much use in other models anyways. (Report/Script completion notifications maybe?)

Marking this as under review for the time being.

@jsenecal jsenecal added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Mar 9, 2023
@rmanyari
Copy link
Contributor Author

rmanyari commented Mar 9, 2023

As discussed in Slack, I'm more than happy to take a shot at implementing this. I'll work on a PoC.

@jsenecal
Copy link
Contributor

jsenecal commented Mar 9, 2023

Great! Thanks for contributing 🥇

@baldgeek
Copy link

Can I ask what under review means for this moving forward? My company was very excited about this, and it appeared he was making progress before that PR was closed. It has also garnered the most likes out of the "under review" category (my company is only one of those).

Thanks.

@jeremystretch
Copy link
Member

@baldgeek you can find descriptions of all our issue labels here. The under review status means:

Further discussion is needed to determine this issue's scope and/or implementation

It will be some time before we can dig into this FR as we're currently focusing on the upcoming v3.5 release. If you'd like to help us reduce the backlog of open issues in the meantime, I would greatly appreciate volunteers for any of the open issues marked needs owner. These can be tackled immediately, and completing them will help to free up maintainer time to consider feature proposals like this one.

@rmanyari
Copy link
Contributor Author

For context @baldgeek - It was my mistake to open a PR here. What I meant to do was work on a PoC on my side to discuss the approach with some of the maintainers and take it from there. I'm glad to see that there's support for these type of features from the community, and once the maintainers prioritize it and flesh out a design, I'm happy to keep contributing as needed.

@jeremystretch jeremystretch added needs milestone Awaiting prioritization for inclusion with a future NetBox release and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Apr 5, 2023
@tobikris
Copy link

tobikris commented May 8, 2023

Thank you for working on this feature, I am looking forward to it!
I have a use case where it is not clear to me if this would already be included in your feature:
Several changes on different objects related to the same "changeset" (e.g. a new device with its IP addresses and cables to other devices). I would like to create a review request for the full change instead of several requests per object. Is this your understanding as well?

@tom0010
Copy link

tom0010 commented Mar 1, 2024

I don't see any updates on here, is this something that could be thought about for 4.0 release?

@jeremystretch jeremystretch added status: backlog Awaiting selection for work complexity: high Expected to require a large amont of time and effort to implement relative to other tasks labels May 21, 2024
@jeremystretch
Copy link
Member

This use case has largely been addressed by the recently released netbox-branching plugin, and user notifications were implemented natively in NetBox v4.1 (see #15621). NetBox Labs is also developing a separate plugin to implement a formal review process for change requests.

Please also note that NetBox's legacy built-in staged changes API is being deprecated in v4.2 (see #17472).

@jeremystretch jeremystretch closed this as not planned Won't fix, can't repro, duplicate, stale Sep 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
complexity: high Expected to require a large amont of time and effort to implement relative to other tasks needs milestone Awaiting prioritization for inclusion with a future NetBox release status: backlog Awaiting selection for work type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants