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

Don't set default mail user/password #46

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Conversation

johanvdw
Copy link
Contributor

It should be possible to connect to a mail server without username and password (on localhost/local network).

This is part of the template, but because these variables are set as default they will always be included

It should be possible to connect to a mail server without username and password (on localhost/local network).

This is part of the template, but because these variables are set as default they will always be included
Copy link
Member

@rixx rixx left a comment

Choose a reason for hiding this comment

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

Hmm, does this still render cleanly when the variable isn't set? I have vague memories of having to do is_defined or |defined or somesuch in Ansible when variables were not guaranteed to be present.

Maybe better to set them to a null value, to make it clear that they exist, and then check if they are empty? AIUI, defaults/main.yml serves as a common entry point for people looking for variable documentation, and removing them entirely makes them hard to discover.

@johanvdw
Copy link
Contributor Author

johanvdw commented Oct 2, 2023

I had another look at the code and setting them to None or False should indeed work.

@rixx
Copy link
Member

rixx commented Oct 3, 2023

Sounds good! ping me when you've updated the PR and I'll merge it :)

@rixx rixx merged commit 9adc509 into pretalx:master Oct 23, 2023
1 check passed
@rixx
Copy link
Member

rixx commented Oct 23, 2023

Thanks!

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.

2 participants