-
Notifications
You must be signed in to change notification settings - Fork 6
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
Setup databases extensions in first changelog #248
Conversation
for more information, see https://pre-commit.ci
I'm not sure if that should be part of the changelogs, that's more part of the database setup and the requirements. But looking at the dumps, the cenxtensions creations are part of it, so...it sounds ok. |
@ponceta I usually also created |
It is needed for the History viewer |
@@ -1,2 +1,3 @@ | |||
CREATE EXTENSION IF NOT EXISTS postgis; | |||
CREATE EXTENSION IF NOT EXISTS hstore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The audit is not currently bundled. I would recommend to revisit this before adding this extension. Jon(b) is generally preferred over hstore nowadays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropping hstore requires changes in the history viewer plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I am just saying that currently there is nothing in this repo related to the history viewer. It's up to the manager to add this extension, it should not be integrated by default if not use by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was in qgep and I don't see why we should now suddenly drop it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#249 I changed the extensions here and added the history viewer needs in a separate PR.
We can always switch to jsonb when we implement it in history viewer.
This is a 1-1 functionnality with QGEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I make myself clear.
I approved the PR before the addition of the hstore extension, added after and merged.
I think it is a good practice to prefer dedicated PRs for each change.
We had former discussions with Arnaud where I think we decided to have this audit functionality apart from the definition of the model. To my opinion, this should be at least optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #249 for later comments.
Should fix #247