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

[V3 Downloader] Revision tracking #2571

merged 37 commits into from Nov 8, 2019


Copy link

@jack1142 jack1142 commented Apr 11, 2019


  • Bugfix
  • Enhancement
  • New feature

Description of the changes

This PR implements features proposed in #2527. (closes #2527)
Fully fixes #1866 - as said in #2605, the change was easier to do after this PR is finished so I decided to do it in this PR as well so that it actually gets done before 3.2 release.
Fixes #2927 - I added a check for installed cogs that have the same name and added a attribute to InstalledModule that keeps repo name as it was saved in Config.
Due to size of the changes, I added myself to CODEOWNERS file.
I know this doesn't actually give any benefit to people that don't have write permission to the repo but I saw other big fella devs doing this, so I assume this is what I should do.

Internal info: All function parameters are explicitly called hash, if it can only be commit's full sha1 hash or rev if it can be anything that names a commit object, see link

Downloader cog - changed/added commands:

  • new [p]repo update updates repos without updating any cogs
  • install and update commands tell about failures during installing/reinstalling requirements and copying cogs and shared libraries now
  • [p]cog install can now install multiple cogs
  • new [p]cog installversion allows you to install cogs from specific revision of repo
    • usage: [p]cog installversion <repo_name> <revision> <cogs>
    • this command also pins (blocks updates for) cogs that were being installed
    • shared libraries will always be installed from the latest commit
  • changes to all install commands:
    • if cog is already installed, Downloader won't try to install it again
    • if cog with same name is already installed (not the same one, but having the same name), Downloader won't allow user to install it without uninstalling the other one first
  • new [p]cog pin and [p]cog unpin allows you to pin and unpin cogs (block/unblock updates)
  • new [p]cog checkforupdates checks for available cog updates (listing will include pinned cogs)
  • changes for all update commands:
    • Now only cogs that need updating will be updated and pinned cogs will be skipped in check
    • Python and bot version are now getting checked during update too
    • if module isn't available anymore, updater will try to find its last occurrence and update to that version (unless that version doesn't support current bot version)
    • NOTE: update commands still update repos first
  • new [p]cog updateallfromrepos updates all cogs from given repos
  • new [p]cog updatetoversion updates chosen (or all) cogs to specific revision of repo
    • usage: [p]cog updatetoversion <repo_name> <revision [cogs]

Downloader's tests:

Here's somewhat big change, I added git integration tests that will make sure that all the git commands we use give expected output. Most git commands require a repo to operate on so along with this I added a test repo and a new (cool) script at tools/ that will allow devs to easily import and export that repo from git fast-import format. I'm not gonna list all of the new tests but other changes that were done indirectly to tests are:

  • using same event loop policy as in Red's code so that our tests are run in the same way as Red would be running it (note: for now this change isn't strictly needed and is more of an improvement, but in future as per Toby's suggestion I will be switching Downloader over to asyncio.subprocess and default Windows loop doesn't work with it)
  • added pytest-mock requirement to tests extra as mocking was very needed to tests Downloader's logic without depending on calling git part.

Aaaaand, here you have changes that are rather boring for average reader:

