ngclient constants and configuration #1470
Conversation
There was a problem hiding this comment.
The RequestsFetcher changes do not belong here IMO: RequestsFetcher is only one fetcher implementation:
- If we think these values must be supported by every implementation we should make that part of the interface, not sneak them in like this
- If these are just RequestsFetcher config, then they shouldn't be in UpdaterConfig
I would just make the requestsfetcher configuration instance attributes of RequestsFetcher: The user can always instantiate one and then modify the attributes before giving the fetcher to updater.
tuf/ngclient/config.py
Outdated
| MAX_ROOT_ROTATIONS: int = 32 | ||
| MAX_DELEGATIONS: int = 32 | ||
| DEFAULT_ROOT_MAX_LENGTH: int = 512000 # bytes | ||
| DEFAULT_TIMESTAMP_MAX_LENGTH: int = 16384 # bytes | ||
| DEFAULT_SNAPSHOT_MAX_LENGTH: int = 2000000 # bytes | ||
| DEFAULT_TARGETS_MAX_LENGTH: int = 5000000 # bytes |
There was a problem hiding this comment.
since these are now not intended to be constants or scary we can lowercase them... that removes a pylint disable as well
There was a problem hiding this comment.
yes, I did and and I think it even looks better f5b61ec Thanks for the proposal!
tuf/ngclient/updater.py
Outdated
| if config is None: | ||
| self.config = UpdaterConfig() | ||
| else: | ||
| self.config = config |
There was a problem hiding this comment.
| if config is None: | |
| self.config = UpdaterConfig() | |
| else: | |
| self.config = config | |
| self.config = config or UpdaterConfig() |
should work in this case?
Add a config module containing a dataclass UpdaterConfig with all client settings. Initialize updater with default settings if no other condig is provided. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
sechkova
force-pushed
the
ngclient-settings
branch
from
db82b29
to
002a37a
Compare
|
Rebased on |
sechkova
changed the title
There was a problem hiding this comment.
Looks good. I'm not super into adding optional things into the constructors... but if others like the config object pattern I'm OK with it.
We should get rid of sleep_before_round: if we're "hogging CPU" as the comment says, let's fix that instead. I don't think we are though: I'm sure requests does not require us to poll constantly. Can you file an issue on that?
| @@ -47,6 +48,11 @@ def __init__(self): | |||
| # Some cookies may not be HTTP-safe. | |||
| self._sessions = {} | |||
|
|
|||
| # Default settings | |||
| self.sleep_before_round: int = 4 # seconds | |||
There was a problem hiding this comment.
er, I think you mixed up sleep_before_round and socket_timeout: this is going to make life painful
tuf/ngclient/config.py
Outdated
| default_root_max_length: int = 512000 # bytes | ||
| default_timestamp_max_length: int = 16384 # bytes | ||
| default_snapshot_max_length: int = 2000000 # bytes | ||
| default_targets_max_length: int = 5000000 # bytes |
There was a problem hiding this comment.
None one of the variables should be called "default" i think -- looks a bit odd in the code since they might be set by the user and they're still called "default"?
There was a problem hiding this comment.
Maybe default makes sense for snapshot and targets where this is the value used if the one in metadata is not se but I renamed all anyway 520b344.
Since configuration constants are now part of a config class, make them lower case. This also avoids pylint's invalid-name error for class attributes. Remove the 'default' prefix as they are now configurable options. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Stop using the settings module and add the RequestsFetcher specific config as instance attributes. Users of the requests-based fetcher implementation can modify them after instantiating a RequestsFetcher object if needed. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
sechkova
force-pushed
the
ngclient-settings
branch
from
002a37a
to
37defa9
Compare
Fixes #1402
Description of the changes being introduced by the pull request:
Adds a new public config module
ngclient/config.py.UpdaterConfigwith all client settingsPlease verify and check that the pull request fulfills the following
requirements: