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

Implement verdi config #2354

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 14, 2018

Fixes #2208

The new command verdi config replaces the various commands under
verdi devel that were used to list, get, set and unset configuration
options, that were used to be called "properties". Since the term
"property" is a reserved keyword, it was decided to rename them to
"options", which is also the term used by "git". The interface of
verdi config also mirrors that of git config. That is to say
to get the value of an option simply call git config <option_name>
and to set it git config <option_name> <option_value>. To unset
it, the --unset flag can be used. Finally, the getting, setting
or unsetting can be applied to a certain "scope", meaning to be
configuration wide or profile specific.

To make the implementation of this command simple and clean, the
bulk of the work was in refactoring the definition, construction and
operation on the configuration of an AiiDA instance. This is now
represented by the Config class, through which these configuration
options can be set or unset as well as retrieved.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.9%) to 61.239% when pulling f46300b307c9f89a65bec6e2b5b6f95894fb3bbe on sphuber:fix_2208_verdi_config into 1e7e25e on aiidateam:provenance_redesign.

@coveralls
Copy link

coveralls commented Dec 14, 2018

Coverage Status

Coverage increased (+0.05%) to 68.182% when pulling 83b35fb on sphuber:fix_2208_verdi_config into 1e7e25e on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2208_verdi_config branch 10 times, most recently from 0c27cdb to 8cc67c6 Compare December 17, 2018 16:38
@sphuber sphuber changed the title [WIP] Implement verdi config Implement verdi config Dec 17, 2018
@sphuber sphuber force-pushed the fix_2208_verdi_config branch 3 times, most recently from 1606744 to 4b1e4ed Compare December 17, 2018 17: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.

Question: are the "mock-profile" part done in such a way that even if we run concurrently (e.g. in Jenkins) the tests both for Django and SQLAlchemy, they don't step on each other toes?

@sphuber
Copy link
Contributor Author

sphuber commented Dec 17, 2018

The mock profile function currently just generates a dummy profile, with dummy variables. The profile is not "actually" used. The only shared part might be the repository path, but since the aiida.backends.tests.utils.configuration.with_temporary_config_instance decorator creates a completely temporary directory in which everything is created, these should never overlap.

Note that currently the creation of a new "profile" is still done manually by the create config functions that are called by quicksetup and setup, i.e. they manually build a dictionary with the required keys. In a future PR this should be refactored, as the Profile class is the only one that should now about these keys.

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.

Just a couple of comments (one above, one to know if this PR changes the way the AIIDA_PATH is used). apart from this, good to go!!

@@ -355,12 +355,7 @@ Manage ``Data`` nodes.
---------------
Commands intended for developers, such as setting :doc:`config properties<properties>` and running the unit test suite.

* **delproperty**: delete a property from the configuration
Copy link
Member

Choose a reason for hiding this comment

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

Are the new command line verdi commands described? (couldn't see the change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the new command to the verdi docs

The new command `verdi config` replaces the various commands under
`verdi devel` that were used to list, get, set and unset configuration
options, that were used to be called "properties". Since the term
"property" is a reserved keyword, it was decided to rename them to
"options", which is also the term used by "git". The interface of
`verdi config` also mirrors that of `git config`. That is to say
to get the value of an option simply call `git config <option_name>`
and to set it `git config <option_name> <option_value>`. To unset
it, the `--unset` flag can be used. Finally, the getting, setting
or unsetting can be applied to a certain "scope", meaning to be
configuration wide or profile specific.

To make the implementation of this command simple and clean, the
bulk of the work was in refactoring the definition, construction and
operation on the configuration of an AiiDA instance. This is now
represented by the `Config` class, through which these configuration
options can be set or unset as well as retrieved.
@sphuber sphuber force-pushed the fix_2208_verdi_config branch from 4b1e4ed to 83b35fb Compare December 18, 2018 07:58
@sphuber
Copy link
Contributor Author

sphuber commented Dec 18, 2018

And to answer your other question, no this does not change the functioning of AIIDA_PATH, it has just been moved to aiida.manage.configuration.settings.

@sphuber sphuber merged commit 14ecf15 into aiidateam:provenance_redesign Dec 18, 2018
@sphuber sphuber deleted the fix_2208_verdi_config branch December 18, 2018 08: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