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

global: remove invenio-admin #208

Merged

Conversation

utnapischtim
Copy link
Contributor

@utnapischtim utnapischtim commented Apr 2, 2024

the dependency to invenio-admin is now an optional dependency.

setup.cfg Outdated
@@ -54,10 +53,6 @@ invenio_access.system_roles =
any_user = invenio_access.permissions:any_user
authenticated_user = invenio_access.permissions:authenticated_user
system_process = invenio_access.permissions:system_process
invenio_admin.views =
Copy link
Member

Choose a reason for hiding this comment

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

These should not be removed as invenio-access is used by other invenio instances e.g cds-videos. If we remove this, upgrading in the latest invenio-access will remove the admin panel views without providing an alternative at the moment. I would wait until invenio-access provides the same implementation utilizing invenio-administration. @kpsherva wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we had a discussion with Lars in Münster about this, thats the reason why i created that PR. But your point is a valid point to not remove invenio-admin at the moment. let's wait for other opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this argument also apply to other packages?

@utnapischtim utnapischtim force-pushed the remove-invenio-admin branch 4 times, most recently from 772c1a4 to 1f03084 Compare April 2, 2024 20:55
@utnapischtim utnapischtim marked this pull request as ready for review April 11, 2024 11:04
@utnapischtim
Copy link
Contributor Author

connected to inveniosoftware/invenio-app-rdm#2628

* pytest-black -> pytest-black-ng since former is unsupported

* reorder the test_requires

* fix black formating
* this step makes the invenio-admin dependency optional and keeps a
  backward compatibility while it makes it possible to remove on other
  packages too.
@utnapischtim utnapischtim merged commit fb183bc into inveniosoftware:master Nov 7, 2024
2 checks passed
@utnapischtim utnapischtim deleted the remove-invenio-admin branch November 7, 2024 20:20
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.

3 participants