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

Setup databases extensions in first changelog #248

Merged
merged 3 commits into from
May 27, 2024

Conversation

ponceta
Copy link
Member

@ponceta ponceta commented May 27, 2024

  • Setup databases extensions in first changelog

Should fix #247

@ponceta ponceta self-assigned this May 27, 2024
@ponceta ponceta added datamodel Concerns the datamodel CI/CD continuous integration/deployment fix Fixing something not working labels May 27, 2024
@ponceta ponceta added this to the TEKSI Wastewater 2024.0 milestone May 27, 2024
@ponceta ponceta requested a review from 3nids May 27, 2024 09:24
@3nids
Copy link
Contributor

3nids commented May 27, 2024

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.

@sjib
Copy link
Contributor

sjib commented May 27, 2024

@ponceta I usually also created hstore - do we need that for anything already?
https://www.postgresql.org/docs/current/hstore.html

@cymed
Copy link
Contributor

cymed commented May 27, 2024

@ponceta I usually also created hstore - do we need that for anything already? https://www.postgresql.org/docs/current/hstore.html

It is needed for the History viewer

@ponceta ponceta merged commit d8a3e3a into main May 27, 2024
7 checks passed
@ponceta ponceta deleted the poa_add_db_extensions_to_code branch May 27, 2024 11:46
@@ -1,2 +1,3 @@
CREATE EXTENSION IF NOT EXISTS postgis;
CREATE EXTENSION IF NOT EXISTS hstore;
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD continuous integration/deployment datamodel Concerns the datamodel fix Fixing something not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creation of extensions missing
4 participants