Skip to content

Run user-patches.sh right before starting daemons#2817

Merged
casperklein merged 6 commits intodocker-mailserver:masterfrom
casperklein:fix-user-patches
Oct 14, 2022
Merged

Run user-patches.sh right before starting daemons#2817
casperklein merged 6 commits intodocker-mailserver:masterfrom
casperklein:fix-user-patches

Conversation

@casperklein
Copy link
Copy Markdown
Member

@casperklein casperklein commented Oct 9, 2022 •
edited
Loading

Description

From the docs:

The user patches script runs right before starting daemons.

That is not the case at the moment.

For example, changes to the file /root/.bashrc done by user-patches.sh will be discarded by the setup function _environment_variables_export, which is called after.

This PR solves that, by not registering _setup_user_patches via _register_setup_function.
Instead _setup_user_patches is called directly right before _start_daemons.

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 the documentation under docs/)
  • 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

@casperklein casperklein added this to the v11.3.0 milestone Oct 9, 2022
@casperklein casperklein requested a review from a team October 9, 2022 00:28
@casperklein casperklein self-assigned this Oct 9, 2022
@polarathene

This comment was marked as outdated.

Sign in to view
@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented Oct 9, 2022

I know that this approach doesn't respect proper SMTP exchange protocol as it doesn't wait for server responses

I don't think that's an issue. If I do the nc commands from a remote host to DMS I get:

503 5.5.0 <myremote-most]>: Client host rejected: Improper use of SMTP command pipelining.

If I do the same on the DMS host, I don't get an error. I guess it's because PERMIT_DOCKER=host is in place and some restrictions do not apply?

Maybe nc -v gives more insights.

@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented Oct 9, 2022

# nc: connect to 0.0.0.0 port 25 (tcp) failed: Connection refused

Postfix seems not to be running when nc is executed.

@polarathene polarathene mentioned this pull request Oct 9, 2022
Merged
7 tasks
@polarathene

This comment was marked as resolved.

Sign in to view
@polarathene
Copy link
Copy Markdown
Member

polarathene commented Oct 9, 2022 •
edited
Loading

The cause of change events being unreliably handled has been found :)

I'll do a quick PR for it (Done) and we can update these PRs after and there should be no more problematic tests.bats setup 👍

polarathene
polarathene previously approved these changes Oct 9, 2022
View reviewed changes
georglauterbach
georglauterbach previously approved these changes Oct 9, 2022
View reviewed changes
@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Oct 13, 2022
@casperklein casperklein marked this pull request as ready for review October 13, 2022 18:31
@casperklein casperklein enabled auto-merge (squash) October 13, 2022 18:37
@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented Oct 13, 2022

If possible, we should exclude CHANGELOG.md or markdown in general from the tests. As long as we have no markdown specific tests, there is no need to run the tests.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 13, 2022

If possible, we should exclude CHANGELOG.md or markdown in general from the tests. As long as we have no markdown specific tests, there is no need to run the tests.

A good idea 👍🏼

Also, #2815 should fix the issue with the tests were seeing currently.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 14, 2022

@casperklein which tastes exactly are you referring to?:)

@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented Oct 14, 2022 •
edited
Loading

I noticed with my last commit, that the CI tests were run again. The only change in that commit however was in CHANGELOG.md.

But I think, that is how it works.

Markddown files per se are not considered for running the tests:

docker-mailserver/.github/workflows/default_on_push.yml

Lines 8 to 13 in 488b256

paths:
- target/**
- .dockerignore
- .gitmodules
- Dockerfile
- setup.sh

I guess, on a commit, the whole PR is taking into account and not just the files altered with the current commit.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 14, 2022

I guess, on a commit, the whole PR is taking into account and not just the files altered with the current commit.

Yes :)

@casperklein casperklein merged commit 6d016ba into docker-mailserver:master Oct 14, 2022
@casperklein casperklein deleted the fix-user-patches branch October 15, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@polarathene polarathene polarathene approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

Assignees

@casperklein casperklein

Labels

area/scripts priority/medium

Projects

None yet

Milestone

v11.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@casperklein @polarathene @georglauterbach