-
Notifications
You must be signed in to change notification settings - Fork 685
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
postfix_prevent_unrestricted_relay: allow whitespaces and no comma for 'smtpd_client_restrictions' value #10219
postfix_prevent_unrestricted_relay: allow whitespaces and no comma for 'smtpd_client_restrictions' value #10219
Conversation
Hi @rmetrich. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include a test scenarios these variations, please?
…r 'smtpd_client_restrictions' value Examples of valid entries per postconf(5) manpage: ("Specify a list of restrictions, separated by commas and/or whitespace.") smtpd_client_restrictions = permit_mynetworks,reject smtpd_client_restrictions = permit_mynetworks, reject smtpd_client_restrictions = permit_mynetworks reject smtpd_client_restrictions = permit_mynetworks , reject smtpd_client_restrictions = permit_mynetworks reject Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
813b723
to
2d03f86
Compare
Code Climate has analyzed commit 2d03f86 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 51.7% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests have passed in local VMs too.
I am fine to merge this PR as it is, but I have three comments that would be good to keep in mind for next PRs:
- in general, after a request for changes, it is preferable to send more commits instead of amending the previous commits and forcibly pushing the changes. This way we can see the last changes and it makes the review time much faster.
- the
ok
prefix is not necessary inpass
test scenarios - each test scenario increases a little bit the testing time, it is not the case here but consider to unify similar tests whenever reasonable
Thanks for the fix and hope to see more PRs from you. : )
@@ -0,0 +1,5 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test scenarios already have a suffix pass
or fail
.
https://complianceascode.readthedocs.io/en/latest/tests/README.html#test-scenarios
It seems redundant to use the ok
prefix in the name of pass
scripts. I would prefer to remove them, but would also be fine with this in this specific PR since there is not technical impact in this case. Only keep this in mind for the next PRs, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments noted for next time. I'm used to squash things because usually other projects do not tolerate unsquashed commits...
Rule xccdf_org.ssgproject.content_rule_postfix_prevent_unrestricted_relay
Description:
The
postconf(5)
manpage specifies that values can be separated by commas and/or whitespace for smtpd_client_restrictions property.Rationale:
The following specifications should be valid: