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

Drop column transport_params from the Computer database model #2946

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 28, 2019

Fixes #2927

The column was intended to contain customized parameters to be used to
configure the transport of a computer. However, these fit better in the
AuthInfo, as these settings can change per user. The design is to keep
the Computer as generic as possible as to not be user dependent, such
that its configuration can be easily shared and reused. Generic computer
configuration can go in the metadata column. User specific settings
need to go in the AuthInfo.metadata instead.

@sphuber
Copy link
Contributor Author

sphuber commented May 28, 2019

@giovannipizzi note that I built this on top of commits that are waiting review in PR #2944
The migrations here simply drop the column. Are we sure we don't need a data migration? Do we know if people had been putting data in transport_params. And if so, is there any way to deterministically migrate it? Where should it go? Computer.metadata or Authinfo.metadata, and in the case of the latter, which ones?

@coveralls
Copy link

coveralls commented May 28, 2019

Coverage Status

Coverage decreased (-0.04%) to 72.594% when pulling 62978e0 on sphuber:fix_2927_drop_computer_transport_params into 6588683 on aiidateam:develop.

@giovannipizzi
Copy link
Member

From the current code, do I understand correctly that there is no direct way for the user to set these from the CLI? If this is the case, I wouldn't be worried (are the prepend/append_text going into the metadata, right?) If instead these are settable from the command line then we must be more careful.

In the first case, if we really want to be sure, we can check if they are empty, and print them in case on file with warnings similar to when we dropped DbWorkflow etc.

The column was intended to contain customized parameters to be used to
configure the transport of a computer. However, these fit better in the
`AuthInfo`, as these settings can change per user. The design is to keep
the `Computer` as generic as possible as to not be user dependent, such
that its configuration can be easily shared and reused. Generic computer
configuration can go in the `metadata` column. User specific settings
need to go in the `AuthInfo.metadata` instead.
@sphuber sphuber force-pushed the fix_2927_drop_computer_transport_params branch from 70d026e to 62978e0 Compare May 29, 2019 07:10
@sphuber sphuber changed the title [BLOCKED]Drop column transport_params from the Computer database model Drop column transport_params from the Computer database model May 29, 2019
@sphuber
Copy link
Contributor Author

sphuber commented May 29, 2019

@giovannipizzi you are right, it was not exposed in the CLI and all relevant data was stored in the metadata. I would say we can merge it as is

@sphuber sphuber merged commit af56391 into aiidateam:develop May 29, 2019
@sphuber sphuber deleted the fix_2927_drop_computer_transport_params branch May 29, 2019 12:41
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.

Drop the transport_params of the Computer model
3 participants