-
Notifications
You must be signed in to change notification settings - Fork 1
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
ENH Update reference to supported modules data #45
ENH Update reference to supported modules data #45
Conversation
de66d65
to
91210a7
Compare
funcs_scripts.php
Outdated
if (isset($repoData['type'])) { | ||
return $repoData['type'] === 'recipe'; | ||
} |
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.
The type
key is only available for repos in the "supportedModules" category - but e.g. testing-recipe is a recipe so we need to still check the old way as well.
Same idea with all other changes below checking type
.
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.
Why do we even need to use the supported-modules repositories.json as the source of truth instead of composer.json of each module? Nothing was broken before?
Ditto for all the other updates in the PR doing the same e.g. is_module()
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.
Just feels more correct. I'll remove it if you insist, but I don't see any reason to.
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'd say remove it, it's confusing as to why we'd need two methods.
Having it here and then having a fallback of composer.json looks just like we don't trust supported-modules to be correct
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 just like we don't trust supported-modules to be correct
A comment could easily solve that lol. I'll remove it just to avoid ping pong, I think either way the result ends up being the same so it doesn't matter.
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.
done
global $MODULE_DIR; | ||
return !is_module() && strpos($MODULE_DIR, '/gha-') !== false; | ||
global $GITHUB_REF; | ||
return in_array( | ||
$GITHUB_REF, | ||
array_column( | ||
MetaData::getAllRepositoryMetaData()[MetaData::CATEGORY_WORKFLOW], | ||
'github' | ||
) | ||
); |
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.
All gha-* repos are in the workflow category, and nothing else is.
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.
Should revert this, this one is like is_module() where we're not using MetaData as source of truth. This function is specifically for gha repos, and the naming convention on gha- will always be there, though there is some chance someone will put in a non-gha workflow file in the workflow section of repositories.json.
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.
The reason we're not using MetaData
in is_module()
is because there might be repositories in other categories which are also modules, so we need to fall back on composer.json values.
This function is different. There's no need to fall back on anything, everything we need to know is in the CATEGORY_WORKFLOW
category. All of the repositories in that category are the "gha-*" repositories. Nothing else is in that category. This is robust, and is much better than checking against a naming convention.
there is some chance someone will put in a non-gha workflow file in the workflow section of repositories.json.
If they do that, they're doing the wrong thing.
The same can be said of naming conventions - there is a chance someone could make a workflow repository that doesn't follow the naming convention, or could make another repository that accidentally starts with "gha-".
Both of those are a low chance and aren't worth considering IMO.
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.
For context, the whole reason that category even exists in the file is so you can tell if something's a gha repo by just checking if it's in that category.
$s = read_file('.git/config'); | ||
if (!preg_match('#github.com:([^/]+)/#', $s, $matches)) { | ||
error('Could not determine github account'); | ||
} | ||
return $matches[1]; | ||
global $GITHUB_REF; | ||
return explode('/', $GITHUB_REF)[0]; |
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.
No need to check .git
- we have this information ahead of time.
function extra_repositories() | ||
{ | ||
$importantRepos = [ | ||
'silverstripe/markdown-php-codesniffer', | ||
'silverstripe/silverstripe-standards', | ||
'silverstripe/documentation-lint', | ||
'silverstripe/.github', | ||
]; |
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.
The set of "extra" repos we're gonna end up with after this change is a lot bigger than we were getting before. I've added a note to silverstripe/.github#242 that when this gets run next we should take care to check if there's anything we want to skip and add an exclude list if there is.
// Some repositories don't have a valid matching CMS major | ||
$ignoreCMSMajor = [ | ||
'/silverstripe-simple', | ||
'/markdown-php-codesniffer', | ||
]; | ||
foreach ($ignoreCMSMajor as $ignore) { | ||
if (strpos($MODULE_DIR, $ignore) !== false) { | ||
return CURRENT_CMS_MAJOR; | ||
} | ||
// This repo matches every major and matches start at the lowest - but we only want the highest stable. | ||
if ($GITHUB_REF === 'silverstripe/silverstripe-simple') { | ||
return MetaData::HIGHEST_STABLE_CMS_MAJOR; | ||
} |
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.
markdown-php-codesniffer will get correctly identified - but the theme still needs some help.
91210a7
to
f7d6f2d
Compare
funcs_scripts.php
Outdated
if (isset($repoData['type'])) { | ||
return $repoData['type'] === 'recipe'; | ||
} |
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.
Why do we even need to use the supported-modules repositories.json as the source of truth instead of composer.json of each module? Nothing was broken before?
Ditto for all the other updates in the PR doing the same e.g. is_module()
f4c0803
to
8123501
Compare
funcs_scripts.php
Outdated
if (isset($repoData['type'])) { | ||
return $repoData['type'] === 'recipe'; | ||
} |
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'd say remove it, it's confusing as to why we'd need two methods.
Having it here and then having a fallback of composer.json looks just like we don't trust supported-modules to be correct
8123501
to
d80e9f7
Compare
global $MODULE_DIR; | ||
return !is_module() && strpos($MODULE_DIR, '/gha-') !== false; | ||
global $GITHUB_REF; | ||
return in_array( | ||
$GITHUB_REF, | ||
array_column( | ||
MetaData::getAllRepositoryMetaData()[MetaData::CATEGORY_WORKFLOW], | ||
'github' | ||
) | ||
); |
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.
Should revert this, this one is like is_module() where we're not using MetaData as source of truth. This function is specifically for gha repos, and the naming convention on gha- will always be there, though there is some chance someone will put in a non-gha workflow file in the workflow section of repositories.json.
Needs silverstripe/supported-modules#29
Issues