-
Notifications
You must be signed in to change notification settings - Fork 150
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
config: granular env-based solution for connection strings #2918
base: master
Are you sure you want to change the base?
Conversation
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.
85be1aa
to
fb44dbe
Compare
Hey @Samk13, would this be better inside Invenio-Config? |
@egabancho, thanks for the feedback! I raised the same question in the PR description. If the core team agrees as well, I'm happy to refactor and move the logic to Invenio-Config. 👍 |
5ebe7db
to
c4e3c53
Compare
@egabancho The logic has been moved to invenio-config:57, once merged, the tests should pass here. |
c4e3c53
to
d12d53a
Compare
use invenio-config util to build connection string partially closes: inveniosoftware/helm-invenio#112
d12d53a
to
e0a468e
Compare
❤️ Thank you for your contribution!
Description
Partially closes: inveniosoftware/helm-invenio#112
Depends on: inveniosoftware/invenio-config#57
Questions:
Should the logic remain in this package, or should it be moved to another one e.g.:invenio_app
orinvenio_config
and imported here?The logic is been moved to invenio-config: inveniosoftware/invenio-config#57
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: