search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Fix PR sandboxes provisioned with PHP 7.2 instead of 8.1 #6133

Closed klonos closed 1 year ago

klonos commented 1 year ago

We switched our PR sandboxes to be using PHP 8.1 in #5926. However, for reasons that seem upstream, our PR sandboxes are being provisioned with PHP 7.2 when using the tugboatqa/php:8.1-apache image.

I have tested this in a "dummy" PR. and switching to tugboatqa/php:8.0-apache or tugboatqa/php:7.4-apache provisions the sandboxes with the respective image 👍🏼 ...I have also checked https://github.com/TugboatQA/dockerfiles/blob/main/php/TAGS.md and 8.1-apache is a valid tag, however testing showed that it simply doesn't work.

We had a discussion in Zulip about this with @BWPanda, @indigoxela and @larsdesigns: https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/Core.20PR.20Sandboxes/near/362303731

Testing has confirmed that using one of the image tags that include the OS in their name works, so we should use those instead.

klonos commented 1 year ago

...to confirm this bug:

To confirm that the PR here fixes things:

indigoxela commented 1 year ago

Wild guessing... as this seems to have broken only recently, it might eventually have to do with a change on their side to prepare for the next Debian release (Bookworm will come out soon).

When I take a look at a (still) functioning 8.2 box (image: tugboatqa/php:8.2-apache), phpinfo shows that it's Buster (very old).

Pretty odd: the working example (from this issue PR, tugboatqa/php:8.1-apache-bullseye) uses ... Buster, too. (Debian 4.19.269-1), so there's definitely something weird going on with their images.

herbdool commented 1 year ago

That is odd. In phpinfo, under System it says "SMP Debian 4.19.269-1" and under Build System it says "SMP Debian 5.10.106-1". Both are really old according to https://www.debian.org/releases/. @indigoxela am I reading that correctly? I wonder if the build defaults to a version if the specified version isn't working properly?

avpaderno commented 1 year ago

Debian 4.19.269-1 means Debian with Linux Kernel 4.19, which is Debian "Buster" 10. The date given after Debian 4.19.269-1 (2022-12-20) helps to understand that: Debian "Etch" 4.0 has been released on 2007; its latest release was 4.0r9, released on 2010.

klonos commented 1 year ago

@herbdool @indigoxela I am currently concerned about the fact that our PR sandboxes are running (have been running) on PHP 7.2 for a while - not about the OS on the sandboxes being old/unsupported/outdated.

So yes, there might be a problem with the tugboat images, and it should be sorted upstream, on their end. Also yes, ideally we should be running on modern/up-to-date OS if possible, BUT that shouldn't be a requirement for our PR sandboxes, as these are short-lived and purpose-built - NOT production sites. What I want us is to have people testing on the latest version of PHP that has the most chances to cause issues (because it's newer, and not "battle-tested" with Backdrop). That version at the moment is 8.1, and once we have core compatible with 8.2, we should switch the sandboxes to 8.2 as well.

With the above in mind, do you still object to merging the PR here, in order to get things done? (rather than wait for the images to be fixed properly upstream)

indigoxela commented 1 year ago

With the above in mind, do you still object to merging the PR here..

@klonos Oh, that's a misunderstanding, I don't object at all, just wanted to share some of my findings. :wink:

Sure, if we find a workaround for a problem, which really seems to be on their end, we should use it – to have the php version we'd expect and need in our PR sandboxes.

klonos commented 1 year ago

Thanks @indigoxela 👍🏼

@herbdool was that the reason you've set the issue back to NW, or did you have further concerns with the PR?

klonos commented 1 year ago

Setting labels back to NT and NCR for visibility (there's nothing that indicates NW, but please feel free to change back if there's actually anything that needs to be done).

quicksketch commented 1 year ago

I was working with Tugboat support today and they suggested that we rebuild the 1.x preview, since configuration changes to the base preview might not take effect until the preview is rebuilt. I rebuilt the 1.x base preview (and all previews after that). But that didn't work and all sandboxes still show PHP 7.2.34. Since @klonos's PR demonstrates getting onto PHP 8.1 correctly, I think we should go with that. If it can be demonstrated that the tugboatqa/php:8.1-apache image works to get PHP 8.1, we can switch back and not couple ourselves to a particular version of Debian.

Because rebuilding caused a number of merge conflicts and our project to go over quota, I had to delete a lot of the current sandboxes. Many PRs may need to be closed/reopened (or have conflicts resolved) to get their sandboxes back.

quicksketch commented 1 year ago

I merged https://github.com/backdrop/backdrop/pull/4451 to get our sandboxes running the latest fully supported version. Thanks everyone for their feedback and help here.

Maybe when we bump to PHP 8.2 we can try removing the Debian version specifier again.

I'll issue another base preview rebuild for the sandboxes that remain to get them onto PHP 8.2.

klonos commented 1 year ago

Maybe when we bump to PHP 8.2 we can try removing the Debian version specifier again.

Definitely. The PR we have for #5927 is provisioned properly with 8.2.