Refactor `update.sh` and add `versions.json`

PhrozenByte commented 1 year ago

Refactor update.sh to better incorporate the new versions.json (see below). The changes are rather minor, they just look like a rewrite due to general code cleanups and moving stuff around. Simply check the commits, you can review all changes on a per-commit-basis. This includes the following changes:

fb5c1a2104b1b6ae9c69dec9721627326df94ad4 Increase Bash's error verbosity by running with set -eu -o pipefail (instead of just set -e)
dbfffe50c79270c4cf7114a5a22a02a1fbbe9f9b Move the main functionality to the create_variant function
f650628a767be59159f10b9f9c18821235fb2d12 + 5239485cfc6ed1099b15d634dc9431ddabb938ab Bake download URL, version, sha256 and GPG key into Dockerfiles
4a5875f495eaa6747f8f597a90f9e94450ca3ed8 Add config variable for PHP version to update.sh
5ee9a9ced5ead717097ca4e6933a7783fa0e3d02 Add 'DO NOT EDIT' note to generated Dockerfiles
2cb9a6214e1f4debe31631ad80d757136a5624f0 Some general code cleanup

The main feature of this PR is adding a new versions.json (e68cbb87af581149bf433be6b94600c0288aa452). versions.json contains information about the latest phpMyAdmin branches (currently just phpMyAdmin 5.2) and container variants available.

It was inspired by PHP's versions.json, but was adapted to better fit phpMyAdmin's needs. The information in versions.json is currently scattered across the repo and often just accessible by humans, e.g. right now it's impossible to reliably determine the latest full version of containerized phpMyAdmin programmatically. The only way is to either fiddle around with sed on the Dockerfile, or to build the container.

versions.json is managed by ./update.sh. There's no need to edit this file manually (even when adding/removing variants and/or versions), just run ./update.sh. See 6d73dc87287963701e601e5d0f8c972f36420a3b for an example.

Changelog was updated accordingly, too (271a3e6a3ed423441726607dfc25d525fc1859a7).

{
  "5.2": {
    "branch": "5.2",
    "version": "5.2.1",
    "sha256": "373f9599dfbd96d6fe75316d5dad189e68c305f297edf42377db9dd6b41b2557",
    "url": "https://files.phpmyadmin.net/phpMyAdmin/5.2.1/phpMyAdmin-5.2.1-all-languages.tar.xz",
    "ascUrl": "https://files.phpmyadmin.net/phpMyAdmin/5.2.1/phpMyAdmin-5.2.1-all-languages.tar.xz.asc",
    "variants": {
      "apache": {
        "variant": "apache",
        "base": "debian",
        "phpVersion": "8.1"
      },
      "fpm": {
        "variant": "fpm",
        "base": "debian",
        "phpVersion": "8.1"
      },
      "fpm-alpine": {
        "variant": "fpm-alpine",
        "base": "alpine",
        "phpVersion": "8.1"
      }
    }
  }
}

PhrozenByte commented 1 year ago

I don't think that the failing test is related to these changes.

williamdes commented 1 year ago

The main feature of this PR is adding a new versions.json (https://github.com/phpmyadmin/docker/commit/e68cbb87af581149bf433be6b94600c0288aa452). versions.json contains information about the latest phpMyAdmin branches (currently just phpMyAdmin 5.2) and container variants available. It was inspired by PHP's versions.json, but was adapted to better fit phpMyAdmin's needs. The information in versions.json is currently scattered across the repo and often just accessible by humans, e.g. right now it's impossible to reliably determine the latest full version of containerized phpMyAdmin programmatically. The only way is to either fiddle around with sed on the Dockerfile, or to build the container.

Can you give us some code to better understand your use case ? This should be a SBOM into the container layer: https://github.com/sudo-bot/docker-phpldapadmin/blob/53338b9feb0096d593d190e9ab637d8ed503f054/docker/Dockerfile#L62-L100

PhrozenByte commented 1 year ago

Can you give us some code to better understand your use case ? This should be a SBOM into the container layer: https://github.com/sudo-bot/docker-phpldapadmin/blob/53338b9feb0096d593d190e9ab637d8ed503f054/docker/Dockerfile#L62-L100

versions.json contains metadata about the repo, i.e. the available container variants and supported versions, not for use inside the container.

PhrozenByte commented 1 year ago

Any updates on this @williamdes @ibennetch?

If you just don't want it, just say so, no need to keep this open then.

If you plan to merge it but want specific changes we've talked about but disagree (like the ENV story above), make a decision and say so, I'll change it then, even when I disagree contentwise, it's your project.

If you need more info (for some more reasoning, see e.g. https://github.com/MariaDB/mariadb-docker/pull/516#issuecomment-1596767023), say so, I'm happy to provide what I can.

williamdes commented 1 year ago

Any updates on this @williamdes @ibennetch?

If you just don't want it, just say so, no need to keep this open then.

If you plan to merge it but want specific changes we've talked about but disagree (like the ENV story above), make a decision and say so, I'll change it then, even when I disagree contentwise, it's your project.

If you need more info (for some more reasoning, see e.g. MariaDB/mariadb-docker#516 (comment)), say so, I'm happy to provide what I can.

Let's be honest, until now I do not see us wanting the change of the Dockerfile and versions.json If you agree I can merge this PR and change the pipe to versions.json to /dev/null so it's easy to add back. And revert the change to the Dockerfiles

You also can do it but with a rebase :)

PhrozenByte commented 1 year ago

Let's be honest, until now I do not see us wanting the change of the Dockerfile and versions.json If you agree I can merge this PR and change the pipe to versions.json to /dev/null so it's easy to add back.

This PR is about adding a versions.json to have some metadata about the repo. Adding it as dead code (like piping it to /dev/null) or requiring a CLI option (even adds the risk of an outdated versions.json) contradicts the purpose, because the required metadata simply isn't there then.

I don't think that investing more work here just to get something merged that is neither used nor wished makes much sense. So I'm closing this now. Thanks for your review.

williamdes commented 1 year ago

Let's be honest, until now I do not see us wanting the change of the Dockerfile and versions.json If you agree I can merge this PR and change the pipe to versions.json to /dev/null so it's easy to add back.

This PR is about adding a versions.json to have some metadata about the repo. Adding it as dead code (like piping it to /dev/null) or requiring a CLI option (even adds the risk of an outdated versions.json) contradicts the purpose, because the required metadata simply isn't there then.

I don't think that investing more work here just to get something merged that is neither used nor wished makes much sense. So I'm closing this now. Thanks for your review.

Okay, the other changes to refactor the file where good ones. Do you mind if I pick them ?

PhrozenByte commented 1 year ago

Okay, the other changes to refactor the file where good ones. Do you mind if I pick them ?

Sure, go ahead :+1: :smiley:

williamdes commented 1 year ago

After random searches, I found out that generate-stackbrew-library.sh seems to reference versions.json. Not sure if it will be required some day

phpmyadmin / docker

Refactor `update.sh` and add `versions.json` #408