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

Check for unknown config options #335

Merged
merged 1 commit into from Sep 7, 2016

Conversation

meejah
Copy link
Contributor

@meejah meejah commented Sep 5, 2016

The list of valid sections + config-items came from grep'ing the source for .get_config

@coveralls
Copy link
Collaborator

coveralls commented Sep 6, 2016

Coverage Status

Coverage decreased (-0.008%) to 92.237% when pulling e2a34f0 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

@warner
Copy link
Member

warner commented Sep 6, 2016

I like it, but I think we need to make some changes before landing:

  • Node is shared between Client and IntroducerNode, and the introducer uses a subset of the config options (really just the [node] section, I think), so we need separate validation rules for each. So I'm thinking that _validate_config() still lives on the Node class, but it accepts the what-is-legal dict as an argument. CONFIG_SECTIONS gets moved out to a top-level definition (not a class attribute) in client.py and introducer/server.py (with separate values in each), and the call to self._validate_config(CONFIG_SECTIONS) moves into Client.__init__ and IntroducerNode.__init__.
  • tests! The approach in test_node.ClientNotListening should work pretty well: it makes a tempdir, writes a tahoe.cfg, then instantiates a Client and sees if it explodes or not. self.mktemp() is probably better for the tempdir than what I've currently got in there. I'd actually consider making _validate_config() a standalone function (instead of a method), so you could test it by making a config object from the file and then submitting it to the function (instead of instantiating a whole Client or IntroducerNode). My goal here is to run the tests quickly, and instantiating a whole Node means it must create a new Tub, with a new RSA key, and really all we care about is the config file. Another option is to make a FakeClient subclass of Client with a __init__ that strips out everything but calling the load-config/validate-config sections.

@meejah
Copy link
Contributor Author

meejah commented Sep 6, 2016

@warner does the latest commit address your code-location etc concerns? I still need to change the lists of valid config items for client vs introducer so not quite ready for merge

@meejah
Copy link
Contributor Author

meejah commented Sep 6, 2016

Okay so b4282b6 is my stab at the minimum list of things the introducer needs (based on searching the code).

@coveralls
Copy link
Collaborator

Coverage Status

Coverage increased (+0.01%) to 92.257% when pulling b4282b6 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

1 similar comment
@coveralls
Copy link
Collaborator

Coverage Status

Coverage increased (+0.01%) to 92.257% when pulling b4282b6 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.007%) to 92.238% when pulling b4282b6 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

1 similar comment
@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.007%) to 92.238% when pulling b4282b6 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

@coveralls
Copy link
Collaborator

coveralls commented Sep 6, 2016

Coverage Status

Coverage increased (+0.02%) to 92.262% when pulling 1d34de0 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

The list of valid sections + config-items came from
grep'ing the source for `.get_config`
@meejah
Copy link
Contributor Author

meejah commented Sep 6, 2016

I squashed everything, should be good-to-go incorporating our discussion.

@coveralls
Copy link
Collaborator

coveralls commented Sep 6, 2016

Coverage Status

Coverage decreased (-0.01%) to 92.236% when pulling 2732c37 on meejah:2809.unknown-configs.0 into 00ea41e on tahoe-lafs:master.

@warner
Copy link
Member

warner commented Sep 7, 2016

Looks good. I think I might add a test to feed the tahoe create-client default config through the validator (seems like a good sanity check), and also update the current tests to make sure the error message includes the right information about which key had a problem. But I'll merge this first.

@warner warner merged commit 2732c37 into tahoe-lafs:master Sep 7, 2016
@meejah meejah deleted the 2809.unknown-configs.0 branch September 8, 2016 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants