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

Rm all legacy #1790

Merged
merged 8 commits into from
Feb 4, 2022
Merged

Rm all legacy #1790

merged 8 commits into from
Feb 4, 2022

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Jan 21, 2022

Fixes #1745

  • Removes legacy code, tests and docs
  • Updates remaining docs and some project metadata accordingly

Preliminary work

Note to reviewers:

  • Excluding deleted files from diff should make review easier
  • I'd appreciate a quick look through the remaining files in the repo, if I missed anything.
  • Check commit messages for details

Note of appreciation
Many thanks to everyone who helped python-tuf to evolve! ❤️

Most notably (based on prior Acknowledgements and git shortlog and in alphabetical order):

Sebastien Awwad, Justin Cappos, Geremy Condra, Vladimir Diaz, Zane Fisher, Pankhuri Goyal, Jussi Kukkonen, Trishank Kuppusamy, Joshua Lock, Marina Moore, Justin Samuel, Teodora Sechkova, Tian Tian, Santiago Torres-Arias, Martin Vrachev, Yuyu Zheng, Kairo de Araujo

@coveralls
Copy link

coveralls commented Jan 21, 2022

Pull Request Test Coverage Report for Build 1788517524

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.4%) to 98.071%

Files with Coverage Reduction New Missed Lines %
tuf/ngclient/updater.py 3 97.96%
tuf/api/metadata.py 6 98.11%
Totals Coverage Status
Change from base Build 1745632739: 0.4%
Covered Lines: 1107
Relevant Lines: 1125

💛 - Coveralls

lukpueh added a commit to lukpueh/tuf that referenced this pull request Jan 24, 2022
Define TESTS_DIR constant in tests/util.py as full path to the
parent directory of the util module. This may be used to reliably
read other files in tests dir, such es "repository_data" or
"simple_server", regardless of cwd.

