-
Notifications
You must be signed in to change notification settings - Fork 70
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
fix bug on update_package.py #1162
Conversation
a318015
to
85cec09
Compare
@@ -37,7 +37,10 @@ def get_latest_version(org, project, version): | |||
print(f"GitHub API response not ok: {response.status_code}") | |||
return None | |||
latest_version = response.json()["tag_name"] | |||
return latest_version | |||
if latest_version.startswith('v'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naacbin introduced the support for packages whose version is preceded by v
in #1067, but it seems the v
was only taken into account when matching the url in the file and not to get the latest version, so it seems like the fix has never worked.
What is really getting me confused is that I see upx being correctly updated by our bot in acec181 before #1067, how is this possible? 🕵️♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sara-rn can you please squash the commits? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sara-rn you have kept all commit messages in the squashed commit. Can you please reword the commit message to explain why the change is needed? For example:
Fix version bug in update_package.py
The regex to match the current tool version in `update_github_url ` does not
include the `v`. However the function `get_latest_version` does include the `v`
producing a URL with a duplicated `v` when replacing the current by the latest
version. Remove the `v` in `get_latest_version` to fix the issue.
You can do this with git commit --amend
(easier in this case) or with git rebase -i HEAD~1
+ r
(which can be used to change the commit message of commits messages that are not the last one too).
I recommend reading https://cbea.ms/git-commit for more info on best practices to write good commit messages. 😉
@@ -37,7 +37,10 @@ def get_latest_version(org, project, version): | |||
print(f"GitHub API response not ok: {response.status_code}") | |||
return None | |||
latest_version = response.json()["tag_name"] | |||
return latest_version | |||
if latest_version.startswith('v'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex to match the current tool version in `update_github_url ` does not include the `v`. However the function `get_latest_version` does include the `v` producing a URL with a duplicated `v` when replacing the current by the latest version. Remove the `v` in `get_latest_version` to fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sara-rn
fix issue #1142 for when --update-type is GITHUB_URL