Tools: update release date checker to check for release date vs. last commit of release#16470
Tools: update release date checker to check for release date vs. last commit of release#16470moderation wants to merge 8 commits intoenvoyproxy:mainfrom moderation:main
Conversation
…vs. last commit date. Signed-off-by: Michael Payne <michael@sooper.org>
repokitteh-read-only
bot
added
the
deps
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
antoniovicente
requested review from
htuch and
phlax
and removed request for
htuch
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good (but noisy with all the reordering, prefer separate commits there). Some nits.
| latest = '' | ||
| pass | ||
|
|
||
| if latest and (github_release.version <= latest.tag_name): |
| assert metadata_version == github_release.version | ||
| commit = repo.get_commit(github_release.version) | ||
| commits = repo.get_commits(since=commit.commit.committer.date) | ||
| if commits.totalCount - 1 > 0: |
| for tag in tags.reversed: | ||
| if tag.name == github_release.version: | ||
| current_metadata_tag_commit_date = tag.commit.commit.committer.date | ||
| if not (version.parse(tag.name).is_prerelease) and version.parse( |
phlax
left a comment
phlax
left a comment
There was a problem hiding this comment.
thanks @moderation - looking good - as commented i was sometimes confused by the release dates on gh <> expected
suggestions below
| return tag.commit.commit.committer.date | ||
|
|
||
| try: | ||
| latest = repo.get_latest_release() |
There was a problem hiding this comment.
this explains why copypastaing release dates from gh sometimes was off-by-one - i had assumed it was a timezone issue
| @@ -40,32 +42,58 @@ def verify_and_print_latest_release(dep, repo, metadata_version, release_date): | |||
| latest_release = repo.get_latest_release() | |||
| if latest_release.created_at > release_date and latest_release.tag_name != metadata_version: | |||
There was a problem hiding this comment.
nit: i would move all of this logic after the try...except and return in the except clause
afaict none of this logic is related to the github exception and it means this chunk can be dedented
| Fore.YELLOW | ||
| + f'*WARNING* {dep} has a newer release than {metadata_version}@<{release_date}>: ' | ||
| f'{latest_release.tag_name}@<{latest_release.created_at}>' + Style.RESET_ALL) | ||
| except github.GithubException as e: |
There was a problem hiding this comment.
as this failure is silently ignored - can we add a comment explaining why it might be triggered and is safe to ignore
|
|
||
| try: | ||
| latest = repo.get_latest_release() | ||
| except github.GithubException as e: |
There was a problem hiding this comment.
likewise here - it would be good to know why this is safe to ignore - if its due to http failure or somesuch then perhaps we should add a warning "unable to retrieve github info" or similar
|
|
||
| # Extract release date from GitHub API. | ||
| def get_release_date(repo, metadata_version, github_release): | ||
| if github_release.tagged: |
There was a problem hiding this comment.
i would factor out the 2 possibilities here into separate functions - ie get_tagged_release_date and get_untagged_release_date or similar - it will allow a load of dedenting and make the logic easier to follow
| return None | ||
| else: | ||
| assert (metadata_version == github_release.version) | ||
| assert metadata_version == github_release.version |
There was a problem hiding this comment.
assert is generally just for testing - i think this should be something more like:
if metadata_version != github_release.version:
raise UpstreamReleaseDateError(...errorinfo...)
| if tag.name == github_release.version: | ||
| current_metadata_tag_commit_date = tag.commit.commit.committer.date | ||
| if not (version.parse(tag.name).is_prerelease) and version.parse( | ||
| tag.name) > version.parse(github_release.version): |
There was a problem hiding this comment.
nit: i reallly dislike multiline if clauses - my suggestion is (always) to express the logic in a variable - eg:
has_newer_release_date = (
not version.parse(tag.name).is_prerelease
and version.parse(tag.name) > version.parse(github_release.version))
if has_newer_release_date:
...do something...
|
Addressed comments and replaced by #16712 |
3 participants
Tools: update release date checker to check for release date vs. last commit of release
Additional Description:
sixfrom pythonhosted to Github location so we can verify release datebazel/repository_locations.bzlandapi/bazel/repository_locations.bzlso thatrelease_datefollowsversionandsha256to aid with dependency maintainancetools/dependency/release_dates.pyto check for release metadata vs. current approach of checking for latest commit in a release which may not equal release date. /cc @htuch + @phlax for Python review.Risk Level: Low
Testing:
bazel --nohome_rc test //test/...,bazel --nohome_rc test @envoy_api_canonical//test/... @envoy_api_canonical//tools/...,bazel --nohome_rc build @envoy_api_canonical//envoy/...,tools/dependency/release_dates.py bazel/repository_locations.bzlDocs Changes: None required
Release Notes: None required
Signed-off-by: Michael Payne michael@sooper.org