| [07:02:10] | <mstenta[m]> | symbioquine paul121 We may have a slight bug in our `deliver.yml` logic... 🤔 |
| [07:02:38] | <mstenta[m]> | I only discovered it because I'm setting up for the `3.x` -> `4.x` switch |
| [07:02:42] | <mstenta[m]> | https://github.com/farmOS/farmOS/actions/runs/14388271892/job/4035077420... |
| [07:04:56] | <mstenta[m]> | We start by building the Docker images and then caching them so we can reload them in later steps... |
| [07:05:16] | <mstenta[m]> | ... but we're caching/loading it as farmos/farmos:4.x-dev-amd64, instead of farmos/farmos:4.x-dev |
| [07:05:56] | <mstenta[m]> | When tests run, it starts up containers with farmos/farmos:4.x-dev |
| [07:06:11] | <mstenta[m]> | So tests aren't actually using the cached image... I think? 🤔 |
| [07:08:21] | <mstenta[m]> | Looks like the phpcs/phpstan/rector tests are using the correct image, so those pass. |
| [07:12:24] | <mstenta[m]> | Hmm maybe this sed command isn't working: https://github.com/farmOS/farmOS/blob/f01dd2524911c16fe492d45df50651a7a8... |
| [07:15:46] | <mstenta[m]> | That line was changed recently (via https://github.com/farmOS/farmOS/pull/947/files#diff-af086e6040f0bcb375d...) ... but I don't think that change caused it... |
| [07:19:20] | <mstenta[m]> | I wonder if this has been an issue for a while... but maybe we didn't notice it because effectively it doesn't matter a whole lot. Our tests mount /opt/drupal into the container, so it doesn't really matter what the farmOS code is in the image itself. But it means that if we were trying to run PHPUnit tests against lower-level environment changes (like PHP extensions that were added in a branch), the tests would fail. (I think) |
| [07:23:34] | <mstenta[m]> | <mstenta[m]> "Hmm maybe this sed command isn..." <- Hmm when I run the `sed` command locally (and manually replace the shell variables) it works... but maybe the shell variable replacement isn't happening in actions |
| [07:27:22] | <mstenta[m]> | Ah... yea... so I think the shell var substitution isn't working... it doesn't work locally either: |
| [07:27:29] | <mstenta[m]> | ACTION sent a bash code block: https://matrix.org/oftc/media/v1/media/download/AaUrlilCNyHcZ5zFoiD9Vzue... |
| [07:28:35] | <mstenta[m]> | Ah it's because of the single quotes... :-) |
| [07:31:26] | <mstenta[m]> | ACTION sent a bash code block: https://matrix.org/oftc/media/v1/media/download/AXtZn8LAsNTZmhyFJuEplZLU... |
| [07:33:28] | <mstenta[m]> | Pushed a test commit to https://github.com/farmOS/farmOS/pull/949... let's see if tests pass |
| [08:53:35] | <mstenta[m]> | That worked. I'm going to push this to 3.x and rebase https://github.com/farmOS/farmOS/pull/948 and https://github.com/farmOS/farmOS/pull/949 |
| [09:14:29] | <symbioquine[m]> | Hmmm, I wonder when/how it stopped working... I did test it when I cobbled it together. (I'm not good enough with sed to write that untested in one go 😆) |
| [09:15:01] | <mstenta[m]> | Well it's very likely that we just didn't notice it wasn't working, because it's a pretty edge-y case for it to fail |
| [09:15:27] | <mstenta[m]> | It probably stopped working when we first started using shell vars in the sed command |
| [09:15:34] | <mstenta[m]> | Oh well... easy fix! |
| [09:16:19] | <mstenta[m]> | > because it's a pretty edge-y case for it to fail |
| [09:16:19] | <mstenta[m]> | The `sed` command doesn't cause an error... it just doesn't replace anything, because the replacement pattern wasn't found |
| [09:16:35] | <mstenta[m]> | So it ends up using farmos/farmos:3.x-dev in tests |
| [09:16:49] | <mstenta[m]> | Rather than the image that was built/cached in actions |
| [09:20:15] | <mstenta[m]> | But yea... mysterious... just backtraced the commits and we ALWAYS had a shell variable inside the single quotes |
| [09:20:19] | <mstenta[m]> | Here's the commit that added it: https://github.com/farmOS/farmOS/commit/a9a021c87fe72adc04820662bad34f6c... |
| [09:20:56] | <mstenta[m]> | But I wonder... is it possible to use shell variables in docker-compose.yml's image:? |
| [09:21:32] | <mstenta[m]> | If so, then even if the shell variable wasn't replaced in the initial sed command, perhaps Docker Compose was replacing it when docker compose up ran 🤔 |
| [09:21:50] | <mstenta[m]> | In which case it would have worked... |
| [09:22:14] | <mstenta[m]> | But then my change may have broken the sed command, because it added a shell var in the search... not just in the replace |
| [09:22:44] | <mstenta[m]> | https://github.com/farmOS/farmOS/commit/f01dd2524911c16fe492d45df50651a7... |
| [09:23:43] | <mstenta[m]> | mstenta[m]: Yes I think that explains it... https://serversforhackers.com/c/div-variables-in-docker-compose |
| [09:25:24] | <mstenta[m]> | So essentially... the original sed command was working... but it was replacing farmos/farmos:3.x-dev in docker-compose.yml with $DOCKER_IMG_PREFIX:3.x-dev... and $DOCKER_IMG_PREFIX must be set when the tests get run... so it gets replaced runtime |
| [09:25:46] | <mstenta[m]> | (rather than getting replaced when sed runs) |
| [09:28:33] | <symbioquine[m]> | <mstenta[m]> "Here's the commit that added it:..." <- `sed -i 's%farmos/farmos:3.x-dev%'$DOCKER_IMG_PREFIX:3.x-dev'%g' docker-compose.yml` |
| [09:28:46] | <symbioquine[m]> | That doesn't have any replacements inside the quotes... |
| [09:29:11] | <symbioquine[m]> | The replacements happen in the unquoted portion. |
| [09:34:49] | <symbioquine[m]> | * That doesn't have any bash variable replacements inside the quotes... |
| [09:34:58] | <symbioquine[m]> | * That doesn't have any shell variable replacements inside the quotes... |
| [09:37:00] | <symbioquine[m]> | So sed "sees" the command sed -i 's%farmos/farmos:3.x-dev%symbioquine/farmos:3.x-dev'%g' docker-compose.yml (e.g. when I tested pushes to my own Docker Hub image) |
| [09:37:26] | <symbioquine[m]> | * So sed "sees" the command sed -i 's%farmos/farmos:3.x-dev%symbioquine/farmos:3.x-dev%g' docker-compose.yml (e.g. when I tested pushes to my own Docker Hub image) |
| [10:14:00] | <symbioquine[m]> | <mstenta[m]> "But then my change may have..." <- Yeah, I missed that during review too... I think it should be something like:... (full message at <https://matrix.org/oftc/media/v1/media/download/AZLJV7AGpqgiMNXrBIa3wdDR...) |
| [10:15:37] | <symbioquine[m]> | (Additional single quotes added before and after the new shell variable substitution such that it ends up outside the single quotes.) |
| [10:20:37] | <mstenta[m]> | Ah I see... |
| [10:21:42] | <mstenta[m]> | I misinterpreted the purpose of the single quotes in the middle. I thought it was intending to intentionally include the single quotes in the docker-compose.yml, for extra safety |
| [10:22:14] | <mstenta[m]> | So then the fix I pushed works, but isn't exactly right... because it adds those single quotes in the replacement now |
| [10:23:03] | <mstenta[m]> | Any reason not to use double quotes, and omit all the singles? |
| [10:23:23] | <mstenta[m]> | (Now that we have three vars being replaced, as opposed to just the original one) |
| [10:23:27] | <symbioquine[m]> | mstenta[m]: I actually don't see the fix in the diff... am I looking at the wrong PR? https://github.com/farmOS/farmOS/pull/949/files#diff-af086e6040f0bcb375d... |
| [10:23:48] | <mstenta[m]> | I didn't open a PR |
| [10:24:01] | <symbioquine[m]> | mstenta[m]: Because `%` has meaning to the shell I think... |
| [10:24:05] | <mstenta[m]> | (thought it was just that simple) |
| [10:24:10] | <mstenta[m]> | https://github.com/farmOS/farmOS/commit/63912e7c148e8662ab67d0569b523a51... |
| [10:25:11] | <mstenta[m]> | symbioquine[m]: Not sure I understand. Does that work differently in single quotes vs double? |
| [10:26:33] | <symbioquine[m]> | We're using % in sed as our regex separator since the things we're finding/replacing include the more common / separator. |
| [10:26:55] | <symbioquine[m]> | Maybe there's a better one, but I think I looked originally. |
| [10:27:37] | <mstenta[m]> | Right. But I don't understand what difference the % makes if we use double vs single quotes |
| [10:27:56] | <mstenta[m]> | sed -i 's%...%...%g' vs sed -i "s%...%...%g" |
| [10:28:02] | <mstenta[m]> | Both seem to work? |
| [10:28:48] | <symbioquine[m]> | In single quotes it definitely gets passed to sed as a string. In double quotes it could get expanded... |
| [10:29:05] | <symbioquine[m]> | * In single quotes it definitely gets passed to sed as-is. In double quotes it could get expanded... |
| [10:30:17] | <symbioquine[m]> | It might actually be okay the way you're proposing too. |
| [10:30:18] | <mstenta[m]> | Oh... % can get expanded? Didn't realize that gets treated specially by bash |
| [10:30:30] | <symbioquine[m]> | Hmmm, I'm less sure now. |
| [10:31:15] | <mstenta[m]> | Well, would it be OK if we keep the change I made (https://github.com/farmOS/farmOS/commit/63912e7c148e8662ab67d0569b523a51...) but push one more that removes the single quotes? |
| [10:31:36] | <mstenta[m]> | So we end up with: |
| [10:31:36] | <mstenta[m]> | `sed -i "s%farmos/farmos:${FARMOS_DEV_BRANCH}-dev%${DOCKER_IMG_PREFIX}:${FARMOS_DEV_BRANCH}-dev-${{ matrix.platform }}%g" docker-compose.yml` |
| [10:35:37] | <symbioquine[m]> | Yeah, I think that should be okay. (Should be a single commit though IMO since the version with the double and single quotes is kinda weird.) |
| [10:35:58] | <mstenta[m]> | Yea I know... hindsight 20/20 :-/ |
| [10:36:18] | <symbioquine[m]> | Sorry that was confusing 😮💨 |
| [10:36:30] | <mstenta[m]> | I basically introduced single quotes to the docker-compose.yml... so this next commit will simply remove those |
| [10:36:48] | <mstenta[m]> | Thanks for the brain powers! |
| [10:37:29] | <symbioquine[m]> | (mix of actual knowledge and superstition about how bash works 😄) |
| [10:37:38] | <mstenta[m]> | lol always |
| [10:38:53] | <symbioquine[m]> | It looks like the two special meanings of % in bash are modulo arithmetic and trimming during certain string variable expansions... |
| [10:40:17] | * farmBOT has joined #farmos |
| [10:47:05] | <mstenta[m]> | fix: https://github.com/farmOS/farmOS/commit/03607f85190d3b3895fcb153d9de0594... |
| [10:47:47] | <mstenta[m]> | symbioquine: while I have you, this is an easy one to review... :-) https://github.com/farmOS/farmOS/pull/948 |
| [17:28:17] | <mstenta[m]> | OK this is ready for review! https://github.com/farmOS/farmOS/pull/949 |
| [17:28:25] | <mstenta[m]> | cc paul121 symbioquine |