python-semantic-release icon indicating copy to clipboard operation
python-semantic-release copied to clipboard
verified publisher icon python-semantic-release

Internal refactoring

Open bernardcooke53 opened this issue 3 years ago • 6 comments

Description

This isn't really a feature request per-se, its more a discussion about python-semantic-release, the development experience and its continued evolution.

Story time: I added a couple of comments onto an issue a few months ago (#423) regarding support for multiple branches. Since then I can see that #485 was opened with the same request.

Since then I've changed my full-time job, etc etc, and I finally found time to start working on a PR yesterday. I love what python-semantic-release and its JS counterpart set out to do, and multibranch support is a must-have for me to integrate it into many projects. I know JS supports this but why should they have all the fun? :wink:

However, pretty quickly after I started working on it, I found a couple of things that I realised were really making the experience of working on the feature difficult. So I have changed track and am working on a PR which addresses the following things:

  1. There are many levels of indirection within the codebase. I know this generally promotes re-use, but I would like to follow 4-5+ levels of function calls as few times as possible to understand what really happens :slightly_smiling_face:
  2. The type annotations are informational but not 100% accurate. For example param: str = None. This makes it hard to reason about default values, edge cases to be accounted for, etc.
  3. (The Big One) There is an innocent looking global variable config.
  4. I think much of the codebase itself has added complexity because config doesn't really abstract any of the implementation details, either. This line is an example - why does it need to know that changelog_sections is a comma-separated string? I am happy to discuss this in more depth - but I think the current implementation is limited by being only a simple str: str key-value store. Leveraging Python's rich type system - lists, enums, namedtuples, etc - in my opinion will drastically simplify the code outside of the configuration setup.

The reason that the config variable (respectfully!) sucks to work with is it pops up absolutely everywhere, effectively as hidden parameters to every function. Take this function for example:

def current_commit_parser() -> Callable:
    """Get the currently-configured commit parser
    :raises ImproperConfigurationError: if ImportError or AttributeError is raised
    :returns: Commit parser
    """

    try:
        # All except the last part is the import path
        parts = config.get("commit_parser").split(".")
        module = ".".join(parts[:-1])
        # The final part is the name of the parse function
        return getattr(importlib.import_module(module), parts[-1])
    except (ImportError, AttributeError) as error:
        raise ImproperConfigurationError(f'Unable to import parser "{error}"')

What if config.get("changelog_components") is None? What is the default - I need to look it up, potentially in the docs (e.g. for commit_version_number, where the behaviour depends on other params). I know having worked through it that we ensure it's populated through defaults.cfg, but now a change to defaults.cfg might ripple throughout the codebase in an at least semi-obscure manner.

When we talk about adding features like multibranch support, or bugs like versions not being set correctly, it's hard to trace the source back to poor configuration.

Also when it comes to testing, there's a lot of mocking and patching required (e.g. test_should_build) just to test different configuration options. These tests don't measure the impact of unrelated parameter default values, and require care to make sure that all the right configuration variables are set and kept in sync with the source code.

The PR I'm working on will look to address just these 4 things - my goal is actually not to add any functionality nor fix any bugs, but to enable feature and bugfix development to happen much more smoothly. I intend to follow that PR up with another for multibranch support, which I guess would take python-semantic-release to 8.0, but addressing this will make testing-for - and fixing - the other bugs simpler.

The PR I'm working on is to address these three things only - the end goal is to have python-semantic-release completely unchanged in terms of features or bug fixes, and to have a like-for-like which is easier to test, fix and develop further.

Possible implementation

Describe any ideas you have for how this could work.

I'm writing my PR's first commit to simply add parameters all over the codebase in all the functions, so that they can be passed in. I'm doing this irrespective of hacks, needed to hard-code into if/else statements, just to show the scale of changes to make this work (and so it doesn't just look like a senseless rewrite).

Following that commit I will make 1+ more gradually cleaning up the mess of the first.

I appreciate that this might be a fairly significant change to the codebase - it certainly feels like it as I'm going through it! - but I do genuinely believe that it will be worth it, and that many other things will be easier to achieve once this is done.

Hoping to have the PR raised in the next couple of weeks :tada:

