ForestAdmin / django-forestadmin

🐍 Django agent for Forest Admin to integrate directly to your existing Django backend application.
https://www.forestadmin.com
GNU General Public License v3.0
123 stars 18 forks source link

feat: implement FOREST_DISABLE_AUTO_SCHEMA_UPDATE and test for it #138

Open gonzalomolbar opened 1 year ago

gonzalomolbar commented 1 year ago

Definition of Done

Note

I didn't add anything in the changelog or versioning because I'm less familiar with it, and I'm not sure if you're the ones to add those changes when releasing it! So let me know if I should add anything

General

Security

jeffladiray commented 1 year ago

Hey @gonzalomolbar, and thanks for your contribution.

Added support for a new optional setting FOREST_DISABLE_AUTO_SCHEMA_UPDATE which, if true, will stop the forestadmin-schema.json to be overwritten but still generating schema data for the app to function properly

This is definitely something we tend to avoid. Not generating the schema could lead to important layout generation and permissions issues, and even outage in the usage of ForestAdmin, especially when using Developer Workflow/Production setup.

Still, I'm curious to understand the underlying issue you are having with the fact that .forestadmin-schema is regenerated - as it is supposed to change only when something important was changed on your end regarding your database and/or your setup. If that not your case and you are encountering conflicts in terms of schema generation, could you please share some example of conflicts you are having ?

Thanks in advance :pray:

gonzalomolbar commented 1 year ago

Hi @jeffladiray! No problem at all.

This is definitely something we tend to avoid. Not generating the schema could lead to important layout generation and permissions issues, and even outage in the usage of ForestAdmin, especially when using Developer Workflow/Production setup.

Mmm curious about this. It feels like it would have the same consequences as having the FOREST_DISABLE_AUTO_SCHEMA_APPLY as True, isn't it? With the difference of that the local file wouldn't be modified, right? If the forest schema is not applied (which u could do manually, true) the same layout generation and permission issues could happen, isn't it?

I guess the difference is that there is an actual command of the forest cli that can apply it (superseeding the setting) and not one for updating the schema file? Mmm. Not a big problem for us, since it's still an optional behaviour

Still, I'm curious to understand the underlying issue you are having with the fact that .forestadmin-schema is regenerated - as it is supposed to change only when something important was changed on your end regarding your database and/or your setup. If that not your case and you are encountering conflicts in terms of schema generation, could you please share some example of conflicts you are having ?

It's just to keep a tighter control of the changes done to forest. Not every engineer that works in our org works on / is aware of how forest work. So even tho they do DB and model changes we prefer to have it changed in the schema only by the people that does. Also sometimes we've found actual git conflicts from 2 people updating something that is close in the forest schema, which had to be solved. Not a big deal but still

Thanks for taking a look! let me know if it makes sense the setting proposal or not :P happy to do any changes to make it work if necessary

jeffladiray commented 1 year ago

Mmm curious about this. It feels like it would have the same consequences as having the FOREST_DISABLE_AUTO_SCHEMA_APPLY as True, isn't it? With the difference of that the local file wouldn't be modified, right? If the forest schema is not applied (which u could do manually, true) the same layout generation and permission issues could happen, isn't it?

Well, you are totally right (And I'm not a big fan of FOREST_DISABLE_AUTO_SCHEMA_APPLY either). Usually it's a bit less of a "bad" thing than not generating the schema at all (As removing the variable and restart your server in production is enough to fix this bad state it generate).

All your frontend/layout configuration on ForestAdmin depend on the forestadmin-schema file.

[...]. So even tho they do DB and model changes we prefer to have it changed in the schema only by the people that does.

To give you an example of possible bad state, let's say you remove a column "foo" from table "Bar" in your business code. "Bar"."foo" is still referenced in your "forestadmin-schema" if not regenerated & sent to our servers, thus, every access to "Bar" on your admin panel will fail.

This is only one example, but there are a lot of others various things that could lead to this kind of issue.

Also sometimes we've found actual git conflicts from 2 people updating something that is close in the forest schema, which had to be solved. Not a big deal but still.

Dropping the schema file & restarting the agent should be enough to cover this case, with no conflict handling needed - but still, I understand the frustration here.

I'm pretty sure I'm missing something for your specific use-case though, or maybe that's related to some specifity in your development setup that I'm missing. We're still discussing your contirbution internally though, (As this would also need to be backported on all our agent if we were to validate this feature).

In the meantime, and if this feature is a blocker for you, I'm pretty sure we can come up with a similar variable to force reset the schema file when you don't want it to be updated.

Sorry for the long back and forth, but be sure you'll be updated once we've got some news to share.

gonzalomolbar commented 1 year ago

To give you an example of possible bad state, let's say you remove a column "foo" from table "Bar" in your business code. "Bar"."foo" is still referenced in your "forestadmin-schema" if not regenerated & sent to our servers, thus, every access to "Bar" on your admin panel will fail.

Yeah I was guessing the bad state would be somewhat around that direction. I've seen it happen by not applying it indeed, tho as u mention is kind of easier to fix that state in production and any non debug environments!

Dropping the schema file & restarting the agent should be enough to cover this case, with no conflict handling needed

Oh yeah that,s personally what I did! No way you could sometimes figure out correctly what should go in or not in the conflicts!

So indeed, I'd say it's not a big blocker tbh. If it's possible to have it in, I can see it as a weird but optional setting for some setups, but I'd understand if it's not wanted!

Will keep an eye on the PR in case u have any update, but thanks for your insight!