Skip to content
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

[CMSP-628, CMSP-680] Update WordPress upstreams from commits [WIP, DNM] #118

Draft
wants to merge 125 commits into
base: master
Choose a base branch
from

Conversation

rwagner00
Copy link
Contributor

@rwagner00 rwagner00 commented Jan 11, 2024

Overview

This pull request:

Full writeup here: https://github.com/pantheon-systems/cms-platform/discussions/63

Resolves https://getpantheon.atlassian.net/browse/CMSP-628

Q A
Bug fix? yes-ish
New feature? no
Has tests? yes
BC breaks? no
Deprecations? no

Summary

Causes normal commits without tags to also trigger the update process.

@rwagner00 rwagner00 requested a review from a team as a code owner January 11, 2024 21:06
Copy link
Contributor

@jazzsequence jazzsequence left a comment

Choose a reason for hiding this comment

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

do we have tests for the WPMS repository? like a fixture in pantheon-fixtures for that? If not, we should make one and ensure that the thing actually does what it says on the box.

@pwtyler
Copy link
Member

pwtyler commented Jan 11, 2024

+1 on WPMS fixture tests.

Copy link
Member

@pwtyler pwtyler left a comment

Choose a reason for hiding this comment

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

I didn't really get into it line by line to give any more specific of a suggestion, but reading through projectUpstreamUpdate, I have the sense the old code checking the version numbers is made redundant by the fact that we're now just checking commits. That could likely just be dropped since the tags are made redundant by that ||. If the tags give us reason to update, so will the commit comparison.

* add wpms tests and fixture
not sure if this will actually work, i guess we run it and find out...

* add multisite tests

* fix comments

* force db drop

* try to foce the db drop

* try to force the db drop...again

* remove the db drop

* try dropping the db via exec

* add a way to get the path from a config file

* use the path to drop the wp database

* add tmate for debugging

* temporarily force tmate to run

* we don't need the commented-out gh auth login

* comment out the conditional

* remove the conditional entirely

* more tmate hacking

* i give up on trying to get tmate to work, debug manually

* try to drop the db a different way.

* remove execute, update exec and exit early

* maybe add some debugging

* pull down a copy of the wpms repo

* remove db drop and exit

* attempt to pass force-db-drop

* an array is definitely not it

* reset to last working commit with only 1 failure & force db drop

* is force-db-drop being passed?

* add forceDbDrop property
which checks the project config for force-db-drop

* remove the var_dump
Copy link
Member

@pwtyler pwtyler left a comment

Choose a reason for hiding this comment

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

Copying over review from #119

tests/ProjectCommandsTest.php Outdated Show resolved Hide resolved
tests/src/Fixtures.php Outdated Show resolved Hide resolved
src/Update/Methods/WpCliUpdate.php Outdated Show resolved Hide resolved
@jazzsequence
Copy link
Contributor

@pwtyler I'm seeing a lot of stuff that would still use the version checking stuff -- if for no other reason than for update notifications that are triggered by tags. If we removed all of it, I think we would break a lot of things. The change @rwagner00 made in 3679b2d are probably sufficient so we aren't requiring that to trigger updates.

@jazzsequence jazzsequence requested a review from pwtyler January 23, 2024 19:10
now that I've synced the histories up
even though I'm not sure what it does
since we removed main branch
this will tell us if the $current === $latest logic is failing
we shouldn't need it but i'm grasping
and remove debug code
@jazzsequence jazzsequence changed the title CMSP-628 WPMS Updates [CMSP-628, CMSP-680] Update WordPress upstreams from commits Feb 1, 2024
@jazzsequence jazzsequence changed the title [CMSP-628, CMSP-680] Update WordPress upstreams from commits [CMSP-628, CMSP-680] Update WordPress upstreams from commits [WIP, DNM] Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants