Provide complete refactoring of openDKIM script by georglauterbach · Pull Request #1812 · docker-mailserver/docker-mailserver

georglauterbach · 2021-02-16T12:08:47Z

Description

Complete refactoring of target/bin/generate-dkim-config and outsourcing of tests.

What's left to do?

Many of the currently present tests need a rewrite. This is indicated in the open_dkim.bats file with # TODO. @wernerfred or @casperklein I have already spent more time than I have on this. I would need your help here.

Note: The Makefile's test target is currently only testing the openDKIM config for speed. This will of course be reverted before merging.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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
I have made corresponding changes to the documentation (README.md or ENVIRONMENT.md or the Wiki)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

wernerfred · 2021-02-16T15:10:15Z

Pretty busy today. I'll take a look at it as soon as I have a larger time frame available.
Last time i hotfixed the bad tests (told ya that calling them bad style is way no nice) i looked for alternatives and discovered that opendkim already provides a checkkey option. Sadly it relies on DNS i think which will not work (at least without changes). But I will see if I find a solution.
If you came up with an idea on how to check the keys in a "correct" way feel free to drop it here.

georglauterbach · 2021-02-16T16:23:36Z

Pretty busy today. I'll take a look at it as soon as I have a larger time frame available.

No hurry. Some tests just require proper formatting (I already did a bit on this topic), some a bit refactoring (get rid of wc -l and stuff), and some require more attention. Take your time.

I'm learning for exams atm, therefore I don not have any more spare time.

polarathene · 2021-02-17T11:07:19Z

After my PRs are sorted, I have a related docker-compose setup I plan to tidy up and publish on github as a reference setup. It has custom DNS service via container for all containers involved to use and another container that can update DNS records via a text template.

Might be appropriate for what you want to do, as I get the time, I'll be extending my reference to cover mail server DNS records myself. If you'd like to include for tests let me know :)

georglauterbach · 2021-02-18T08:58:44Z

@wernerfred I would like to propose an idea: I would like to focus on the unit tests later in a separate PR because of two reasons:

I would like to see this PR merged :)
I don't want to pressure you into writing tests up. Currently tests are passing fine and I already separated them into their own file, which makes working with them in the future a lot easier.

If you agree, you can just go ahead and merge this or tell me and I will do it :)

wernerfred · 2021-02-18T09:27:52Z

Seems logic to me. I will agree as we will definitely need to refactor at least half of the test. Doing this in a separate step together with all others might safe more time in the end as doing it step by step and need to get into it every time again. But we should focus on this in the next time.
This will be a massive PR on which multiple people can work for quite a while. Shouldn't affect the development of the project at all only rebases from time to time will be necessary.

georglauterbach · 2021-02-18T09:32:42Z

This will be a massive PR on which multiple people can work for quite a while. Shouldn't affect the development of the project at all only rebases from time to time will be necessary.

Completely agree.

wernerfred · 2021-02-18T16:55:55Z

+        break
+        break


Why are you using break here instead of shift ?

Because I messed up. Can you correct this for me? Currently short on time...

6e6b5be#diff-8ec5e15bee29bcdefebd50369ee7e19ad72d8b6e8c2d9475e3c4e8a463985a85

https://github.com/docker-mailserver/docker-mailserver/runs/1929465064?check_suite_focus=true#step:6:325

@aendeavor any idea?:

# -- line differs -- # index : 3 # expected : open-dkim - configure DomainKeys Identified Mail (DKIM) # actual : open-dkim - configure DomainKeys Identified Mail (DKIM) # -- #

Actually your change (d221c58#diff-9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a089) never went through CI as it was no PR and therefore tests didn't run only the linting.

Might the output have problems with colors?

Tested it locally with \e[0m but no difference. So the problem might be something else. But I don't get it. The output is exactly the same.

So I added --partial to the assert line and the test went through locally, will push this as a hotfix but I want to know which "invisible" char breaks the check

This is absolutely weird... For setup.sh it seems to work just fine...

provide complete refactoring of openDKIM usage and tests

a905567

georglauterbach added pr/needs review priority/medium area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 16, 2021

georglauterbach requested a review from wernerfred February 16, 2021 12:08

georglauterbach self-assigned this Feb 16, 2021

georglauterbach mentioned this pull request Feb 16, 2021

Feat/enable custom dkim selector #1811

Merged

11 tasks

georglauterbach added 2 commits February 16, 2021 13:11

fix leftover linting errors

c6b8f91

correct defualt key size and README usage

11ce3eb

georglauterbach added the meta/wip label Feb 16, 2021

provide independent order for arguments

440acb8

georglauterbach commented Feb 16, 2021

View reviewed changes

Comment thread README.md Outdated

georglauterbach commented Feb 16, 2021

View reviewed changes

Comment thread target/bin/generate-dkim-config Outdated

wernerfred reviewed Feb 16, 2021

View reviewed changes

Comment thread Makefile

georglauterbach added 2 commits February 16, 2021 17:50

added config and adjusted usage information

a04052d

fixing shift in setup.sh

a53cb11

georglauterbach added 2 commits February 17, 2021 17:14

adjust usage information to use new style and rename script

5a3552e

use updated argument keysize instead of size

54aceed

georglauterbach changed the title ~~Provide complete refactoring of openDKIM usage and tests~~ Provide complete refactoring of openDKIM script Feb 18, 2021

georglauterbach requested a review from wernerfred February 18, 2021 08:58

georglauterbach removed the meta/wip label Feb 18, 2021

wernerfred approved these changes Feb 18, 2021

View reviewed changes

wernerfred merged commit 1005bb3 into master Feb 18, 2021

wernerfred deleted the open-dkim branch February 18, 2021 09:29

wernerfred reviewed Feb 18, 2021

View reviewed changes

wernerfred mentioned this pull request Feb 18, 2021

fix: dkim help message test #1817

Merged

7 tasks

polarathene mentioned this pull request Feb 5, 2023

tests(refactor): open_dkim.bats #3060

Merged

5 tasks

Uh oh!

Conversation

georglauterbach commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What's left to do?

Type of change

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wernerfred commented Feb 16, 2021

Uh oh!

georglauterbach commented Feb 16, 2021

Uh oh!

polarathene commented Feb 17, 2021

Uh oh!

georglauterbach commented Feb 18, 2021

Uh oh!

wernerfred commented Feb 18, 2021

Uh oh!

georglauterbach commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

georglauterbach commented Feb 16, 2021 •

edited

Loading