This commit also replaces a couple of `getcwd() + "filename"` with
`TESTS_DIR + filename`, so that in the future (post theupdateframework#1790) we
should be able to invoke the tests from anywhere, not only from
within the tests directory as is now the case.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh added a commit to lukpueh/tuf that referenced this pull request Jan 24, 2022
Update new test modules to stop using unittest_toolbox, in
preparation for its removal in theupdateframework#1790.

The tools provided by unittest_toolbox can easily (in a more
obvious way) be replaced by using the standard library modules
`tempfile` and `random` (no more used) directly.

In the case of tempdir and -file creation/removal, skipping the use
of unittest_toolbox, which does this by default, also uncovers some
test cleanup failures, which would occur when temporary test
directories were removed while a test server hadn't released them.
(see `except OSError: pass` in unittest_toolbox's `tearDown`
method)

**Change details**

**test_fetcher_ng.py:**
- Stop implicitly creating (setUp) and removing (tearDown) tmp test
dirs.  -Move now manual creation of an exemplary targets file to
setUpClass, as the same file is used by all tests. And remove it
explicitly in tearDownClass after killing the server (see note
about failure above).  - Trigger URL parsing error with a hardcoded
invalid URL string instead of a random string.

**test_updater_ng.py**
- Stop implicitly creating (setUp) and removing (tearDown) tmp test
dirs.
- Explicitly create tmp test dirs in setUp, but don't remove
them in tearDown to avoid above mentioned failures. They will be
removed all at once when removing the tmp root test dir in
tearDownClass

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh
Copy link
Member Author

lukpueh commented Jan 24, 2022

Just moved out test updates for unittest_toolbox removal in this PR to #1792 to make reviewing easier. That PR also fixes the failing windows run seen in this PR.

lukpueh added a commit to lukpueh/tuf that referenced this pull request Jan 25, 2022
TODO: polish commit message

-----
  - Remove outdated ATTACKS.md
    requires basic_client.py which was renamed to client.py in 2018
    and is removed in theupdateframework#1790 (host of problems with client.py described in theupdateframework#881)

    --> reference ticket TODO: create ticket

  - QUICKSTART.md also based on cli tools (see above)
    --> reference ticket TODO: create ticket

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Legacy tools will be removed in subsequent commits.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Remove documentation for legacy client, repository/developer tool
and command line tools, which will be removed in subsequent
commits.

See theupdateframework#1797 and theupdateframework#1798 for replacing ATTACKS.md and QUICKSTART.md.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Remove metadata generation scripts based on legacy
repository/developer tools, which will be removed in subsequent
commits.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Old tests are not touched as they will be removed in subsequent
commits, along with the custom log module.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Remove tests for legacy client, repository/developer tool and
command line tools, which will be removed in subsequent commits.

This commits also removes obsolete test tooling:
- Regarding simple_https_server + test certificates -- http/https
is no longer handled by tuf client directly but transparently by
the underlying requests module used by the default fetcher
implementation.
- For details about unittest_toolbox see theupdateframework#1792

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Remove legacy client, repository/developer tool, command line
tools, and underlying libraries and utilities.

See docs/1.0.0-ANNOUNCEMENT.md for details about their replacement,
deprecation strategy and migration instructions.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
- Update linter config to no longer distinguish between legacy
and new implementation. This requires addressing a linter warning
in an until now not linted module (tuf/__init__.py).

- Remove obsolete rules in MANIFEST.in (source distribution) and
tests/.coveragerc (test coverage).

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh marked this pull request as ready for review January 27, 2022 09:04
@lukpueh lukpueh changed the title Rm all legacy (WIP) Rm all legacy Jan 27, 2022
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.

Looks great 😁 I might have to take another look though, it's hard to cover everything in one go.

Some observations:

  • files in examples/repo_example/ reference repository_tool and repository_lib: they could be updated to indicate that those have now been deprecated
  • ATTACKS.md would indeed be nice to have but that's clearly a project of its own and probably won't look much like the old one so I'm fine with leaving that for now

@lukpueh
Copy link
Member Author

lukpueh commented Jan 27, 2022

files in examples/repo_example/ reference repository_tool and repository_lib: they could be updated to indicate that those have now been deprecated

Thanks for catching! This was on my mind at some point.. but then it wasn't. :D Will push another commit here.

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.

looks good to me. The only additional potential removal I found was the outdated AUTHORS.txt but that does not need to be in this PR.

EDIT: and the test data generation issue I just filed #1806

@jku
Copy link
Member

jku commented Jan 28, 2022

$ tox -e py
...
Ran 128 tests in 2.058s

this is so good.

(if anyone is wondering why there are so many fewer tests: we're using a lot more subtests and those are not counted individually: my Stetson-Harrison assessment is that the new code on average has better coverage than the old one did, although specific areas may be worse).

Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

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

I was waiting for this pr for a long time!
Great stuff here.

I totally forgot that the old updater is 3070 lines, repository_lib 2306 lines, and repository_tool 3291... wow

Also, thanks for the last commitment! I was planning on doing that.

Only one question: do we have a test for key revocation?

@@ -38,17 +38,12 @@ commands =

[testenv:lint]
changedir = {toxinidir}
lint_dirs = tuf/api tuf/ngclient examples tests
lint_dirs = tuf examples tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check if all of the tools recursively check the next folders in tuf/ - tuf/ngclient and tuf/api?

@jku
Copy link
Member

jku commented Jan 28, 2022

Only one question: do we have a test for key revocation?

In the sense that the old tests used the word "revocation", yes we do.

test_updater_key_rotations.py contains loads of different cases of key rotations: some remove ("revoke") old keys, some don't. test_updater_top_level_update.py has tests for fast forward recovery which include complete key rotations (these are made easier to read in #1766).

@jku
Copy link
Member

jku commented Feb 2, 2022

I should have stated this explicitly:

  • I think the PR is good and we should deal with any fallout with new issues after merging
  • I did not merge myself as I wanted other maintainers to have time to review as it's a major change

Rephrase deprecation info in repo_example modules doc headers
to reflect that the deprecation has happened.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh
Copy link
Member Author

lukpueh commented Feb 3, 2022

Thanks for your reviews, @jku and @MVrachev!

  • files in examples/repo_example/ reference repository_tool and repository_lib: they could be updated to indicate that those have now been deprecated

Fixed in 9816c40.

Did you check if all of the tools recursively check the next folders in tuf/ - tuf/ngclient and tuf/api?

Yes, I did. And yes, they do.

Only one question: do we have a test for key revocation?

I admit that I didn't question test completeness. I just rm tests/test_*_old.py (relying on 22fe1e6) and cross-checked with effective test coverage, which didn't decrease.

@lukpueh
Copy link
Member Author

lukpueh commented Feb 3, 2022

I should have stated this explicitly:

  • I think the PR is good and we should deal with any fallout with new issues after merging
  • I did not merge myself as I wanted other maintainers to have time to review as it's a major change

@-'other (active) maintainers', i.e. @mnm678, @joshuagl, mind wrapping this up? 🚀 😃

Copy link
Contributor

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

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

Thanks to all the people who worked on the new client! This is really exciting, and looks good to me.

@lukpueh
Copy link
Member Author

lukpueh commented Feb 4, 2022

Thanks for the reviews, everyone! Merging...

@lukpueh lukpueh merged commit 31fd8d4 into theupdateframework:develop Feb 4, 2022
lukpueh added a commit to lukpueh/tuf that referenced this pull request Oct 11, 2023
- metadata.staged: related to a removed tutorial and outdated deployment
  recommendation
- project: related to the removed developer_tool (theupdateframework#1790)
- map.json: related to TAP4, which is not supported by python-tuf

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
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.

Remove "legacy code"
5 participants