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

Adapt for SMTP without auth (GSI-949) #17

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

TheByronHimes
Copy link
Member

This PR enables running the service with empty auth credentials. Testing was slightly improved by moving the SMTP instantiation to a separate method that can then be mocked.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10354400973

Details

  • 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.7%) to 85.214%

Totals Coverage Status
Change from base Build 10348389921: 1.7%
Covered Lines: 219
Relevant Lines: 257

💛 - Coveralls

@TheByronHimes TheByronHimes marked this pull request as ready for review August 12, 2024 15:15
Copy link
Member

@lkuchenb lkuchenb left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think you can get around your dual-None validation and checking by modelling the config accordingly:

class SmtpAuthConfig(BaseModel):
    username: str
    password: str

class SmtpClientConfig(BaseSettings):
    smtp_host: str
    smtp_port: int
    smtp_auth: None | SmtpAuthConfig

You would then need to set env_nested_delimiter = '__' or alike to enable configuring the nested fields through environment variables. That way you can skip the validator and a conditional check whether auth is required would also be more straight forward!

@TheByronHimes TheByronHimes merged commit 559ecec into main Aug 13, 2024
8 checks passed
@TheByronHimes TheByronHimes deleted the feature/stmp_without_auth_GSI-949 branch August 13, 2024 09:59
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.

3 participants