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

cli for schema generator should use orm config more #2912

Closed
wants to merge 2 commits into from

Conversation

m11m
Copy link

@m11m m11m commented Mar 14, 2022

There are a number of issues with postgres and session_replication_role

I think I found another one here, I could not use the recommended fixes (add disableForeignKeys to orm config) with success.

In my case, I needed the following code fix, then I could run cli command like schema:create without the --fk-checks cli flag and it would use the config specified in orm configuration like:

{
  ...
  schemaGenerator: {
    disableForeignKeys: false,
  }
}

We can see why we might want this to be 'tri state' at the place I made the change by looking what happens down in the schema generator

We should probably be able to fallback to orm configuration (as suggested in related issues) when we don't add --fk-checks to cli
Comment on lines +100 to +101
let params = args.fkChecks !== undefined ? {wrap: !args.fkChecks} : {};
params = {...params, ...args};
Copy link
Member

@B4nan B4nan Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont understand how this can fix anything, it should do exactly the same thing is the previous version, but more verbose/less readable.

more importantly, if there is a bug, we need a test case

edit: ok i get it, if there is a value, we ignore the global config. i should have renamed the wrap thing, its confusing on its own...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I apologize for not knowing about the rules, such as needing a test case. I don't know much about this project and was only trying to learn a little about it when I found the issue.

@B4nan B4nan closed this in f1b8e46 Mar 19, 2022
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.

None yet

2 participants