bernardcooke53 avatar Aug 06 '22 08:08 bernardcooke53

Quick update - I am/have been actively working on this for the last couple of weeks. It's a bigger job than I first anticipated, going through the code in depth is well worthwhile, but there are more things I need to unpick than just the configuration to really make this clean. A couple of examples would be:

  • Hvcs classes don't really use any instance state, they're currently backed by the config global. Unfortunately they also post-process the values from config with environment variables etc, and that postprocessing is needed in say changelog. At the moment, there are a couple of delayed import hacks to avoid circular imports - something I hit up against a few days ago which made me have to redesign some bits.
  • I'm also looking to remove this singleton in favour of explicit parameterization - I'm envisioning testing against a pytest fixture with a repo initialized in a temp directory with a controlled commit/tag history (and that wouldn't require patching/mocking).
  • I have looked back through the issues and seen one about generating a CHANGELOG.rst instead of a CHANGELOG.md, and as rightly noted there it's currently extremely difficult to do so because of the way the changelog is pieced together. I'm looking at this function and it seems to create a rough "AST"-like dict of components, I want that to provide some kind of non-markdown-specific representation that other functions like a changelog_rst could use to render the changelog.

Regardless of change in functionality or not, it's looking sensible to flag this change as breaking, since the internals are going to be drastically different - anyone importing into other apps won't be able to use in the same way. I'll try to maintain as much compatibility for command-line/config users according to the documented features, add some deprecation warnings where things need to be removed, and then follow it up with a cleanup removal for a v9. For things like commit parsers, there are parser-specific configuration options which hugely complicate the dynamic loading of parser functions. Use of a custom parser is a documented feature, so anyone doing so would need to update their parser to a new standard interface.

I'll update here periodically and link to my fork when there's code up that a) works and b) would be valuable to see what I'm up to 🙂

bernardcooke53 avatar Aug 18 '22 08:08 bernardcooke53

Brief note - I'm trying to stay abreast of issues/fixes/features being added, but only incorporating them into this refactor if they get merged to main (the hope being that addressing other issues will be easier once the refactor is done)

bernardcooke53 avatar Aug 23 '22 16:08 bernardcooke53

