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

if relayhost is defined, then use the satellite profile #310

Closed
wants to merge 50 commits into from

Conversation

kapouik
Copy link

@kapouik kapouik commented Oct 5, 2021

Pull Request (PR) description

when relayhost is define in the init.pp, there is nothing call behind it. As it, if you use it like show in the documentation, it don't work.
After reading the code, it look like that the standard usage for relayhost is to use satellite profile that permit to configure postfix to send all email to $relayhost.

This Pull Request (PR) fixes the following issues

Fixes #295

manifests/init.pp Outdated Show resolved Hide resolved
@kenyon kenyon changed the title if relayhost is define, then use the satellite profile if relayhost is defined, then use the satellite profile Oct 10, 2021
@@ -334,12 +334,11 @@
end
end
context 'when mta is enabled' do
let(:params) { { mta: true, mydestination: '1.2.3.4', relayhost: '2.3.4.5' } }
Copy link
Member

Choose a reason for hiding this comment

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

Why remove?

Also, I think testing should be added to verify the intended behavior.

Copy link
Author

Choose a reason for hiding this comment

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

Whith this change, $relayhost become like $satellite. So as you can't have $satellite and $mta define in the same time, you can't have $relayhost and $mta in same time.
When you use the mta profile whit the relayhost, there is two correct way to do it :

  • being call by satellite profile
  • being call directly
    So this change is necessary to made test working.

Copy link
Member

Choose a reason for hiding this comment

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

Since these items are mutually exclusive, there should be a test for them that shows the module fails as expected if they are both set.

@kapouik kapouik requested a review from kenyon October 11, 2021 22:34
manifests/init.pp Outdated Show resolved Hide resolved
@kapouik
Copy link
Author

kapouik commented Oct 15, 2021

I have made the necessary modification in the documentation and in the error message.

kenyon and others added 11 commits October 17, 2021 21:08
Parameters used for generating resources should be plural.

Remove incorrect usage of Optional.

Also, use loops instead of `create_resources()`.
The `regexp` map type is similar to the `pcre` type in that it doesn't
need a database generated by `postmap`.

Also some cleanup of the parameter formatting.
Tests for wrong parameter data types aren't really needed since Puppet
will do the type checking itself. These tests also caused warnings
because they didn't provide specific error messages to the
`raise_error` matcher.

Also, provide error messages to the remaining `raise_error` matches
that lacked error messages, to avoid warnings like this:

WARNING: Using the `raise_error` matcher without providing a specific
error or message risks false positives, since `raise_error` will match
when Ruby raises a `NoMethodError`, `NameError` or `ArgumentError`,
potentially allowing the expectation to pass without even executing
the method you are intending to call. Actual error raised was
a Resource Statement, Postfix::... expects a match for Enum['absent',
'present'], got Tuple (line: 5) on node lithium.kenyonralph.com>.
Instead consider provi ding a specific error class or message. This
message can be suppressed by setting:
`RSpec::Expectations.configuration.on_potential_false_positives =
:nothing`. Called from /home/kenyon/git/
puppet-postfix/spec/defines/postfix_virtual_spec.rb:88:in `block (5
levels) in <top (required)>'.
…tional-usage

init.pp: correct param numbers and use of optional
allow creation of postfix::map resources with hiera
remove unneeded tests; provide error message for raise_error
traylenator and others added 7 commits February 10, 2022 11:11
For CentOS 9 and RHEL 9 the `mailx` and `mail` commands
are provided by the package `s-nail` rather than
`mailx`.

For information: https://www.sdaoden.eu/code.html#s-nail
Declare CentOS 9 Support, Install s-nail on 9
…November of 2032. It does not appear to have breaking changes. It would be nice for the dependencies on the Postfix module to reflect the latest versions.
increase dependency of puppet/alternatives to next major version
@@ -334,12 +334,11 @@
end
end
context 'when mta is enabled' do
let(:params) { { mta: true, mydestination: '1.2.3.4', relayhost: '2.3.4.5' } }
Copy link
Member

Choose a reason for hiding this comment

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

Since these items are mutually exclusive, there should be a test for them that shows the module fails as expected if they are both set.

@ghoneycutt ghoneycutt added the enhancement New feature or request label Feb 23, 2022
jcpunk and others added 10 commits March 9, 2022 11:08
Add switches for simple domain masquerade
Fixes variable typos that has been added with Pull Request 326.
This is just cosmetic since this values are also pick_default inside postfix::mta and does result to the same catalog on the systems.
Because pick_default converts undef to an empty string we need to use
lest here as a work around.
https://tickets.puppetlabs.com/browse/MODULES-6534
For datatype consistency we also change it in the manifests/mta.pp.
Fix typos errors in postfix::satellite from PR 326
@vox-pupuli-tasks
Copy link

Dear @kapouik, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@smortex
Copy link
Member

smortex commented May 13, 2023

It looks like that branch got somewhat messed-up for some times. I will close this PR for now, but feel free to submit a cleaned-up version if you want your change to be upstreamed.

@smortex smortex closed this May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request merge-conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please fix postfix::relayhost parameter