fix: `update-check.sh` should query GH Releases by polarathene · Pull Request #3666 · docker-mailserver/docker-mailserver

polarathene · 2023-11-27T06:45:48Z

Description

We have had 3 concerns encountered related to this feature in the past.

Resolved by:

CI will now provide the VERSION as an image ENV (DMS_RELEASE) in the Dockerfile to build. No need to manually update VERSION (well, until a future release warrants removing it).
Future releases will now check the GH Releases page instead of current master branch VERSION file.

Past concerns addressed:

:edge should not care about the update process, these concerns should not have been raised:
- bug fix: remove exit 0 from update-check.sh #3659
- bug fix: push :edge when VERSION is updated as well #3662 (review)
A release was made without bumping VERSION accordingly:
- The fix was to publish a new patch version/tag.
- Wouldn't be a problem when checking GH releases for tag version.
A concern when the release PR is pushed but tagging an official release is delayed:
- :edge unaffected since it's not actually tied to the /VERSION file that it would compare to Github VERSION.
- Tagged releases would avoid sending notification until a new tagged release is actually published.

Additional context

Applies changes were originally proposed here:

ℹ️ Note:

The curl response will be the tag value, which includes a v to strip away. Whereas the current VERSION file has no v prefix.
VERSION should remain committed for now. Prior versions will still check it, notably v13. Perhaps by v14 or later removing the file would fine? 🤷‍♂️

⚠️ Caution:

I have not verified the workflow.
- I expect the value is correct from the Github Actions context docs + conditional expression example.
- We do have an existing workflow using github.ref / ${GITHUB_REF}, github.ref_name should be just the tag like in the JSON tag_name field.

`yq` vs `jaq`

I have expressed interest in yq previously for managing DKIM / rspamd config (and processing JSON output from step CLI for TLS cert tools):

However it weighs 9.3MB vs an alternative jaq at 1.7MB, which in this case was simpler to query the JSON field for the tag and strip the v prefix character.

See the jaq link for preference of it over jq. Docs wise, if using jaq elsewhere in DMS scripts you can follow the jq docs, or see the jaq README for some additional features/capabilities.

Type of change

Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change that does improve existing functionality)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes
I have added information about changes made in this PR to CHANGELOG.md

Now CI builds triggered from tagged releases will always have the correct version. No need for manually updating a separate file.

Compare to the remote GH release tag published, rather than contents of a `VERSION` file. `VERSION` file remains in source for now as prior releases still rely on it for an update notification.

- Can more easily express a string subslice. - Lighter weight: 9.3M vs 1.7M. - Drawback, no YAML input/output support. If `yq` is preferred, the `v` prefix could be removed via BASH easily enough.

georglauterbach

LGTM 👍🏼

One tiny nitpick (style) 🚀

polarathene

Some review comments for context to the latest changes.

polarathene · 2023-11-28T03:53:01Z

-#### Version
+#### Minimum supported version

-We make use of build-features that require a recent version of Docker. Depending on your distribution, please have a look at [the official installation documentation for Docker](https://docs.docker.com/engine/install/) to get the latest version. Otherwise, you may encounter issues, for example with the `--link` flag for a [`#!dockerfile COPY`](https://docs.docker.com/engine/reference/builder/#copy) command.


I removed the --link example as I think that has a fallback behaviour for compatibility and the information here with link doesn't provide that context.

The HereDocs feature arrived earlier around 2021H2

The COPY --link feature around 2022Q2

Docker v23 was released Feb 2023. It enables BuildKit by default. It makes for a good baseline (for building the image), instead of just "recent version", while being a little vague on earlier version support for brevity.

polarathene · 2023-11-28T03:54:01Z

+- To get the latest version for your distribution, please have a look at [the official installation documentation for Docker](https://docs.docker.com/engine/install/).
+- If you are using a version of Docker prior to v23.0, you will need to enable BuildKit via the ENV [`DOCKER_BUILDKIT=1`](https://docs.docker.com/build/buildkit/#getting-started).

-If you are not using `make` to build the image, note that you will need to provide `DOCKER_BUILDKIT=1` to the `docker build` command for the build to succeed.


This page no longer makes mention of the make build command, it doesn't add much value to the reader so seemed like noise?

polarathene · 2023-11-28T03:57:08Z

+The `Dockerfile` includes several build [`ARG`][docker-docs::builder-arg] instructions that can be configured:

-The `Dockerfile` takes additional, so-called build arguments. These are
+- `DOVECOT_COMMUNITY_REPO`: Install Dovecot from the community repo instead of from Debian (default = 1) 
+- `DMS_RELEASE`: The image version (default = edge)
+- `VCS_REVISION`: The git commit hash used for the build (default = unknown)

-1. `VCS_VERSION`: the image version (default = edge)
-2. `VCS_REVISION`: the image revision (default = unknown)
+!!! note

-When using `make` to build the image, these are filled with proper values. You can build the image without supplying these arguments just fine though.
+    - `DMS_RELEASE` (_when not `edge`_) will be used to check for updates from our GH releases page at runtime due to the default feature [`ENABLE_UPDATE_CHECK=1`][docs::env-update-check].
+    - Both `DMS_RELEASE` and `VCS_REVISION` are also used with `opencontainers` metadata [`LABEL`][docker-docs::builder-label] instructions.
+
+[docs::env-update-check]: https://docker-mailserver.github.io/docker-mailserver/latest/config/environment/#enable_update_check
+[docker-docs::builder-arg]: https://docs.docker.com/engine/reference/builder/#using-arg-variables
+[docker-docs::builder-label]: https://docs.docker.com/engine/reference/builder/#label


Revised this section, there is technically another ARG for log output but I've omitted that. The Dovecot one seemed relevant to the reader.

I've clarified the revision context, and to an extent DMS_RELEASE, with some insights that might be relevant to the reader for custom images.

polarathene · 2023-11-28T03:59:54Z

-	@ DOCKER_BUILDKIT=1 docker build \
-		--tag $(IMAGE_NAME) \
-		--build-arg VCS_VERSION=$(shell git rev-parse --short HEAD) \
-		--build-arg VCS_REVISION=$(shell cat VERSION) \
-		.


Since v23 is now the min version advised for building and the related docs make note of DOCKER_BUILDKIT=1, the ENV is removed from here.

The build args have also been dropped. The inputs were around the wrong way, and generally for building an image that isn't a release these inputs aren't useful. If users need them they should prefer a more explicit command like we do with CI.

Not part of the image releases, so I don't see this as a breaking change.

github-actions · 2023-11-28T09:35:22Z

Documentation preview for this PR is ready! 🎉

Built with commit: 98a2283

georglauterbach

LGTM 👍🏼

I will wait for @casperklein before merging - please be advised that this is a high-priority PR - I'd like to publish v13.0.1 by the end of the day :)

Dubbeldrank · 2023-11-29T19:15:31Z

LGTM 👍🏼

I will wait for @casperklein before merging - please be advised that this is a high-priority PR - I'd like to publish v13.0.1 by the end of the day :)

Is it possible to release this DKIM hotfix without this merge?

cybermcm · 2023-11-29T19:25:18Z

Issue is fixed currently with :edge version

Dubbeldrank · 2023-11-29T19:26:50Z

Issue is fixed currently with :edge version

It doesn't feel okay to run edge on a stable environment.

casperklein · 2023-11-29T19:40:56Z

One regression I noticed:

[[ ${ENABLE_UPDATE_CHECK} -eq 1 ]] && [[ ${DMS_RELEASE} != 'edge' ]]

Previously, when you are on a tagged release that contained a bug of some kind and got advised by us to switch to edge, because it was fixed there already, you would still be notified via update-check, when a new tagged release is available, so you could switch back.
That's because in :edge, the VERSION file contained the latest tagged version at the time the image was pulled.

:edge should not care about the update process, these concerns should not have been raised:
#3659
#3662 (review)

No real issues if you ask me. In #3659 the root cause for the mail flood wasn't identified. But not related to the update-check IMO. #3662 is already fixed.

The initial question was, if we want to check against the latest release or the VERSION file in the master branch. Checking the release is the way to go.
However, replacing the VERSION file with an ENV (with other content --> "edge") and not doing an update-check at all when having a edge image is another story.

Otherwise the PR looks good 👍

polarathene · 2023-11-29T21:19:22Z

That's because in :edge, the VERSION file contained the latest tagged version at the time the image was pulled.

I don't consider that desirable as a default.

If a user is switching to :edge temporarily and would like this kind of notification they can now set the DMS_RELEASE ENV with the release tag they want to monitor for newer updates since with (eg: `DMS_RELEASE='v12.1.0'), and they will get that functionality.

We've already had reports of the prior behaviour not being desired on :edge (granted they could opt out via UPDATE_CHECK=0).

If you'd prefer to source the version tag from a git command to find the latest tag, that could be done instead, likewise UPDATE_CHECK could then be given a build arg to default it to disabled on :edge.

Alternatively, if you'd like the DMS_RELEASE ENV to be documented for :edge users, I can add that.

No real issues if you ask me. In #3659 the root cause for the mail flood wasn't identified. But not related to the update-check IMO. #3662 is already fixed.

There wasn't anything in #3659 to say they expected a notification on :edge, and I can't say that I'd want that behaviour on other software I'd run on a nightly/beta/edge like release channel. Update notifications on :edge should be opt-in.

#3662 shouldn't have been raised in the first place as :edge shouldn't need to care about stable releases. Regardless, with this PR it's resolved with an alternative approach.

However, replacing the VERSION file with an ENV (with other content --> "edge") and not doing an update-check at all when having a edge image is another story.

If you can refer to other software that notifies users of stable updates on an :edge equivalent release channel, I'd be more supportive of that.

Otherwise I'd rather our users express actual demand for the functionality and I'll implement/document it.

polarathene · 2023-11-29T21:25:11Z

I'll publish v13.0.1 shortly 🚀 (EDIT: Done!)

polarathene added 4 commits November 27, 2023 19:36

fix: Source VERSION from image ENV

123be4c

Now CI builds triggered from tagged releases will always have the correct version. No need for manually updating a separate file.

fix: Query latest GH release tag

90038ec

Compare to the remote GH release tag published, rather than contents of a `VERSION` file. `VERSION` file remains in source for now as prior releases still rely on it for an update notification.

chore: Switch from yq to jaq

168c68a

- Can more easily express a string subslice. - Lighter weight: 9.3M vs 1.7M. - Drawback, no YAML input/output support. If `yq` is preferred, the `v` prefix could be removed via BASH easily enough.

chore: Add entry to CHANGELOG.md

c9c7ea8

polarathene added area/ci area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Nov 27, 2023

polarathene added this to the v13.0.1 milestone Nov 27, 2023

polarathene requested review from casperklein and georglauterbach November 27, 2023 06:45

polarathene self-assigned this Nov 27, 2023

ci: VERSION has no relevance to :edge

dd362c2

polarathene mentioned this pull request Nov 27, 2023

release: v13.0.0 #3641

Merged

4 tasks

georglauterbach requested changes Nov 27, 2023

View reviewed changes

Comment thread target/scripts/start-mailserver.sh Outdated

Comment thread target/scripts/update-check.sh

georglauterbach mentioned this pull request Nov 27, 2023

bug report: Test on rspamd private key files not working as expected #3665

Closed

1 task

georglauterbach added the priority/high label Nov 27, 2023

georglauterbach added 2 commits November 27, 2023 22:21

Update target/scripts/start-mailserver.sh

1d35225

Merge branch 'master' into fix/update-checker-query-gh

1fe5520

georglauterbach reviewed Nov 27, 2023

View reviewed changes

Comment thread target/scripts/update-check.sh Outdated

chore: DMS_VERSION => DMS_RELEASE

c4bed0c

polarathene requested a review from georglauterbach November 27, 2023 21:58

georglauterbach previously approved these changes Nov 27, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

docs: Update build guide + simplify make build

84d5414

polarathene dismissed georglauterbach’s stale review via 84d5414 November 28, 2023 03:43

polarathene commented Nov 28, 2023

View reviewed changes

Merge branch 'master' into fix/update-checker-query-gh

98a2283

georglauterbach self-requested a review November 28, 2023 09:34

georglauterbach approved these changes Nov 28, 2023

View reviewed changes

jessec mentioned this pull request Nov 28, 2023

other: dms version dkim fix #3672

Closed

polarathene merged commit 19e96b5 into docker-mailserver:master Nov 29, 2023

polarathene mentioned this pull request Nov 29, 2023

fix: Logging - Welcome should use DMS_RELEASE ENV #3676

Merged

6 tasks

casperklein mentioned this pull request Dec 5, 2023

Add warning, when update-check is enabled, but no stable release image is used #3684

Merged

12 tasks

polarathene mentioned this pull request Dec 23, 2023

[TODO]: Consider revising DKIM generation #3630

Open

polarathene mentioned this pull request Feb 8, 2025

fix: Better support container restarts #4323

Merged

8 tasks

Uh oh!

Conversation

polarathene commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

yq vs jaq

Type of change

Checklist:

Uh oh!

georglauterbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

polarathene Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 28, 2023

Uh oh!

georglauterbach left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dubbeldrank commented Nov 29, 2023

Uh oh!

cybermcm commented Nov 29, 2023

Uh oh!

Dubbeldrank commented Nov 29, 2023

Uh oh!

casperklein commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Nov 29, 2023

Uh oh!

polarathene commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

polarathene commented Nov 27, 2023 •

edited

Loading

`yq` vs `jaq`

georglauterbach left a comment •

edited

Loading

casperklein commented Nov 29, 2023 •

edited

Loading

polarathene commented Nov 29, 2023 •

edited

Loading