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

Add Dockerfile #124

Merged
merged 9 commits into from
Nov 12, 2021
Merged

Add Dockerfile #124

merged 9 commits into from
Nov 12, 2021

Conversation

mattias-p
Copy link
Member

@mattias-p mattias-p commented Oct 27, 2021

Purpose

This PR provides dockerization of Zonemaster LDNS.

Context

This is a step towards zonemaster/zonemaster-cli#216.

Changes

This PR adds a docker file and steps to build it.

How to test this PR

Follow the new steps in the README file.

@matsduf matsduf added this to the v2021.2 milestone Oct 27, 2021
@@ -91,6 +92,26 @@ is unset (those tests are not run). To run all tests, execute
TEST_WITH_NETWORK=1 make test
```

## Docker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Docker should be seen as an alternative installation and be a sub-section to "Installation and verification". Furthermore, we will also here add that there are (will be) pre-built Docker images.

In contrast to Engine and CLI, I think the details can be kept in the README file.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ make distcheck
"/usr/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck
Not in MANIFEST: Dockerfile

README.md Outdated Show resolved Hide resolved

```sh
make docker-tag-version
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a make docker-build I tried this command:

$ make docker-tag-version
make: *** No rule to make target 'docker-image', needed by 'docker-tag-version'.  Stop.

Some argument seems to be missing. On the other hand, it looked like tagging was done as part of make docker-build

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed a typo so it should work now.

Yes, when you build a new image locally it automatically gets the local tag. That tag is used by the make docker-tag-version and make docker-tag-latest targets know which image to use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when do I use make docker-tag-version and make docker-tag-latest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary use case for both of these are when you want to publish a new version of the image. You use docker-tag-version to create a version-specific tag, and docker-tag-latest to move the latest tag to the latest version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I can see the rules creates images with other tagging.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -91,6 +92,32 @@ is unset (those tests are not run). To run all tests, execute
TEST_WITH_NETWORK=1 make test
```

## Docker

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that the following is added to make it obvious:

Clone the Git reposistory and prepare the clone:
```sh
git clone https://github.com/zonemaster/zonemaster-ldns
cd zonemaster-ldns
perl Makefile.PL
```

Comment on lines +103 to +106
Tag the local base image with the current version number:

```sh
make docker-tag-version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule also builds an images. I suggest:

"Build an image tagged with the current version number:"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the implementation to match the documentation.

Comment on lines +109 to +113
Tag the local base image as the latest version:

```sh
make docker-tag-latest
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule also builds an images. I suggest:

"Build an image tagged with the 'latest':"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the implementation to match the documentation.

Makefile.PL Outdated Show resolved Hide resolved
Makefile.PL Outdated Show resolved Hide resolved
@mattias-p
Copy link
Member Author

@matsduf @blacksponge Please review again.

Copy link
Member

@hannaeko hannaeko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will suggests some updates when it has been merged (new PR).

@mattias-p mattias-p merged commit 576ea29 into zonemaster:develop Nov 12, 2021
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.

3 participants