-
Notifications
You must be signed in to change notification settings - Fork 13
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
Upgrade notes rewrite #99
Conversation
7436581
to
1381e9a
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
95d2176
to
12f183d
Compare
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, @andrewnicols. Great stuff working on these changes! Well done!
I think this is almost ready.
Just noticed some minor things...
Readme
We need to update the readme file about the requirements (PHP 8.2+ and running composer install
)
Unit test
There are some calls to assertNull()
for methods with void
return. Although they're passing, I'm not sure if it's appropriate.
- HelperTest::testRequireBranchNameValid()
- HelperTest::testRequirePathValid()
- HelperTest::testRequireVersionFileValid()
Existing issue
Possibly as a separate issue, this might also be a good opportunity to improve/fix the --type rc
release. I noticed that (even before this PR) if a number is not provided, the release script does not seem to recognise that it's an RC release type and performs a major version release type, leading to the following output.
Starting pre-release processing for rc release.
...
Pre-release processing has been completed.
Changes can be reviewed using the --show option.
Please propagate these changes to the integration repository with the following:
git push origin main
git tag -a 'v4.5.0' -m 'MOODLE_405' 23d349419204162f9fd280d702f11bd5021696a6
Once CI jobs have ended successfully, you can safely push the release tag(s) to the integration repository:
git push origin --tags
We could either do the following:
- Option 1: Exit with an error message that the RC number is required
- Option 2: The RC number will fall back to 1 if a number is not supplied
We'd probably need to validate the supplied RC number to ensure we avoid errors.
But yeah, I think we can just fix this on a separate issue. So, I'll leave it up to you.
12f183d
to
99aa5d8
Compare
Updated to add the composer command. I've also amended the lock file to support PHP 8.1 too.
I considered this when I wrote them and decided that this was currently the least worst evil. These methods either throw an exception or return void. I could make them return I've changed the return val to bool. We can change it to true later.
Let's raise an issue for that and fix it later. We should be basing a lot of these transitions on the current value and either erroring or warning on invalid results already. |
Pinging @junpataleta |
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.
Awesome work, @andrewnicols ! I think this is good to go!
This is a fairly comprehensive rewrite of the version.php handling in the release tool.
I've added copious amounts of unit tests which cover most situations.
In order to correctly generate the upgrade notes we need to specify the version that we will be generating for. To do that we need to determine what the next version is easily, without bumping the version immediately.
The easiest way to do so is to specify the target version as an argument to the upgrade notes tooling.
This set of changes completely refactors the
bumpversions.php
file to break out the reading, writing, and checking, into Helper and VersionInfo classes which are themselves thoroughly unit tested.Note: This also adds support for Moodle 4.3 => Moodle 5.0 transitions.