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

Make the node repository API backend agnostic #2506

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Feb 20, 2019

Fixes #2504

Initially, the only backend supported for the file repository of nodes
was the local file system. In the near future, additional backends such
as an object store or a remote file system will be implemented. For this
to work the API of the node repository will have to be adapted because
now it is geared to work with a local file system. To prevent too many
changes after the release of the beta, we already define the API here
that should be able to support the different repository backends that
will be implemented in the future.

@sphuber sphuber force-pushed the fix_2504_node_repository_api_change branch from 1d64dde to 69e946a Compare February 20, 2019 15:06
@coveralls
Copy link

coveralls commented Feb 20, 2019

Coverage Status

Coverage increased (+0.2%) to 69.715% when pulling e3e4579 on sphuber:fix_2504_node_repository_api_change into 7b1c90a on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2504_node_repository_api_change branch 9 times, most recently from 62a8da6 to 5424666 Compare February 21, 2019 10:41
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

I like it!
Only additional comment, for the RemoteData class:

@sphuber
Copy link
Contributor Author

sphuber commented Feb 22, 2019

* maybe now the 'disabling' of add_path doesn't work anymore?

actually, the add_path method no longer exists on the Node class, so there is no point in disabling it. I think we should just remove it. Currently, there exist no Node methods that operate directly on the repository, they are only exposed on the Repository class itself through the repository property. Probably we will need to expose the methods that should be available for the repository, directly on the node itself, which internally will just pipe through, because in the future changes to the repository will anyway need changes on the node instance in the database, so these have to be coordinated by the Node class and cannot be done directly on the repository. That will also make it possible again to disable certain actions for certain node subclasses.

Maybe we should already do this in this PR. Make it impossible to access the repository directly from the node class (i.e. remove the property) and only expose methods that should be callable directly on the node.

@sphuber sphuber force-pushed the fix_2504_node_repository_api_change branch 4 times, most recently from 255b94e to e3db4c2 Compare February 22, 2019 14:44
Initially, the only backend supported for the file repository of nodes
was the local file system. In the near future, additional backends such
as an object store or a remote file system will be implemented. For this
to work the API of the node repository will have to be adapted because
now it is geared to work with a local file system. To prevent too many
changes after the release of the beta, we already define the API here
that should be able to support the different repository backends that
will be implemented in the future.
@sphuber sphuber force-pushed the fix_2504_node_repository_api_change branch from e3db4c2 to e3e4579 Compare February 22, 2019 15:20
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Thanks a lot, that's really a nice step toward the repo improvement!

@sphuber sphuber merged commit f8c0191 into aiidateam:provenance_redesign Feb 22, 2019
@sphuber sphuber deleted the fix_2504_node_repository_api_change branch February 22, 2019 17:30
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