Repository navigation
env: fall back to last known WordPress version if the mirror hasn't synced the new tag yet - #81534
josephfusco wants to merge 11 commits into
Conversation
github-actions
Bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @josephfusco! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…'t synced the new tag yet The stable-check API on WordPress.org reports a new version the moment it ships, but the WordPress/WordPress git mirror wp-env fetches from syncs separately and can lag by hours. During that window wp-env tries to fetch a tag that doesn't exist yet and fails outright, rather than using the previous, still-valid version.
… to use The previous fallback only helped when a wp-env cache already existed from an earlier run. On a fresh environment with no prior run -- a CI runner, most notably -- there was nothing to fall back to and the original failure still occurred. This adds a last-resort path: list the mirror's actual tags and use the highest one instead of the version stable-check reported but the mirror does not have yet.
Sort a copy rather than the source array before popping its max, so a future reader doesn't have to verify mirrorVersions is unused afterward to know it's safe. Also makes the last-known-good-version test use a cached value the mirror list doesn't contain, so it proves the cache wins outright rather than coincidentally matching the mirror's own highest tag.
f4e0bc8 to
88606e5
Compare
desrosj
left a comment
desrosj
left a comment
There was a problem hiding this comment.
I'm not convinced that this is a good path forward.
Personally, I think that seamlessly falling back to a non-latest version when latest is used is a bug, not a feature. Unless someone indicates that they'd like to force download the most recent available release through a flag or configuration value, this could possibly result in unexpected behavior and outcomes. This would especially be true if say latest is expected to correspond to 7.1, but instead 7.0.4 is all that is available.
I also don't know how I feel about serving an insecure version of WordPress when a secure one is unavailable, which is what would have happened in this case. Sure, wp-env is intended to be used for local environments and running tests, but there is also a non-zero level of risk with using an insecure version of WordPress. I understand that this tool is only used for running tests and local environments, but my preference for a tool like this is for it to be less opinionated while still being easy to use and configure.
I have not reviewed the actual code here at a per-line level, and I'm not saying this should be abandoned and never merged. However, I did want to play devil's advocate and voice this for everyone to consider.
|
I also considered recommending that the tool switch off of the mirror on GitHub in favor of using the w.org Git repository ( I opened #81553 to output some informative notices when an |
The fallback only fires during the sync-lag window, a few hours after each release, not as a general use whatever's available policy.
Good call out. Worth weighing against the cost of the status quo though. This exact failure is currently blocking CI on a dependabot PR in a real plugin (WordPress/presence-api#272), and dependabot's whole purpose is shipping dependency security fixes. A hard-failing wp-env doesn't avoid risk, it just moves it, from briefly running a WordPress core that's a few hours behind on mirror sync, to leaving an unrelated security-relevant PR unmergeable. |
|
I agree with @desrosj's points here.
|
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
| jest.mock( 'dns', () => ( { | ||
| promises: { | ||
| resolve: jest.fn(), | ||
| }, | ||
| } ) ); | ||
| jest.mock( 'got', () => jest.fn() ); | ||
| jest.mock( 'simple-git', () => jest.fn() ); |
There was a problem hiding this comment.
Mocking externals makes me anxious. I'd be more open to our implementing interfacing functions that we can then mock more confidently. For example:
+export function listWordPressRemotes() {
+ return SimpleGit().listRemote( [
+ '--tags',
+ WORDPRESS_MIRROR_URL,
+ ] );
+}
async function getMirrorVersions() {
try {
- const output = await SimpleGit().listRemote( [
- '--tags',
- WORDPRESS_MIRROR_URL,
- ] );
+ const output = await listWordPressRemotes()
Likewise, introducing fetchStableCheck.
I don't love it, but it seems like a lesser evil.
There was a problem hiding this comment.
Thanks for the feedback, appreciate the thorough review.
parse-config.js and config-integration.js already mock got and simple-git directly, in this same package. Wrapping only this file's calls would leave three different mocking approaches to the same two libraries in packages/env, one more inconsistency, not less. If this is worth doing, it reads as a package-wide test convention change rather than something scoped to this PR.
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
…mirror listing failure
|
The On prompting and rejecting when non-interactive: since CI is non-interactive, that reduces to just rejecting, which is the exact failure this PR removes. Restricting the fallback to non-insecure versions has the same problem in practice, everything the mirror has synced in the case that started this is currently marked insecure, so there'd be nothing left to fall back to. |
| const fallback = [ ...mirrorVersions ] | ||
| .sort( compareWordPressVersions ) | ||
| .pop(); |
There was a problem hiding this comment.
Let's nevertheless exclude insecure releases. Grosso modo:
const versions = await got(
'https://api.wordpress.org/core/stable-check/1.0/'
).json();
+const secureVersions = new Set(
+ versions.filter( version => /* exclude those marked "insecure" */ )
+);
const fallback = [ ...mirrorVersions ]
+ .filter( secureVersion.has )
.sort( compareWordPressVersions )
.pop();
mcsf
left a comment
mcsf
left a comment
There was a problem hiding this comment.
Thanks for addressing my feedback, @josephfusco. I think this only needs the exclusion of insecure releases; even though in practice we're unlikely to match them, it's the right thing to do. :)
This branch has not been deployed
3 participants
Fixes #81536.
What
getLatestWordPressVersion()returns whatever version the stable-check API reports as latest, then immediately tries to fetch that version's tag from theWordPress/WordPressmirror.Why
The mirror sync lags the stable-check API by hours. During that window the fetch fails with
fatal: couldn't find remote ref <version>. Confirmed live at the time of writing: the API reports7.0.4, the mirror has no7.0.4tag.How
List the mirror's actual tags before trusting the reported version. If it's missing:
Testing Instructions
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Sonnet 5
Used for: Assisting with the fix and tests