Skip to content

fix: get_current_release_version incorrectly parses commit message with version #444

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

Closed

Conversation

ngshiheng
Copy link

@ngshiheng ngshiheng commented May 6, 2022

Context:

Main changes:

  • get_commit_log now yields an additional commit.author info to the returned tuple
  • get_current_release_version_by_commits now checks that commit_author.name == "github-actions" and commit_author.email == "action@github.com" before returning version that matches regex in the commit message

Description:

  • The main idea behind this change is that there is not really a guaranteed way for us to parse versions correctly from users' commit messages as users can write commit messages however they want
  • This fix checks for the commit author to cope with the issue above

How has this been tested:

  • Ran tox locally and passes all test cases
  • Tried running ../python-semantic-release/semantic_release/.venv/bin/python -m semantic_release publish -v DEBUG --noop locally on one of my repository that uses and failed python-semantic-release action recently

Let me know if this works! I'm more than happy to work together to fix this!

@ngshiheng ngshiheng force-pushed the fix/get-new-version branch from ab19b1e to 167c3ce Compare May 6, 2022 05:12
@ngshiheng
Copy link
Author

ngshiheng commented May 6, 2022

I'm not sure why mypy failed here, any ideas?

Update: the mypy failure seems to be related to python/typeshed#7724 and python/typeshed#7696. I'm skipping mypy check for that specific line for now.

Copy link
Member

@danth danth left a comment

Choose a reason for hiding this comment

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

When Python semantic release is not running under GitHub Actions, the commit author will be different. I don't see any obvious way of knowing what the author will be in these cases.

https://github.com/relekang/python-semantic-release/blob/94c94942561f85f48433c95fd3467e03e0893ab4/action.sh#L15-L17

We could parse the local Git configuration, however, some people might be using their own account details for Python semantic release commits. It's also possible for the commit author to be changed at any point in the project's history.

Currently, these changes will stop versions from being detectable in a non-Actions setting.

@jacksbox
Copy link
Contributor

jacksbox commented May 9, 2022

first: sorry for introducing this issue. I wrote about this issue in context of the change-log here already #424 but somehow missed to to think about it in terms of the prerelease implementation.

I see two options for quick solutions:

  1. rollback the prerelease changes until we have a better solution
  2. add an unchangeable identifier to the commit message, e.g. always let the commit subject end with @python-semantic-release and use this to verify that we have the correct commit...

For the future, let's discuss here #424 if perhaps having only tags as version sources could make this package more robust and make changes / addons easier....

@ngshiheng
Copy link
Author

@danth makes sense. I didn't consider the fact that users could be using python-semantic-release without GitHub Actions. this PR won't work then. i'll go ahead and close this PR

@jacksbox no worries! i'd go with quick solution 1 as i'd expect this bug to break lots of repositories' CI. however, take my words with a grain of salt; i'm sure the maintainers know a lot better

I really appreciate you guys putting effort into this library, thank you!

@ngshiheng ngshiheng closed this May 9, 2022
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.

3 participants