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

Cleanup docker #249

Merged
merged 5 commits into from
Sep 12, 2023
Merged

Cleanup docker #249

merged 5 commits into from
Sep 12, 2023

Conversation

testower
Copy link
Collaborator

@testower testower commented Sep 11, 2023

Since #246 the Dockerfile isn't really needed in the root anymore. docker-compose users may decide if they want to use the public image (available at docker.io/entur/lamassu) or use this example dockerfile?

cc @derhuerst @hbruch

@derhuerst
Copy link
Contributor

derhuerst commented Sep 11, 2023

The "standalone" Dockerfile provides one advantage: It allows building an image without having installed Java tooling (JDK, Maven). However, I'm not sure if there is a target group behind this use case, given that those people who want to modify Lamassu code like have the tooling installed anyways.

TLDR: I think we could remove it. IMHO it depends on how likely Lamassu maintainers are to keep it up-to-date in the long term.

@derhuerst
Copy link
Contributor

from #246 (comment):

Note that the Docker image, as currently built by Jib, lacks the following:

  • a README.md, which Docker Hub would pick up and render
  • OCI image annotations like org.opencontainers.image.documentation or org.opencontainers.image.licenses, which various Docker-related tools read and display to provide a better DX

@testower
Copy link
Collaborator Author

Another oustanding issue: docker build should be verified on pull requests (but not push the image). Thanks for raising this @derhuerst

@testower
Copy link
Collaborator Author

a README.md, which Docker Hub would pick up and render

As far as I can tell this has to be done separately. DockerHub doesn't look inside the image for a readme, it has to be uploaded manually or programatically.

OCI image annotations like org.opencontainers.image.documentation or org.opencontainers.image.licenses, which various Docker-related tools read and display to provide a better DX

Not sure I understand. JIB has an option to output the OCI format instead of Docker, but that's probably not what you had in mind?

@testower
Copy link
Collaborator Author

Another oustanding issue: docker build should be verified on pull requests (but not push the image). Thanks for raising this @derhuerst

Fixed with jib:dockerBuild in test job

@sonarcloud
Copy link

sonarcloud bot commented Sep 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derhuerst
Copy link
Contributor

a README.md, which Docker Hub would pick up and render

As far as I can tell this has to be done separately. DockerHub doesn't look inside the image for a readme, it has to be uploaded manually or programatically.

But many other tools do (e.g. GitHub Packages, docker-pushrm, and Docker Desktop will do it soon). Also, one can always manually check the readme from the command line. This is a lot less error-prone, albeit not done often, than google-ing the tool's repo's readme, since one can be sure to have the right version.

IMHO there's no good reason not to add the readme to the image.

@derhuerst
Copy link
Contributor

derhuerst commented Sep 12, 2023

OCI image annotations like org.opencontainers.image.documentation or org.opencontainers.image.licenses, which various Docker-related tools read and display to provide a better DX

Not sure I understand. JIB has an option to output the OCI format instead of Docker, but that's probably not what you had in mind?

AFAIK, but I'm not sure, there is the Docker-proprietary format (Image Manifest v2) and there is the OCI format. By now, all tooling, including Docker CLI & Docker Hub, should support both.

The OCI format also defines annotations, which with certain pre-defined well-known annotations (e.g. org.opencontainers.image.licenses) that can be picked up by tooling (e.g. the Docker CLI, Docker Hub, quay.io, GitHub Packages, Kubernetes, etc.).

Even when building images in the Docker-proprietary format, one can add OCI annotations which will be picked up (e.g. here on GitHub Packages). So, given that most of the ecosystem supports them, they should under many circumstances improve the UX for developers using the image.


Not sure how to get JIB to put the annotations though.

@testower
Copy link
Collaborator Author

IMHO there's no good reason not to add the readme to the image.

Not really arguing against it, just pointing out it's not a short-coming of jib per se as this would perhaps suggest "Note that the Docker image, as currently built by Jib, lacks the following:"

I would suggest creating a separate issue / PR for this.

Not sure how to get JIB to put the annotations though.

This is beyond the scope of what we need at Entur and I'm not familiar with OCI. Feel free to contribute on this separately if you need it.

@testower testower merged commit f9cc7b2 into master Sep 12, 2023
4 checks passed
@testower testower deleted the cleanup-docker branch September 12, 2023 18:22
@derhuerst
Copy link
Contributor

Just noticed that the current images are missing some helpful metadata.

docker image inspect docker.io/entur/lamassu:2024-07-15T11-30 | jq -r --tab '.[0]'

shortened for readability:

{
	"Comment": "jvm arg files",
	"Created": "1970-01-01T00:00:00Z",
	"DockerVersion": "",
	"Author": "",
	"Config": {
		"Labels": {}
	},
	"Metadata": {
		"LastTagTime": "0001-01-01T00:00:00Z"
	}
}

Compare this to a docker buildx build-based image which also has OCI annotations in the Dockerfile. (Some are missing/wrong there, too.)

docker image inspect ghcr.io/mobidata-bw/postgis-with-pg-plan-filter:2024-05-15T15.25.26-d44ab82 | jq -r --tab '.[0]'
[
	{
		"Comment": "buildkit.dockerfile.v0",
		"Created": "2024-05-15T15:25:39.210927025Z",
		"DockerVersion": "",
		"Author": "",
		"Config": {
			"Labels": {
				"maintainer": "PostGIS Project - https://postgis.net",
				"org.opencontainers.image.authors": "MobiData-BW IPL contributors <[email protected]>",
				"org.opencontainers.image.description": "PostGIS Docker image with the pg_plan_filter extension.",
				"org.opencontainers.image.documentation": "https://github.com/mobidata-bw/postgis-with-pg-plan-filter",
				"org.opencontainers.image.licenses": "(EUPL-1.2)",
				"org.opencontainers.image.source": "https://github.com/mobidata-bw/postgis-with-pg-plan-filter",
				"org.opencontainers.image.title": "postgis-with-pg-plan-filter"
			}
		},
		"Metadata": {
			"LastTagTime": "0001-01-01T00:00:00Z"
		}
	}
]

While the metadata is not strictly necessary, various tools (GitHub Container Registry, Docker Registry, Docker CLI, Renovate Bot , etc.) pick them up to provide smarter defaults and a better UX.

@testower
Copy link
Collaborator Author

Please refer to the previous comment from me in this PR. If you need this, create a new issue, and contribute a solution.

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.

2 participants