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

Fixes to package info summaries #98

Merged
merged 13 commits into from
Sep 7, 2023
Merged

Fixes to package info summaries #98

merged 13 commits into from
Sep 7, 2023

Conversation

javierggt
Copy link
Collaborator

@javierggt javierggt commented Aug 22, 2023

Description

The biggest changes in this PR are in skare3_tools.github.scripts.release_merge_info (used to add a list of merges to the each release page) and skare3_tools.packages.get_repository_info (used by the dashboard to collect information on packages, and when releasing ska3-flight, to make a list of all merges since last release).

This PR fixes these issues:

  • when looking for changes between releases, use the packaging module to parse version strings and sort them according to version. This fixes:
    • Case A: When updating the release description, the releases were sorted by version using lexicographical ordering (which is clearly wrong). This caused long lists of PRs in release descriptions.
    • Case B: In get_repository_info, when assembling the list PRs corresponding to releases, the releases were sorted by commit date. It is not uncommon to have the latest release at the HEAD commit. In these cases, the first and second entries in the list (current head and last release) were swapped. This caused an empty version in the dashboard, and showed merged PRs as open PRs.
  • Case C: Changed the caching of get_repository_info so the cache is the same when the arguments are not given and when they are given, but equal to the defaults. Fixes Caching issue in dashboard #97
  • Handle merges with no message when editing release description (Case D) and when getting package info (Case E). Fixes Some PRs are missing in changes summary #90
  • Improve the handling of releases from branches when editing the release description (Case F) and when getting package info for the skare3 release PR changes summary (Case G). Fixes PR list in changes summary is wrong if a release is not from the main branch #100. NOTE: this is not perfect. When listing changes from a release in a branch to a release from master, there could be PRs that replicate PRs on the branch (e.g. PR 423 replicates PR 423 in sot/starcheck).

And these tiny issues:

  • when running skare3-test-dashboard, do it in batch mode (useful for me when I debug, no-op in production)
  • fix a few functions in github.Repository that return lists (including fixing a broken generator)

Interface impacts

None

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

For all these tests, I used the current ska3-flight, while setting PYTHONPATH to point to the skare3_tools local repo on this branch.

  • Case A: I tried the release_merge_info.py script on proseco 5.10.0 (previously a long PR list because the list was sorted lexicographically).
    on the branch:
    python -m skare3_tools.github.scripts.release_merge_info --repository sot/proseco --tag 5.10.0 --stdout
    
    on master one can do this and then remember to fix the release page on Github
    python -m skare3_tools.github.scripts.release_merge_info --repository sot/proseco --sha 18daf21006f73f62f4793fcf8eff604433287ca3
    
  • Case B: I checked the dashboard (proseco 5.10.0 was displaying an empty version because the HEAD commit has the same time stamp as the release commit, so the sorting was wrong)
  • Case B: Also ranget_repository_info interactively. Note that this should include a previous bugfix, release 14.0.2 has no merges because it is at the same commit as 14.0.1.
    from skare3_tools import packages
    pkg_info = packages.get_repository_info('sot/proseco', since='5.9.0', update=True)
    for rel in pkg_info['release_info']:
        print(rel['release_tag'])
        for merge in rel['merges']:
            print(f" - PR#{merge['pr_number']}: {merge['title']}")
  • Case C: I called get_repository_info interactively to verify it was cached as expected:
    packages.get_repository_info.clear_cache()
    %time proseco_2 = packages.get_repository_info('sot/proseco')
    and
    %time proseco_2 = packages.get_repository_info('sot/proseco', version="v4")
  • Case D: The release description should include PRs 127 and 128
    from the branch:
    python -m skare3_tools.github.scripts.release_merge_info --repository sot/xija --tag 4.28.0 --stdout
    
    from master, run this and then fix the release page on Github
    python -m skare3_tools.github.scripts.release_merge_info --repository sot/xija --sha 5b7ca0c400e9346c3161e81955e3b5ef0a6a39c9
    
  • Case E: The following should include PRs 127 and 128
    from skare3_tools import packages
    repo_info = packages.get_repository_info('sot/xija', update=True, since=8)
    repo_info['release_info']
    release_info = {r['release_tag']: r for r in repo_info['release_info']}
    release_info['4.28.0']['merges']
    
  • Case F: called the code in skare3_tools.github.scripts.release_merge_info to verify the PR list is correct (see PR list in changes summary is wrong if a release is not from the main branch #100). Not possible on master.
    from skare3_tools.github import Repository
    from skare3_tools.github.scripts.release_merge_info import merges_in_range
    repo = Repository('sot/starcheck')
    info = {
      '14.3.0': merges_in_range(repo, '14.2.1', '14.3.0'),
      '14.2.1': merges_in_range(repo, '14.1.0', '14.2.1'),
      '14.1.0': merges_in_range(repo, '14.0.3', '14.1.0'),
    }
    for tag, rel in info.items():
        print(tag)
        for merge in rel:
            print(f" - PR{merge['pr']}: {merge['description']}")
  • Case G: Called get_repository_info interactively to verify the PR list is correct (see PR list in changes summary is wrong if a release is not from the main branch #100)
    from skare3_tools import packages
    info = packages.get_repository_info('sot/starcheck', update=True)
    for rel in info['release_info']:
        print(rel['release_tag'])
        for merge in rel['merges']:
            print(f" - PR#{merge['pr_number']}: {merge['title']}")
  • Case G: Used this branch to generate the list of code changes for the upcoming ska3-matlab release (2023.6). Note how the PR between starcheck@14.1.0 and starcheck@14.2.1 is not included because it is not between 14.1.0 and 14.3.0.

@javierggt javierggt changed the title Fixes to package info summaries WIP: Fixes to package info summaries Aug 23, 2023
@javierggt javierggt changed the title WIP: Fixes to package info summaries Fixes to package info summaries Sep 5, 2023
@javierggt javierggt requested a review from jeanconn September 5, 2023 16:51
Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

I haven't given this a line-by-line, but the documented / tested fixes in the description seem good so I think this should just get merged.

@javierggt javierggt merged commit 68d4283 into master Sep 7, 2023
@javierggt javierggt mentioned this pull request Oct 4, 2023
@javierggt javierggt deleted the fixes branch January 5, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants