NEW Use supported modules repo for branch-based logic #87

GuySartorelli · 2024-04-29T02:38:38Z

Here is a CI run built using this fork: https://github.com/GuySartorelli/silverstripe-framework/actions/runs/8932090672
Notably that produced the same matrix as on a regular run: https://github.com/silverstripe/silverstripe-framework/actions/runs/8896485327

Issue

Merge-ups - logic needs to match gha-merge-ups rhino#17

GuySartorelli · 2024-05-02T03:44:25Z

tests/JobCreatorTest.php

-            github_repository: 'silverstripe/installer'
+            github_repository: 'silverstripe/fake-module'


Note that silverstripe/installer was incorrect here, and all the tests for this are assuming therefore that this is effectively some random repo in the silverstripe org. I felt it's best to preserve those tests as they are and explicitly indicate that this is just a random repo.

The alternative is to rename this to silverstripe/silverstripe-installer here but that fundamentally changes what's being tested. I think it's better that this works for any-old-random-repo rather than that it specifically works for installer.

GuySartorelli · 2024-05-02T03:46:02Z

tests/JobCreatorTest.php

-                'dev-4.10-release'
+                '4.11.x-dev'


I've removed all of the logic for -release branches and removed most scenarios that tested against them - but I've kept this one (and just updated the expected value accordingly) so we do still have some check against what would happen with a branch with that suffix.

GuySartorelli · 2024-05-02T03:48:50Z

job_creator.php

-    private function getBranchName(): string
+    private function getInstallerBranch(): string


Renamed to be clearer what this actually is - it's not the branch name in the repo, it's the name of the branch of installer that will be pulled in via composer.

GuySartorelli · 2024-05-02T03:51:22Z

job_creator.php

