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
fix: update-check.sh
should query GH Releases
#3666
fix: update-check.sh
should query GH Releases
#3666
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏼
One tiny nitpick (style) 🚀
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some review comments for context to the latest changes.
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page no longer makes mention of the make build
command, it doesn't add much value to the reader so seemed like noise?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@ 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) \ | ||
. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Documentation preview for this PR is ready! 🎉 Built with commit: 98a2283 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
Issue is fixed currently with :edge version |
It doesn't feel okay to run edge on a stable environment. |
One regression I noticed:
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.
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. Otherwise the PR looks good 👍 |
I don't consider that desirable as a default. If a user is switching to We've already had reports of the prior behaviour not being desired on If you'd prefer to source the version tag from a Alternatively, if you'd like the
There wasn't anything in #3659 to say they expected a notification on #3662 shouldn't have been raised in the first place as
If you can refer to other software that notifies users of stable updates on an Otherwise I'd rather our users express actual demand for the functionality and I'll implement/document it. |
I'll publish v13.0.1 shortly 🚀 (EDIT: Done!) |
Description
We have had 3 concerns encountered related to this feature in the past.
Resolved by:
VERSION
as an image ENV (DMS_RELEASE
) in theDockerfile
to build. No need to manually updateVERSION
(well, until a future release warrants removing it).master
branchVERSION
file.Past concerns addressed:
:edge
should not care about the update process, these concerns should not have been raised:exit 0
fromupdate-check.sh
#3659:edge
whenVERSION
is updated as well #3662 (review)VERSION
accordingly::edge
unaffected since it's not actually tied to the/VERSION
file that it would compare to GithubVERSION
.Additional context
Applies changes were originally proposed here:
ℹ️ Note:
v
to strip away. Whereas the currentVERSION
file has nov
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? 🤷♂️context
docs + conditional expression example.github.ref
/${GITHUB_REF}
,github.ref_name
should be just the tag like in the JSONtag_name
field.yq
vsjaq
I have expressed interest in
yq
previously for managing DKIM / rspamd config (and processing JSON output fromstep
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 thev
prefix character.See the
jaq
link for preference of it overjq
. Docs wise, if usingjaq
elsewhere in DMS scripts you can follow thejq
docs, or see thejaq
README for some additional features/capabilities.Type of change
Checklist:
CHANGELOG.md