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.

Sign up for GitHub

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

Jump to bottom

Client refactor: code reformatting #1341

Merged
merged 11 commits into from
Apr 14, 2021
Merged

Client refactor: code reformatting #1341

merged 11 commits into from
Apr 14, 2021

Conversation

sechkova
Copy link
Contributor

@sechkova sechkova commented Apr 8, 2021

Description of the changes being introduced by the pull request:

  • Move file download related modules inside client directory
  • Remove (un)safe_download functions
  • Apply black and isort to the newly moved files
  • Apply the new pylint config from api/pylintrc and fix the various linter issues
  • Remove the use of six and future and make the rest of the imports vendoring-compatible

Please verify and check that the pull request fulfills the following
requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@sechkova sechkova added the experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch) label Apr 8, 2021
@sechkova sechkova requested review from jku and removed request for jku April 8, 2021 11:59
jku
jku requested changes Apr 14, 2021
View reviewed changes
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Thanks, looks mostly good to me:

  • disagree with logging-not-lazy (we can discuss though)
  • The *_target_download() functions still make me itch -- whatever we do here let's next concentrate on getting those fixed

tuf/client_rework/updater_rework.py Outdated
Comment on lines 151 to 155
for temp_obj in mirrors._mirror_target_download(
target, self._mirrors, self._fetcher
):

try:
Copy link
Member

Choose a reason for hiding this comment

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

commit message says it only moves code but this obviously changes things... We already have an issue on figuring out the download methods design (and fixing the bugs it has with multiple mirrors) so I'm not expecting this commit to fix everything -- but I also don't understand what this change does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, it was a mistake after cherry-picking this commit from another branch.
I had to rebase and force push to fix this. This is the new commit: 35587c4

tuf/client_rework/download.py Outdated
Comment on lines 102 to 103
msg = f"Downloading: {url}"
logger.info(msg)
Copy link
Member

Choose a reason for hiding this comment

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

I disagree with the whole logging-not-lazy commit: Working around warning like this is not useful.

  • if we want to use f-strings with logging, let's silence the warning in the linter config
  • alternatively, let's just use %-style strings (I lean this way)

tuf/client_rework/mirrors.py Outdated
@@ -110,7 +99,7 @@ def get_list_of_mirrors(file_type, file_path, mirrors_dict):
in_confined_directory = securesystemslib.util.file_in_confined_directories

list_of_mirrors = []
for dummy, mirror_info in six.iteritems(mirrors_dict):
for dummy, mirror_info in mirrors_dict.items():
Copy link
Member

Choose a reason for hiding this comment

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

nit: dummy is not even required for mirror_info in mirrors_dict.values()

tuf/client_rework/download.py Outdated
import logging
import tempfile
import timeit
import urllib
Copy link
Member

Choose a reason for hiding this comment

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

from urllib import parse maybe -- and then use parse.unquote() etc. Current code only works by accident since some other module happens to import the right thing

@sechkova sechkova added this to the Experimental client milestone Apr 14, 2021
@sechkova
Move download modules inside client directory
35587c4 
The modules performing network download are used only by
the client side of TUF. Move them inside the client
directory for the refactored client.

Move the _mirror_*download functions from Updater to
mirrors.py.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Remove (un)safe_download functions
81747ed 
The two functions safe/unsafe_download differ only by setting
a single boolean flag. Remove them and call directly
_download_file instead.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Reformat client code
3f89c01 
Run black and isort over the old modules which were
moved inside the client directory:
- download.py
- fetcher.py
- mirrors.py
- requests_fetcher.py

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Fix pylint C0301: Line too long
1c6ab32 
Fix line lenght exceeding 80 characters since the
black formatter does not wrap lines inside comments.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Fix pylint W0212: protected-access
575c15b 
Fix "Access to a protected member _mirror_target_download,
_mirror_meta_download of a client class"

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Fix pylint C0103 and W0612
7a717f6 
- fix C0103 invalid argument name STRICT_REQUIRED_LENGTH
- use 'dummy' as an accepted by pylint unused variable
name (W0612)

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Fix pylint R1720: Unnecessary "else" after "raise"
1d2721d 
Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Fix pylint W0703: broad-except
aaedcfe 
pylint cannot figure out that we store the exceptions
in a dictionary to raise them later so we disable
the warning. This should be reviewed in the future still.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Remove use of future and six
2f0bbd0 
six and future are Python 2 compatibility modules which are
no longer needed after the end of Python 2 support.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Fix client imports
546bb78 
Fix imports to be vendoring compatible.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Disable pylint W1201: logging-not-lazy
101ab3d 
Disable pylint's "Use lazy % formatting in logging functions"
warning until a common logging approach is decided. See theupdateframework#1334.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Copy link
Contributor Author

Force pushed the new commits. The differences are in

  • 35587c4, thanks for catching the error
  • 2f0bbd0
  • Dropped the "logging-not-lazy" commit and simply ignored the warning with 101ab3d

I chose the lazy way of just ignoring the logging warning now, since there is already an issue capturing the need of a decision: #1334

jku
jku approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

The issues left IMO are:

  • download implementation is fishy
  • the internal file locations may not be final (I think they should be in _internal/ or something)
    but we have these in writing already.

So let's go with this and see where we end up :)

@jku jku merged commit 5dde1e7 into theupdateframework:experimental-client Apr 14, 2021
This was referenced Apr 20, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jku jku jku approved these changes

Assignees
No one assigned
Labels
experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch)
Projects
None yet
Milestone
Experimental client
Development

Successfully merging this pull request may close these issues.
2 participants
@sechkova @jku