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

Adds db read-only middleware #12490

Merged
merged 3 commits into from
May 12, 2023
Merged

Adds db read-only middleware #12490

merged 3 commits into from
May 12, 2023

Conversation

abhi1693
Copy link
Member

@abhi1693 abhi1693 commented May 4, 2023

Fixes: #11233

image

@abhi1693 abhi1693 requested a review from jeremystretch May 4, 2023 20:13
@jeremystretch
Copy link
Member

I like the approach here, however I don't think it satisfies FR #11233, which specifically asks for a dedicated "read-only" mode separate from maintenance mode. There's definitely a lot of overlap between the two concepts though, and I'm not sure whether having both makes sense.

@jeremystretch
Copy link
Member

Maybe we could keep this as is, and introduce a BANNER_MAINTENANCE configuration parameter which, if set to None, would disable the maintenance banner. Seems like that would be sufficient.

@abhi1693
Copy link
Member Author

abhi1693 commented May 10, 2023

Would it not be confusing to have 2 different modes? Also, currently maintenance mode only displays a banner that does not do anything functionaly which I believe this PR fixes by putting the app in read only.

In maintenance mode, I as an admin would want to continue to do whatever I need in the background but still keep the application up in read-only which IMO is a better option.

@abhi1693
Copy link
Member Author

introduce a BANNER_MAINTENANCE configuration parameter

I think that's also a good idea. Do you want to do this implementation as a part of this PR itself?

@jeremystretch
Copy link
Member

No, I think it deserves its own FR. I'll put one in.

@jeremystretch
Copy link
Member

Opened #12554 for the BANNER_MAINTENANCE config parameter.

@abhi1693
Copy link
Member Author

@jeremystretch Should I update the error message with the BANNER_MAINTENANCE value here? That seems logical to me.

@jeremystretch
Copy link
Member

I'm not sure about that, because we can't be certain that whatever text the user configures for the banner is going to be relevant. IMO it's best to stick with a static error message that explicitly explains that write operations aren't currently supported. (If there's a maintenance banner value, they'll see it in the UI anyway.)

netbox/netbox/middleware.py Outdated Show resolved Hide resolved
@abhi1693 abhi1693 requested a review from jeremystretch May 12, 2023 14:35
@jeremystretch jeremystretch merged commit 9b80ec2 into develop May 12, 2023
@jeremystretch jeremystretch deleted the feat/11233-ro branch May 12, 2023 14:50
@jeremystretch
Copy link
Member

Thanks @abhi1693!

jsenecal pushed a commit to jsenecal/netbox that referenced this pull request May 18, 2023
* adds db read-only middleware netbox-community#11233

* fixed attribute error

* replaces getattr with get_config
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webfrontend with database in readonly mode
2 participants