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

feat: implement FOREST_DISABLE_AUTO_SCHEMA_UPDATE and test for it #138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gonzalomolbar
Copy link

@gonzalomolbar gonzalomolbar commented Jul 19, 2023

Definition of Done

  • 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
  • It only applies in the scenario where the file is written, i.e. if DEBUG = true
  • Wrote test for it, following standards
    • Corrected a few usages of self.assertRaises in other tests in the file wich were not being used properly

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

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@jeffladiray
Copy link
Member

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 🙏

@gonzalomolbar
Copy link
Author

gonzalomolbar commented Jul 20, 2023

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
Copy link
Member

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
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants