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

Raiden --showconfig flag #2737

Merged
merged 4 commits into from
Oct 10, 2018
Merged

Raiden --showconfig flag #2737

merged 4 commits into from
Oct 10, 2018

Conversation

rakanalh
Copy link
Contributor

@rakanalh rakanalh commented Oct 9, 2018

Resolves #2708

Depends on #2715

Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

Our configuration is in toml, I don't see the point of printing it out in a different format. Additionally, I don't trust yaml since it has some really weird extensions that make it a security vulnerability, from the docs:

Warning: It is not safe to call yaml.load with any data received from an untrusted source! yaml.load is as powerful as pickle.load and so may call any Python function. Check the yaml.safe_load function though.

@rakanalh
Copy link
Contributor Author

@hackaugusto the idea was that yaml displays better than JSON... and Toml looks more like .conf style with section headers which i don't like personally.

@ulope
Copy link
Collaborator

ulope commented Oct 10, 2018

While I agree that there is not really a need for using yaml, the safety argument doesn't really come into play here since we're only writing yaml.

(And generally yaml.safe_load() is the way to avoid the arbitrary code execution problem ;))

@rakanalh
Copy link
Contributor Author

Alright, i'll update it to use Toml instead.

@LefterisJP
Copy link
Contributor

@rakanalh needs rebase

@rakanalh
Copy link
Contributor Author

Rebased + Changed config dump format to Toml

constraints.txt Outdated
@@ -65,6 +65,7 @@ pystun-patched-for-raiden==0.1.0
pytest==3.8.0
pytoml==0.1.19
pytz==2018.5
pyyaml==3.13
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont need this dependency anymore

requirements.txt Outdated
@@ -22,6 +22,7 @@ py-solc
pystun-patched-for-raiden
pytest
pytoml
pyyaml
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont need this dependency anymore

print('Configuration Dump:')
dump_config(config)
dump_cmd_options(self._options)
dump_module('cettings', settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/cettings/settings/


def dump_cmd_options(options):
print(toml.dumps({
'options': _clean_non_serializables(options),
Copy link
Contributor

Choose a reason for hiding this comment

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

in the future we should definitely remove the non serialize objects from the options dictionary

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 agree... the merged splitting of CLI opened up the door way for a little bit of refactoring as i wouldn't call what i did there as a refactor of the code but rather more organization. But we should definitely look into the way we pass data around between CLI components.

[ci integration-cli]
Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

All comments where fixed, approving.

But I think the CLI interface would be better served by a command that print an usable configuration, this flag is only useful to debug the application and has very little user value.

import inspect
from enum import Enum

import toml
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have toml in requirements :( you should use pytoml

[ci integration-cli]
@rakanalh rakanalh merged commit d9c96f9 into raiden-network:master Oct 10, 2018
@rakanalh rakanalh deleted the showconfig branch October 10, 2018 14:33
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.

5 participants