-
Notifications
You must be signed in to change notification settings - Fork 43
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
services: make commands aware of rdm version #310
Conversation
invenio_cli/commands/services.py
Outdated
), | ||
] | ||
|
||
if rdm_version[0] < 10: |
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.
if rdm_version[0] < 10: | |
if rdm_version[0] >= 10: |
?
What if we do something more structured, e.g. create a enum/map of features with version, for example (allow me pseudo-code):
versions = {
10: [custom-fields, administration],
...
}
then an helper:
get_enabled_features():
return versions[current-app-rdm version]
and then in the code we get that map and use it?
if custom-fields-in-version:
run init command
07349d2
to
64e5506
Compare
@ntarocco I've for now just parsed the app-rdm version from the pipfile (I tried with the existing configparser but it doesnt work, my guess is the double [[ ]] at the beginning) Regarding the more complex logic of commands/features per version I do agree but I think we can leave it for a different/PR issue and tackle it along with the distinction of RDM/ILS of the cli? so we unblock this on the release. I can start working on that, but I think is better on a separate PR since it will be a big change. |
64e5506
to
67283cd
Compare
67283cd
to
06946c3
Compare
Opened relevant issues:
|
I do not like this way because it requires installing
invenio-app-rdm
which technically cli should not be aware of (i.e. in containerized version).Shall we parse the
Pipfile
?