Unfortunately, few public methods had to have their behaviour changed and therefore this PR is breaking change for devs using Downloader framework. To make it a little easier, I added [BREAKING] at the end of each change, which can be considered breaking (hopefully I haven't missed anything).

Downloader cog - internal changes:

  • new config schema - now both installed cogs and installed libraries are kept in config
    • both of those config groups are grouped like this:
      installed_cogs = {'repo_name': {'cog_name': cog_json}}
    • old conf format is getting converted with _maybe_update_config()
  • new installed_libraries() gives you installed libraries and installed_modules() gives you installed cogs and libraries

RepoManager and Installable classes - changes:

  • RepoManager now has repos attribute which gives you tuple of Repo
  • new RepoManager.get_all_cogs() gets all cogs from all repos
  • RepoManager.update_repo() returns 2-tuple instead of dictionary with one key [BREAKING]
  • Installable holds commit information now
  • Installable now has repo attribute containing repo object or None if repo is
  • new InstalledModule class inheriting from Installable - all installed cogs/libraries should be represented by this
    • new from_installable() method of InstalledModule creates instance from Installable
    • to_json() and from_json() methods moved from Installable to InstalledModule
      • cog_name key in internal cog's json changed its name to module_name, new keys commit and pinned (only for cogs) added

Repo class - changes:

  • Repo holds commit information now
  • new Repo.is_ancestor() checks if one revision is ancestor of the other.
  • new Repo.is_on_branch() checks if repo is currently on branch
  • new Repo.get_last_module_occurrence() tries to get module from last commit in which it still occurs (if it couldn't, it returns None)
  • new Repo.get_modified_modules() gets modified modules between two revisions and if any module doesn't exist in new revision, it will try using Repo.get_last_module_occurrence() to find it.
  • new Repo.get_full_sha1() gets full sha1 object name from given revision.
  • new Repo.checkout() checks out repo to provided revision. The method can be awaited or used as async ctx manager:
    • if awaited, method will only change current commit
    • if used as async ctx manager, it will change current commit and go back to previous one after exiting context (or one provided in exit_to_rev argument)
    • no matter which option is used, when rev is None, method won't do anything and when force_checkout is set to True, checkout will be done even if Repo.commit equals rev/exit_to_rev

  • Repo.current_branch() will now properly error out if repo is in detached HEAD state instead of returning "HEAD" [BREAKING]
  • Repo.current_commit() now returns commit, which the repo is on, instead of latest commit of branch, Repo.latest_commit() provides old behavior of this method [BREAKING]
  • Repo._get_file_update_statuses() will now work properly with paths with spaces and with renames (treating them separately as addition and removal)
  • all Repo._run() calls will redirect stderr to debug log, if valid exit code is returned by process or to error log otherwise
  • Repo.install_cog() returns InstalledModule instance now instead of bool representing success of install [BREAKING]
    • if cog copying was unsuccessful, this method raises CopyingError
  • Repo.install_libraries() returns 2-tuple of installed and failed libraries instead of bool representing the success of install [BREAKING]

@mikeshardmind mikeshardmind self-requested a review Apr 13, 2019
@jack1142 jack1142 force-pushed the V3/issue_2527 branch 2 times, most recently from 8a40137 to e43271e Compare Apr 21, 2019
@jack1142 jack1142 marked this pull request as ready for review Apr 21, 2019
Copy link
Member Author

@jack1142 jack1142 commented Apr 21, 2019

Okay, I think, this is ready for review now, I added all things from my TODO list (which you can find in previous revision of issue description) and I changed whole issue description and updated it with all changes this PR does.

At the end, I have one question, should I make any new unit tests? I don't have a lot of experience with those, so I didn't add any, but if I should, I'll look into it.

@jack1142 jack1142 changed the title [V3 Downloader] WIP - Revision tracking [V3 Downloader] Revision tracking Apr 23, 2019
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone May 23, 2019
Copy link

@mikeshardmind mikeshardmind commented May 23, 2019

I've marked this for 3.2 due to breakage concerns. While we haven't explicitly exposed some of this, changes to downloader have a large potential for negative impact if we don't announce every potential change in behavior with advance warning.

@Tobotimus Tobotimus added Type: Feature Big fella and removed V3 labels Jun 29, 2019
@mikeshardmind mikeshardmind added the Breaking Change label Sep 26, 2019
@jack1142 jack1142 force-pushed the V3/issue_2527 branch 3 times, most recently from 3112ca5 to 32f0559 Compare Oct 27, 2019
jack1142 added 24 commits Nov 7, 2019
- changes to `Repo.checkout()`:
  - `exit_to_rev` is now keyword only argument
  - added
`force_checkout` to force checkout even if `Repo.commit` value is the same as target hash
- added two keyword arguments:
  - `valid_exit_codes` which specifies valid exit codes, used to
determine if stderr should be sent as debug or error level in logging
  - `debug_only` which
specifies if stderr can be sent only as debug level in logging
- fix wrong type annotations and add a lot of new ones
- add checks for `Installable.repo` being `None`
- fix wrong return type in `Downloader._install_requirements`
- show repo names correctly when updating all repos
- fix error when some requirement fails to install

- type of `Repo.available_modules` is now consistent (always `tuple`)
This script aims to help update the human-readable version of repo
used for git integration tests in ``redbot/tests/downloader_testrepo.export``
by exporting/importing it in/from provided directory.

Editing `downloader_git_test_repo.export` file manually is strongly discouraged,
especially editing any part of commit directives as that causes a change in the commit's hash.
Another problem devs could encounter when trying to manually edit that file
are editors that will use CRLF instead of LF for new line character(s) and therefore break it.

I also used `.gitattributes` to prevent autocrlf from breaking testrepo.

Also, if Git ever changes currently used SHA-1 to SHA-256 we will have to
update old hashes with new ones. But it's a small drawback,
when we can have human-readable version of repo.

Known limitations
``git fast-export`` exports commits without GPG signs so this script disables it in repo's config.
This also means devs shouldn't use ``--gpg-sign`` flag in ``git commit`` within the test repo.
Also added Markdown file that is even more clear than export file
on what the test repo contains.
This is manually created but can be automated on later date.
These tests use expected output that is already guaranteed by git tests.
I know this doesn't actually give any benefit to people that don't have
write permission to the repo but I saw other big fella devs doing this,
so I think this might be advisable.
…om `IntEnum`

There's desync of `InstallableType` class types due to hot-reload
and `IntEnum` allows for equality check between different types
Kowlin approved these changes Nov 7, 2019
Copy link

@Kowlin Kowlin left a comment

Works great.

@mikeshardmind mikeshardmind merged commit e2c8b11 into Cog-Creators:V3/develop Nov 8, 2019
1 check passed
@jack1142 jack1142 deleted the V3/issue_2527 branch Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Breaking Change Type: Feature
None yet
5 participants