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

config: use ext and only set defaults #39

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

fenekku
Copy link
Contributor

@fenekku fenekku commented Dec 5, 2019

Step in the right direction but may be is not enough yet. If it runs after invenio-records-rest config, that wouldn't do: I think we need to have invenio-app-rdm hard set the config from this module to close that loophole.

@fenekku fenekku marked this pull request as ready for review December 6, 2019 20:45
@fenekku fenekku changed the title [WIP] config: use ext and only set defaults config: use ext and only set defaults Dec 6, 2019
@fenekku fenekku requested a review from ppanero December 6, 2019 20:45
Copy link
Member

@ppanero ppanero left a comment

Choose a reason for hiding this comment

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

It is not clear to me, now if we "setdefault" and invenio-records-rest has been already loaded, this config will no be added. This means we need an ext.py in app-rdm, and to move the config back there?

@fenekku
Copy link
Contributor Author

fenekku commented Dec 9, 2019

Yup, like I said, on its own this is not enough (although I should not have said "may" in that first sentence, but rather "will" :) )

we need to have invenio-app-rdm hard set the config from this module to close that loophole

That's the approach that I think could work best. We have invenio-rdm-records define the configuration it needs in its own config.py and set_defaults it in its own ext.py. Then, invenio-app-rdm would set invenio-rdm-records' config.py in its (invenio-app-rdm) config.py (via config entrypoint). I think this is possible because invenio-rdm-records is just a dependency (if not we will use ext.py rather than config entrypoint in invenio-app-rdm). We split definition and setting for 2 reasons:

1- so we can still keep coupled things together and, in particular,
2- test invenio-rdm-records with the correct configuration.

What do you think about that?

Copy link
Member

@ppanero ppanero left a comment

Choose a reason for hiding this comment

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

@fenekku fenekku merged commit 1b98c62 into inveniosoftware:master Dec 10, 2019
@fenekku fenekku deleted the adapting_configuration branch December 10, 2019 13:46
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