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

Release v2025.1 #881

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

Release v2025.1 #881

wants to merge 17 commits into from

Conversation

matthew-white
Copy link
Member

This PR prepares the release of v2025.1. It should only contain changes from other PRs that have already been approved and merged (and possibly merge commits from the master branch).

spwoodcock and others added 9 commits January 21, 2025 14:45
If check-migrations fails with non-1 exit code, the server will continue to start.

The existing check-migrations call and test for exit code 1 was originally introduced as a fix for #461.  The change in this PR should be more robust than the existing approach, but `set -e` and `set -o pipefail` might provide even more safety in handling failures in this script.  These latter options would also allow for complete removal of `make check-migrations` from production.

There may also be other critical scripts in this repo which would benefit from similar changes.
This change brings consistency: all other executables in `files/**` are marked executable in git rather than `chmod`'d in `.dockerfile` files.

The `chmod` call was originally introduced in #676 without discussion.

There is a wider debate whether git can be trusted to manage file permissions, touched on at #830 (comment)
Progress towards #677 - this will eventually allow the GHCR upload job to depend on the outcome of tests previously run in CircleCI.
Noted while working on #882: `files/postgres/upgrade-postgres.sh` is not actually run with `/bin/bash -eu` as its shebang declares, but with `/bin/sh`.

-----

This commit

1. fixes the shell used for `upgrade-postgres.sh`
2. introduces a test which would have detected this issue, and should prevent similar issues in future
3. fixes use of deprecated dockerfile syntax detected by the new test
4. pulls `shellcheck` invocation into a script file to allow easier use locally, and possible future extension

-----

Docker build checks

Currently in beta, "docker build checks" are best-practice lint checks for dockerfiles.

See: https://docs.docker.com/build/checks/#check-a-build-without-building

-----

Dockerfile fixes

* `postgres-upgrade.dockerfile`: https://docs.docker.com/reference/build-checks/json-args-recommended/
* `postgres14.dockerfile`: https://docs.docker.com/go/dockerfile/rule/legacy-key-value-format/

-----

Related: #342
Allowing access to enketo's redis instances locally allows this docker-compose environment to be used to work on enketo-express as well as other central services:

1. run in this repo:

	make stop && make dev && docker compose stop enketo

2. run in `enketo` repo:

	yarn workspace enketo-express grunt develop
@matthew-white
Copy link
Member Author

@alxndrsn, GitHub notes that CircleCI is failing here. We've gotten rid of CircleCI, right? Is there a config change that needs to happen in the GitHub settings for the repo?

@alxndrsn
Copy link
Contributor

Is there a config change that needs to happen in the GitHub settings for the repo?

That sounds right, but I can't find where to (un)configure the check. Maybe it needs to be deactivated through the CircleCI UI?

@matthew-white
Copy link
Member Author

Hmm I think you might be right. If it were configured within GitHub, I'd expect to see it under branch protection rules, but I'm not seeing CircleCI there. I am seeing an option within CircleCI to "Stop Building", maybe that's it. But I think we should probably hold off on that until we're closer to the release of v2025.1. If we were to put out a patch release before then, we'd still rely on CircleCI for that, so I don't think we should turn off CircleCI completely until we're sure that we're done with patch releases.

matthew-white and others added 8 commits February 21, 2025 10:37
In commit b2a9d4f, the directory name containing the nginx tests was changed from `test` to `nginx`.  `docker compose` uses the parent directory name as the default name for containers, so this change broke log output which refers to specific docker logs by container name.

The approach in this commit should be more robust as it uses the docker compose service names instead of directory-derived container names.
Whitespace changes are often introduced inadvertently due to different editor configurations.  They then add noise to PRs, and if committed, add noise to future diffs and the git log.

This PR adds checks for the following in `.sh` files:

* trailing spaces on lines
* trailing newlines in files
* tabs vs spaces
In some circumstances, `npx` will transparently install packages when asked to run a command.

From https://docs.npmjs.com/cli/v8/commands/npx#description:

> If any requested packages are not present in the local project dependencies, then they are installed to a folder in the npm cache, which is added to the PATH environment variable in the executed process. A prompt is printed (which can be suppressed by providing either `--yes` or `--no`).

Ref vuejs/vue-cli@f36a4ed
* use bash everywhere, instead of a mixture of `sh`, `bash`, and (implicitly) `dash`
* for consistency, always set flags/options:
  * `-e`: "Exit immediately if a pipeline...returns a non-zero status."
  * `-u`: "Treat unset variables...as an error"
  * `set -o pipefail`: "the return value of a pipeline is the value of the last (rightmost) command to exit with a non-zero status"
  * `shopt -s inherit_errexit`: "command substitution inherits the value of the errexit [`-e`] option"

See: https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#The-Set-Builtin
See: https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#The-Shopt-Builtin

In the service container's `start-odk.sh` script, this also removes the need for the explicit `make check-migrations` check, as all steps in scripts are now checked implicitly.

Closes #341
Closes #342
Approach suggested in #818 (comment)

-----

For nginx config, a new approach is implemented with mawk.

This is similar to envsubst, but more ergonomic:

* no need to explicitly list all variables
* throw error on missing variables
* do not replace nginx vars like $host, $request_uri with empty strings (in contrast to envsubst when executed without an explicit variable list)

There are a couple of changes which may break existing deployments:

* changing client-config.json.template
* requiring all substituted variable to be defined

Closes #473
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.

4 participants