-        if (in_array($this->repoName, NO_INSTALLER_LOCKSTEPPED_REPOS) || in_array($this->repoName, NO_INSTALLER_UNLOCKSTEPPED_REPOS)) {
+        if (!$this->needsInstallerVersion()) {


This change exits earlier under any scenario where we don't need installer for this repo.
Previously we had scenarios where we'd get through a bunch of this method only to hit a return '' - now it does most of the relevant checks upfront.

This is mostly unrelated refactoring, but made the rest of it a lot easier to debug and refactor the relevant stuff.

GuySartorelli · 2024-05-02T03:52:15Z

job_creator.php

+    {
+        if (!$this->composerJsonContent) {
+            if (!file_exists($this->composerJsonPath)) {
+                return null;
+            }
+            $this->composerJsonContent = json_decode(file_get_contents($this->composerJsonPath));
+            if ($this->composerJsonContent === null) {
+                throw new RuntimeException('Could not decode composer.json - last error was: ' . json_last_error_msg());
+            }
+        }
+        return $this->composerJsonContent;
+    }


Previously we were decoding the file multiple times at different stages - now we just decode it once, whenever the first time is that we need it, and use that same decoded result every time.

GuySartorelli · 2024-05-02T03:53:37Z

hardcoded.php

Checked this by commenting out the 5.2 in consts and ran this with the 5.2.0 copy of .cow.pat.json

GuySartorelli · 2024-05-02T04:09:21Z

job_creator.php

-            $installerVersions = array_diff($installerVersions, ['4', '5', '6', '7', '8', '9']);
-            // get the minor portions of the verisons e.g. [9, 10, 11]
+            $installerVersions = array_filter($installerVersions, fn ($v) => !ctype_digit((string) $v));
+            // get the minor portions of the versions e.g for ['4.9', '4.10', '4.11'] this returns [9, 10, 11]


Unrelated refactoring that makes this more robust - we don't care about those specific values, we just care whether the value is a major (all digits) or not.

emteknetnz · 2024-05-06T01:39:54Z

action.yml

+          rm ${{ github.action_path }}/composer.lock
+        fi
+        if [[ -d ${{ github.action_path }}/vendor ]]; then
+          rm -r ${{ github.action_path }}/vendor


Suggested change

rm -r ${{ github.action_path }}/vendor

rm -rf ${{ github.action_path }}/vendor

Shouldn't be necessary since we're not doing a --prefer-source - but done nevertheless since it doesn't hurt anything I think.

emteknetnz · 2024-05-06T01:40:44Z

composer.json

@@ -0,0 +1,14 @@
+{
+    "require-dev": {


require-dev should be below require, just cos :-)

That 1000000000% does not matter, but I'll make that change to humour you since I have to touch this file anyway. :p

emteknetnz · 2024-05-06T01:40:47Z

composer.json

+        "phpunit/phpunit": "^9.6"
+    },
+    "require": {
+        "silverstripe/supported-modules": "dev-pulls/main/combine-all-versions"


Suggested change

"silverstripe/supported-modules": "dev-pulls/main/combine-all-versions"

"silverstripe/supported-modules": "dev-main"

Yup, that'll be done once the PR for that fork is merged, so we can easily rerun CI here against that PR to see it go green if changes are needed over there.

emteknetnz · 2024-05-06T01:40:57Z

composer.json

+    "repositories": {
+        "silverstripe/supported-modules": {
+            "type": "vcs",
+            "url": "[email protected]:creative-commoners/supported-modules.git"
+        }
+    }


Suggested change

"repositories": {

"silverstripe/supported-modules": {

"type": "vcs",

"url": "[email protected]:creative-commoners/supported-modules.git"

}

}

Discussion in #87 (comment)

emteknetnz · 2024-05-06T23:23:23Z

@GuySartorelli Remember to create a new minor branch and manually tag

GuySartorelli mentioned this pull request Apr 29, 2024

Merge-ups - logic needs to match gha-merge-ups silverstripe/rhino#17

Closed

GuySartorelli marked this pull request as draft April 29, 2024 02:38

GuySartorelli force-pushed the pulls/1/use-supported-modules-repo branch 9 times, most recently from 14002c3 to a2f5fca Compare May 2, 2024 03:42

GuySartorelli commented May 2, 2024

View reviewed changes

hardcoded.php Outdated

Copy link

Member Author

GuySartorelli May 2, 2024

Choose a reason for hiding this comment

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

Checked this by commenting out the 5.2 in consts and ran this with the 5.2.0 copy of .cow.pat.json

GuySartorelli commented May 2, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/1/use-supported-modules-repo branch 2 times, most recently from 89dcff5 to 840b80e Compare May 3, 2024 00:30

emteknetnz requested changes May 6, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/1/use-supported-modules-repo branch from 840b80e to 4b44f91 Compare May 6, 2024 02:14

NEW Use supported modules repo for branch-based logic

d7e9094

GuySartorelli force-pushed the pulls/1/use-supported-modules-repo branch from 4b44f91 to d7e9094 Compare May 6, 2024 21:49

GuySartorelli marked this pull request as ready for review May 6, 2024 21:49

emteknetnz approved these changes May 6, 2024

View reviewed changes

emteknetnz merged commit 1dfa92a into silverstripe:1 May 6, 2024
4 checks passed

emteknetnz deleted the pulls/1/use-supported-modules-repo branch May 6, 2024 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Use supported modules repo for branch-based logic #87

NEW Use supported modules repo for branch-based logic #87

GuySartorelli commented Apr 29, 2024 •

edited

Loading

GuySartorelli May 2, 2024

GuySartorelli May 2, 2024

GuySartorelli May 2, 2024

GuySartorelli May 2, 2024

GuySartorelli May 2, 2024

GuySartorelli May 2, 2024

GuySartorelli May 2, 2024

emteknetnz May 6, 2024

GuySartorelli May 6, 2024

emteknetnz May 6, 2024

GuySartorelli May 6, 2024

emteknetnz May 6, 2024

GuySartorelli May 6, 2024

GuySartorelli May 6, 2024

emteknetnz May 6, 2024

GuySartorelli May 6, 2024

GuySartorelli May 6, 2024

emteknetnz commented May 6, 2024

		github_repository: 'silverstripe/installer'
		github_repository: 'silverstripe/fake-module'

		private function getBranchName(): string
		private function getInstallerBranch(): string

		if (in_array($this->repoName, NO_INSTALLER_LOCKSTEPPED_REPOS) \|\| in_array($this->repoName, NO_INSTALLER_UNLOCKSTEPPED_REPOS)) {
		if (!$this->needsInstallerVersion()) {

	rm -r ${{ github.action_path }}/vendor
	rm -rf ${{ github.action_path }}/vendor

	"silverstripe/supported-modules": "dev-pulls/main/combine-all-versions"
	"silverstripe/supported-modules": "dev-main"

NEW Use supported modules repo for branch-based logic #87

NEW Use supported modules repo for branch-based logic #87

Conversation

GuySartorelli commented Apr 29, 2024 • edited Loading

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz commented May 6, 2024

GuySartorelli commented Apr 29, 2024 •

edited

Loading