OK - I've been doing some in-depth time with the code, playing about with how it fits together, what's ergonomic, what isn't, and trying to put it back together in different ways. So far, I have a fairly good idea of what I'd like to do, and for transparency I've put the pieces of analysis onto a branch on my fork. I haven't got onto publish but I had all the other commands behaving (the is also definitely not what I'd call polished).

The absolute biggest headache, by far, is backwards-compatibility. I'd estimate 40-50% of the time I've spent looking at this has been trying to make non-breaking changes. This is a lot easier if compatibility is limited to only the CLI but there's no guarantees about other users installing & using the programmatic interface. Once I realised this I decided to leave the note above about breaking changes, since no matter what's changed, that side of PSR would break with a refactor.

That said I want to change tack a bit - I'll put together a PSR v8, leave it up for discussion and as part of that hopefully we can decide what needs to have compatibility adaptors. Ideally major versions don't come about too often, so I want to absorb a couple of key feature enhancements into this too to avoid a rapid v7 -> v8 -> v9 cycle.

Because of this - @relekang, @danth I'd really love your thoughts on the below, but also it might be an idea to have an 8.0.x branch for 8.0.0-rc.N? That way it could start being used before pushing a breaking change out on main?

Here are my observations for things I think are important to address:

Multiple branches There are two open issues about configuring PSR to release from multiple branches - #485 and #423. Given that this is a core feature of the JavaScript SR, it seems like the key feature to absorb into the next major release, particularly as it will definitely be breaking on this line.
Changelogs One thing I've recognised as I've spent time with the code is that the Changelog implementation is extremely tightly coupled to Markdown. Because of the information flow, making new metadata is available is challenging. Most of the current implementation hangs together through dynamic imports and grabbing from the config global. There are 6 open issues and a milestone for changelog improvements (admittedly it sounds like a couple will be resolved by this PR).
Configuration The sheer size of defaults.cfg... it makes it really simple to add a new configuration key, grab it from the global config and use it wherever it's needed, sure. But in terms of identifying what each parameter does, what behaviour/commands it affects and if it needs to exist, I think it's grown into a pain point now - I'm looking at parser_angular_minor_types and minor_emoji (which is actually an iterable of emojis) as an example. It also means that when looking at issues, you can't easily scope out sections of config to ask for unless you know what settings you need to see.
Testing & bugs At the time of writing there are a handful of open bugs about incorrect next versions, changelog generation, updated source code, etc. It seems hard to dig into these because creating a replica environment to reproduce requires exact configuration (see above) and because identifying the path taken through code is challenging - not only do you have to follow several levels of function-calling-function, but you also need to cross-check the config settings and follow dynamic imports. Testing may help with this, but the current PSR does much of its testing through mocking/patching. There's value in this, but on its own it limits what you can test in more real-life environments.

PSR v8

At a high level here's what I want to achieve:

Project Structure I would like to modularise the CLI into a sub-package, and within that sub-package hold all of the code for setting click options, reading config from files etc, as well as defining the commands. Outside of this package I would like to structure PSR as a clear library with a consistent functional API - the CLI would just be the primary consumer of this API. It will help people who want to build their own apps on top of PSR's awesome functionality, but most importantly it will help with testing the library code so that the commands are thinned down to getting the right parameters and invoking library code. I would like to ensure there's no need to globals like config and repo floating around, and no danger of circular imports that need to be avoided.
Configuration What I originally set out to do - remove the config global, and parse the configuration file into rich Python types instead of using str, int and bool + splitting where needed. However, I would also like to rethink the structure of the configuration file to organise options around the commands/behaviour that they impact, and I would like to add support for overlaying values in a sub-table that will take effect only when releasing from a particular branch (likely a regex match on git.Repo.active_branch). Coupled with a few other changes I would ideally like to simplify similar options down and I think some will be redundant.
Changelog Template I would like to add a configuration option for changelog_template, pulling in Jinja2 as a template engine, and provide the ability to just render the template during a release. This relies on providing a contract as a CLI for certain metadata to be available during the rendering, however given that PSR already has support for custom components and the other config options are to do with stylistic/structural choices for the changelog, having this available would greatly simplify this part of the config. Custom sections could be done either explicitly in the template or as macros, and it would give a great deal of flexibility to the user regarding how they want their changelogs to look. Coupled with this, I'd like to restrict changelog generation down to just an abstract representation of the information parsed out of the commit logs - by not tying down to markdown syntax, we get the ability to produce RST-format changelogs almost for free (with a different template).
Multi-branch release support I guess this is pretty self-explanatory. Worth re-iterating from the Config heading, though, the idea is to create a subtable of config with a regex to match against the branch name, and overlay those values if the regex matches (same idea as I added back on #423)
Drop "commit" as a version source It seems like identifying the version from the release commit is superfluous - I'm not sure I see the merit in adding just a release commit without a tag to identify it by, but I could easily be mistaken so happy to hear of any valid use cases for this. On the flip side, just recently #490 crept in, and unsure if #474 is resolved as a result but it is definitely caused by tricky commit parsing. tag_commit is defaulted to true anyway. #424 gives me a decent amount of confidence that this would be an improvement, too. Small note - I think the docs should also stress that version_variable, version_pattern, version_toml etc should not be used for identifying the latest version, but are locations where the version metadata will be updated.
Testing I'd really like to set up the test structure to perform unit tests against individual library functions, and add some pytest fixtures with git.Repo instances over temporary directories to really test PSR against an isolated repository. It would be ideal to not have PSR's testing depend on its own metadata (admittedly, this looks like a pretty old test and I only saw this in one or two places).

So I will crack on with working towards this, but would absolutely welcome any steer and any feedback either here or in an issue against my fork.

bernardcooke53 avatar Aug 29 '22 10:08 bernardcooke53

Heyhey @bernardcooke53, thanks for the work you are putting into this - I was thinking about starting a bigger refactoring for some time (and wrote one or two small issues about this), but never had the time to really do so.

I read all the issues you raised in the last comment here, and I agree with most / all. Especially the config (too many options), testing situation (it is currently really hard to test something in dept) and project structure.

I also had a glance at your proposed project structure in you fork.

What I would like to bring in, open a discussion about: It seems clear, that any bigger refactoring will mean a complete overhaul of the repository and code, and bring breaking changes as well. Given that, maybe it would make sense to switch to a plugin approach like the js version of this package is using. e.g. hvcs could all be individual packages, changelog generation / uploading, support for different build systems etc. pp. This way we could slim down the code base and sort it in smaller, easier testable chunks.

We could have the core package, with the plugin and lifecycle architecture / handling, with one implemented default flow. (maybe for github, as the project itself is hosted here) Then we would have additional packages to support fancy change-log generation, gitlab, butbucket, toml, you name it support.

jacksbox avatar Aug 31 '22 10:08 jacksbox

Hey @jacksbox - totally relatable re: time, I'm impatient to get it finished but it's also important to take time to rest when you need it and spend time with family etc; OSS is a hobby and it would be a shame to spoil that with too much pressure!

I'm happy to hear you're on-board with what I proposed, I spent quite a bit of time reasoning out what's important short-term & trying to justify that thoroughly as time invested :slightly_smiling_face:

I agree with you that it would be nice to adopt a plugin-style architecture, giving the flexibility to just install what you're interested in or indeed to release your own plugin - as the JS version supports.

My main concern with going straight for this is that the JS implementation relies on a shareable configuration contract - in your .releaserc you are able to define per-plugin configuration and the core package is able to make certain guarantees about how the plugin is loaded, passed the configuration, the order of steps that are executed (e.g. prepareCmd, publishCmd) so that plugin authors can code to meet some kind of specification. Also, all of that functionality currently exists in this one repo - we would be taking it out to move it into plugins, which could equally be done later as an invisible change by installing a default set of plugins rather than having the code immediately available in a single package. Actually one of the proposals - refactoring the CLI into a sub-package - was deliberately designed to be extensible, so that a plugin author could potentially layer additional commands on top of the core semantic-release. A set up such as

# semantic_release/cli/__init__.py
from semantic_release.cli.commands import main, version, publish, changelog

main.add_command(version)
main.add_command(changelog)
main.add_command(publish)

would potentially enable a plugin author to extend the CLI via:

# my_semantic_release_plugin.py
import click
from semantic_release.cli import main, pass_config, Config

@main.command
@pass_config
def custom_command(cfg: Config):
    print("Hello, World!")

(Disclaimer: that's super off-the-cuff and I spent a good 30 seconds thinking about it, but it seems more modular and easily-maintainable to set the currently-included commands up the way they are regardless).

My opinion is that we're not ready to look at plugins yet because we need to solidify the contract that we'd offer to plugin developers via configuration structure/execution order etc, but that there are other breaking changes which are also highly sought-after (multibranching!) and some work to do making the tests a bit more comprehensive and robust, and this would enable the conversation about plugins to progress much faster once out of the way.

bernardcooke53 avatar Aug 31 '22 16:08 bernardcooke53

This feature request has been labelled as help wanted since there has been no activity in the last 3 weeks. It will not be closed.

github-actions[bot] avatar Sep 22 '22 00:09 github-actions[bot]

Hey @jacksbox, @danth / anyone else watching along :wave:

I wanted to drop an update on this issue about the rework I've been doing. I feel like it's going pretty well - not finished yet, but I roughly have the structure and CLI the way I had in mind, I'm able to do multibranch releases, I've added a bit more testing, and so on. I still need to do some polish work on things like the default changelog, a bunch of TODO in comments, static analysis, tests for the CLI etc, but nothing showstopping. I've also completely ignored the docs for now, I wanted to figure the code out first.

Actually I wanted to bring this up firstly to show how different the project is - now that I've stopped to look back over it, what I have is totally different to what I started with and I appreciate that scale of change might not be popular. So if you have a little time, the diff URL is here - would you be able to have a look over what I've done so far and let me know your thoughts?

Secondly - I've reached a point when looking at configuration where I think it'd be a lot of effort to work with ini-format configuration as well as toml, and so I'm really thinking about dropping support for setup.cfg like black does, in favour of just using pyproject.toml for settings. I also wanted to get your thoughts on that?

If you want to set up an example project to try this out with - I've added a new CLI command, so from your virtual env you can run

python3 -m semantic_release generate-config --format toml > config.toml

You'll then be able to try out other commands with (e.g.)

python3 -m semantic_release version --print -c config.toml -vvv

As I said I haven't really touched the docs yet, so if you want me to explain something I'm more than happy to - hopefully it's not too confusing. One more note - for the moment I have the existing tests in a tests.old directory just so I can check I'm covering all the same bases as the existing testing, so if you want to run pytest please could you do pytest tests :slightly_smiling_face:. As of this morning there's only a couple of failing tests to do with invoking commands in the CLI that I haven't got to yet.

Keen to hear what you think - thank you!

bernardcooke53 avatar Oct 22 '22 10:10 bernardcooke53

Great work @bernardcooke53 👏 Thanks for taking the time to take a real shot at this! There is a lot to go through, but I really like the direction it is going.

Secondly - I've reached a point when looking at configuration where I think it'd be a lot of effort to work with ini-format configuration as well as toml, and so I'm really thinking about dropping support for setup.cfg like black does, in favour of just using pyproject.toml for settings. I also wanted to get your thoughts on that?

Yes, do it :+1:

I'm thinking we should probably freeze v7 except for critical bugs after merging #486. What do you think? Maybe we could set up a v8 branch with the current changes that you have now? I kind of want to get this repo over to an org. I'm a bit unsure if creating a psr org or trying to transfer to jazzband would be best. I also need to do some investigation into how that will affect existing users of the github action.

relekang avatar Oct 22 '22 17:10 relekang

@relekang I'm really glad to hear you like the way it looks! And dropping setup.cfg is a huge help too 👍

I was thinking a v8 branch here would help a lot - I've watched the way poetry have approached their 1.2 development and that was really community-driven, it'd be great to get an alpha version or two out for more people to try v8 out. I don't have any view on how the internals for psr are getting used at the moment so there's a good chance that an unpinned version somewhere will cause a headache for someone. I think a change freeze would help keeping v7 stable and encourage contributions to go into v8 - sounds like a good idea to me.

Regarding an org - I think if you transfer ownership GitHub will redirect from the old url, at least for a while. My only thought is that if plugins are on the horizon they might be better grouped into their own org - jazzbox is 80% Django at the moment.

I absolutely don't want to impose on you or the project - but if you want some help with maintenance I'd be more than happy to pitch in, wherever that's part of ✨️

bernardcooke53 avatar Oct 22 '22 19:10 bernardcooke53

I think we'd be better with a separate organisation if there are going to be multiple repositories. That way people could use the organisation homepage to search for plugins.

I also need to do some investigation into how that will affect existing users of the github action.

Git operations are redirected indefinitely, unless you create a new repository under the old name. I would expect this to apply to actions too?

danth avatar Oct 23 '22 11:10 danth

Sounds good, I also think that it will be redirected, but I'll do a little test before moving the repo to make sure it works and if not i will try to fork it back to my account to see if that works.

relekang avatar Oct 23 '22 18:10 relekang

I've created https://github.com/orgs/python-semantic-release and will configure and try to move over the next few days.

relekang avatar Oct 23 '22 19:10 relekang

thumbs up for the progression to an own org!

jacksbox avatar Oct 24 '22 08:10 jacksbox

@bernardcooke53 will try to have a look at the v8 code in the next days and come up with some feedback!

jacksbox avatar Oct 24 '22 08:10 jacksbox

Looks like it is working as we expected: https://github.com/relekang/ubiquitous-adventure/actions job 2 is when the action repo was on my user, run 3 is when it was on the new org. Will move this repo probably some time during the weekend.

relekang avatar Oct 27 '22 19:10 relekang

Since (functionality wise) one of the biggest improvements is the support for multiple branches I suggest that before this is merged to the master branch and released it is first verified that this functionality is working as expected by creating a test repo and then follow all the steps outlined in the examples for the JS counterpart library when working with multiple branches (see links below)

If this results in the expected version and git history graph being produced at every step of those examples then I think we can safely say that it is working.

Publishing pre-releases Publishing maintenance releases

My main point here is that once this has been merged into master and released it will be harder to fix any such issues related to this without breaking the behavior expected by the users.

ash23 avatar Nov 16 '22 12:11 ash23

Hey @ash23 ,

I've been testing it against a test repo of my own as I've been working on this, but I believe the intention is to create an 8.x.x branch and make a pre-release on PyPI before merging to master. There's a good chance that it'll need a few alpha/rc versions from that branch before it can be merged to master and released as 8.0.0

I don't seem to have permission to create that branch @relekang - are you ok to make it?

It's also just waiting on me to finish up on the documentation before that first alpha version could be published

bernardcooke53 avatar Nov 16 '22 12:11 bernardcooke53

Hey @relekang - I think I'm ready to raise a PR for this. Do you want to create that 8.0.x branch or should I raise it against master? 😊

bernardcooke53 avatar Dec 15 '22 08:12 bernardcooke53

I bet it is way to big to review in one go so my suggestion is that you push it directly to 8x branch on this repo and start to do some testing and then if we find some stuff to fix or improve we can create pr's against that branch. What do you think?

relekang avatar Dec 15 '22 09:12 relekang

Sounds good to me - I don't have push access, though, and don't think I can raise a PR from my fork against a non-existent branch

bernardcooke53 avatar Dec 15 '22 14:12 bernardcooke53

Thought I gave you access weeks ago 😅 Sorry about that, does it work now?

relekang avatar Dec 16 '22 07:12 relekang

No worries 😅 yeah it works now, the PR is up!

bernardcooke53 avatar Dec 16 '22 15:12 bernardcooke53

All the code is now at https://github.com/python-semantic-release/python-semantic-release/tree/8.0.x

bernardcooke53 avatar Dec 18 '22 22:12 bernardcooke53

Nice to be able to post this link: https://pypi.org/project/python-semantic-release/8.0.0a3/ Any help testing/reviewing would be much appreciated :pray:

bernardcooke53 avatar Feb 04 '23 17:02 bernardcooke53

Hi there, I would like to report a few issues I discovered while trying out the pre-release.

1:

If I set a non-default git commit user in pyproject.toml like this:

[tool.semantic_release.commit_author]
env = "GIT_COMMIT_AUTHOR"
default = "github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>"

and then run "semantic-release version" in CI, the CI workflow fails with:

git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git commit -m chore: release 0.1.1

[skip ci] --author=github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --date=1677569964
  stderr: 'Committer identity unknown

*** Please tell me who you are.

Run

  git config --global user.email "[email protected]"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for <runner@fv-az341-892.xez1knj354qubb1hnut5jnrmef.gx.internal.cloudapp.net>) not allowed'
Error: Process completed with exit code 1.

I guess the git commit user is not set, like it is done in the GitHub action explicitly? Or is the setting done incorrect?

You can reproduce with: https://github.com/afuetterer/minimal-example/

afuetterer avatar Feb 28 '23 07:02 afuetterer

2:

If I try to use a custom changelog template ("CHANGELOG.md.j2", located in templates, as per my understanding of the docs, the version command fails locally and in CI.

The CHANGELOG.md gets generated and populated alright, but I get an error, when semantic-release tries to commit:

 Traceback (most recent call last):
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/bin/semantic-release", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/semantic_release/cli/commands/version.py", line 307, in version
    repo.git.add(updated_paths)
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/git/cmd.py", line 741, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/git/cmd.py", line 1315, in _call_process
    return self.execute(call, **exec_kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/.cache/pypoetry/virtualenvs/minimal-example-g8q-b6Jv-py3.11/lib/python3.11/site-packages/git/cmd.py", line 1109, in execute
    raise GitCommandError(redacted_command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git add CHANGELOG.md.j2
  stderr: 'fatal: pathspec 'CHANGELOG.md.j2' did not match any files'
Error: Process completed with exit code 1.

It seems to try to add CHANGELOG.md.j2 instead of CHANGELOG.md to the commit.

You can reproduce with: https://github.com/afuetterer/minimal-example2/

afuetterer avatar Feb 28 '23 09:02 afuetterer

Hey @afuetterer, thanks a lot for testing the pre-release out and reporting those issues! I have created a branch with fixes for those items. Your minimal examples are extremely helpful in debugging and testing fixes out, thanks for those too. Regarding the templates folder - it'd be interesting to hear whether or not you think it's useful. I wrote some code doing essentially that a while ago so I thought I would add it as a feature, the rationale being that it's possible to store e.g. macros in a separate file to the CHANGELOG, and to render other files (e.g. AUTHORS.md) during a release. But it might be overkill and I'm not opposed to removing it if it's not useful

One thing that this has really highlighted is that the test coverage is pretty poor - particularly around the version command - I had some trouble getting click.CliRunner into the tests to hit the command-line itself, and ought to factor that out into more testable units. I'll try and batch that in with the fixes to "prove" they're working, just leaving the thoughts on this thread as mini-update for where the effort is going next.

bernardcooke53 avatar Feb 28 '23 20:02 bernardcooke53

One more thought which has occurred to me:

Now that the bulk of the logic has moved to the version command, it's pretty obvious that publish has just two purposes:

  • Upload 1+ Python distributions to a package repository (e.g. PyPI) using twine
  • Upload 1+ Python distributions as assets to a VCS release (e.g. a GitHub release)

However - effectively we are running a wrapped version of twine upload, with all the added complexity of dealing with Twine settings whilst bypassing the validation that Twine's CLI would typically perform for its users. Given that version will handling the project's metadata, and publish the upload part of a release process, one would have to use two separate commands:

semantic-release [OPTIONS] version [CMD_OPTIONS]
semantic-release [OPTIONS] publish [CMD_OPTIONS]

The thing is, that's no more code than doing:

semantic-release [OPTIONS] version [CMD_OPTIONS]
twine upload [OPTIONS]

For users of GitHub actions, there is also https://github.com/pypa/gh-action-pypi-publish which could equally handle the functionality of publishing to PyPI, and https://github.com/softprops/action-gh-release which can handle the publish of assets to GitHub releases; I'm still looking into alternatives for other VCS providers, though it would be completely possible to split out that functionality from Python Semantic Release into another tool.

I'm not saying this is definite, by any means, but would like to get a feel for whether or not keeping this complexity is worth it given that there are other, focused tools out there which could do those jobs. If the general feeling is that actually, it's just as easy to use alternative tools to replace publish, then I think it would be worth adding a page in the docs on recommended replacements for that functionality (e.g. use gh-action-pypi-publish as follows: "..." for more info see <link>).

bernardcooke53 avatar Feb 28 '23 20:02 bernardcooke53

IMO it makes sense to remove the publish command if we aren't adding any extra functionality compared to using twine directly.

danth avatar Mar 01 '23 08:03 danth

Hey @afuetterer, thanks a lot for testing the pre-release out and reporting those issues! I have created a branch with fixes for those items. Your minimal examples are extremely helpful in debugging and testing fixes out, thanks for those too. Regarding the templates folder - it'd be interesting to hear whether or not you think it's useful. I wrote some code doing essentially that a while ago so I thought I would add it as a feature, the rationale being that it's possible to store e.g. macros in a separate file to the CHANGELOG, and to render other files (e.g. AUTHORS.md) during a release. But it might be overkill and I'm not opposed to removing it if it's not useful

One thing that this has really highlighted is that the test coverage is pretty poor - particularly around the version command - I had some trouble getting click.CliRunner into the tests to hit the command-line itself, and ought to factor that out into more testable units. I'll try and batch that in with the fixes to "prove" they're working, just leaving the thoughts on this thread as mini-update for where the effort is going next.

Thanks a lot for fixing this so quickly @bernardcooke53. I used the branch as a git dependency in my pyproject.toml to test the fixes. Both issues I reported earlier are fixed (https://github.com/afuetterer/minimal-example).

I am glad that the minmal examples were useful so far. I will report any future bugs in this issue.

As for your question about the templates folder: I find it extremely useful to be able to customize the changelog generation per project.

Regarding version vs. publish: So far I only used the version command to bump, tag and publish a GitHub release. Publishing to PyPI can be easily done with poetry, twine, gh-action-pypi-publish. I second @danth there.

afuetterer avatar Mar 01 '23 09:03 afuetterer