-
Notifications
You must be signed in to change notification settings - Fork 172
RFC 7 Monty Versioning #186
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
RFC 7 Monty Versioning #186
Conversation
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.
LGTM, optional suggestions.
rfcs/0000_monty_versioning.md
Outdated
|
||
* We want the current version to be in the source code. | ||
* The `main` branch is protected and changes can be pushed only via Pull Requests. | ||
|
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.
I would add a constraint that it needs to be programmatically accessible for things like the print_version tool. https://github.com/thousandbrainsproject/tbp.monty/tree/main/tools/print_version
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.
Added a constraint "We want the current version to be programmatically accessible, e.g., by the tools.print_version.cli
tool." in fb95a0c
rfcs/0000_monty_versioning.md
Outdated
|
||
# Future possibilities | ||
|
||
If we were to adopt the [Conventional Commits specification](https://www.conventionalcommits.org/en/v1.0.0/#specification) then the version number updates could be automated based on the commit message. |
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.
We could also put this in the tbp.cli
tool which would take care of most of the manual steps.
tbp bump-minor
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.
I added a future possibility that "The tbp
CLI could be extended to include a version
command that prepares a Pull Request to update the version number." fb95a0c
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.
Nice, thanks for outlining this! I like the server proposal. Just one question on the process for updating the version.
rfcs/0000_monty_versioning.md
Outdated
|
||
### Process | ||
|
||
1. Create a Pull Request that ***only*** updates the `__version__` variable in [`src/tbp/monty/__init__.py`](https://github.com/thousandbrainsproject/tbp.monty/blob/main/src/tbp/monty/__init__.py). |
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.
Can you elaborate why we would want a PR with only that change instead if including the version number update in the PR with the change that triggers the version update? If we have a PR with a breaking change + a PR with the version number update, which PR would be merged first? I would assume we don't want to add the breaking change into main under the old version. So then we would increment the version before introducing the change? Why couldn't we increment the version in the same PR as the change and have the discussion on whether the change warrants a version update there?
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.
I think of it as the process of assigning Monty a version being separate and happening at a different cadence from the process of making changes to Monty.
Some considerations:
- We will want to bundle multiple breaking changes into the same version.
- We will want to bundle a breaking change and some patches together.
- We will want to use version update as a trigger to create a new release / publish Monty.
Also, I added a note explaining what "current version" means, which may have contributed to my lack of clarity.
Note
"Current version" can have a counterintuitive meaning due to different interpretations of what "current" means. For the purposes of this RFC, "current" is (somewhat circularly) defined as the version number in the source code.
The future workflow will essentially be as follows:
- commit
- commit
- commit
- commit
- review all commits above and update the version accordingly, which publishes Monty
- commit
- commit
- review all commits above and update the version accordingly, which publishes Monty
To facilitate this workflow, we'd want to have version-only PRs, which would also trigger our release / publish workflows.
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.
I also added this future possibility in 7ff85b6
In the future, integrating into the main
branch may become cumbersome if the pace of commits to main
is high. In that case, we will update our versioning strategy. One way to do this is to create a release
branch that is used for releasing new versions. This note is intended to highlight the possibility, but if we get there, we will get into the details then.
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.
Looks great, thanks for putting this together!
Just two high level comments/questions
- Is it possible to elaborate a bit how historical versions will be accessed (e.g. if someone wants to run something from the DMC paper, and so access v0.0.1)? Would this require some use of PyPi or the like?
- I don't have a good suggestion, and maybe there's no solution, but do you have any thoughts on how we can help us to remember to update the version number when merging PRs into
tbp.monty
? E.g. an automated (but non-blocking) email / message whenever a PR is opened fortbp.monty
? We might eventually get in the habit, but it feels like we may merge several PRs before suddenly remembering that we need to increment the version number.
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.
LGTM. I was literally going to ask if we are going to auto-update via Conventional Commits and/or Github Actions, but you have read my mind and answered them. :)
To @nielsleadholm:
Just my thoughts on the above question...(hopefully I am understanding the question correctly). For accessing historical versions, e.g. let's say someone wants to access If we want to check out a historical version from GitHub, we can tag the releases and do something like
I think your question may be answering @vkakerbeck's question on why the PR update for versions should be separate! 😄 If there is a PR that just updates the version, then your own PR will have to be rebased on top of that, so one does not need to manually update / remember to update the version number, as long as the PR is "up-to-date" with main branch (as all PRs are currently doing). |
I'm adding the following to the RFC in bc351c6, which is intended to answer the above. Accessing specific versionsThe most detailed and specific way to access a specific version of If we tag the version commit with the corresponding version number, then the version can be accessed by checking out the tag. GitHub has releases and tags pages that list the tagged versions. If we publish Monty, then the specific published version could be installed via a package manager by specifying the published version. |
I don't think we should update the version with every commit. It will be OK and even desirable to merge several PRs before updating the version number. I commented above regarding what I expect the workflow to be. |
FYI, I added some more context around It is worth noting that the We will use |
@codeallthethingz, I want to explicitly ask if what I proposed here will work with how we currently use readme.io. I imagine that if we go from |
That is the current behaviour. We truncate to the minor version for the docs. |
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.
Sounds good :) Thanks for the clarifications & updates!
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 for the clarifications, looks great!
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 @tristanls, looks great!
FYI, added expected workflow section from the comment to the RFC Expected WorkflowBy adopting this versioning scheme and process, the expected workflow is as follows:
|
Motion for Final Comment PeriodI motion for the Final Comment Period with the disposition to merge this RFC. I interpret @codeallthethingz, @hlee9212, @vkakerbeck, @nielsleadholm, and @ramyamounir current PR acceptance as approval.
|
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.
LGTM. I've not worked with GitHub releases and tags, but it sounds clean. Can you clarify what is meant by "if we publish Monty..."? I'm assuming that means putting on PyPi or conda-forge.
@scottcanoe, correct. Currently, I'm thinking conda. I will update the RFC to be more specific. |
elaborated on release branches option
bb3870e
to
4ee4518
Compare
This concludes the Final Comment Period, with all maintainers approving to merge. The PR will be merged shortly after the final pre-merge changes. |
For easier reading, see final RFC https://github.com/thousandbrainsproject/tbp.monty/blob/main/rfcs/0007_monty_versioning.md