Skip to content

bsmtp bls bextract: fixes for command line parsing#1455

Merged
pstorz merged 3 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/s5465-bsmtp-mailhost-default
May 11, 2023
Merged

bsmtp bls bextract: fixes for command line parsing#1455
pstorz merged 3 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/s5465-bsmtp-mailhost-default

Conversation

@alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Apr 25, 2023 •
edited by bruno-at-bareos
Loading

Description

This PR makes sure the -h or --mailhost option is not required and can be left out. It can then be set to the SMTPSERVER environment variable, or default localhost.

  • SMTPSERVER can handle fqdn:port/0.0.0.0:port/[::]:port syntaxes
  • debug level -d20 show now from which source the host and port is taken.

This PR also include a small fix to handle --include-list (-i) flag in bextract and bls

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR

@bruno-at-bareos bruno-at-bareos self-assigned this Apr 27, 2023
@bruno-at-bareos
Copy link
Contributor

bruno-at-bareos commented Apr 27, 2023

It is almost working well except if someone use the SMTPSERVER env var with a :PORT

export SMTPSERVER=smtp.domain.tld:25
bsmtp -f 'Bareos daemon<hostmaster@ioda.net>' -s "bsmtp test without env var" -c ops@domain.tld admin@domain.tld <<< "New test of bsmtp with env var and 
 option -h non required"
bsmtp: tools/bsmtp.cc:407-0 Error unknown mail host "smtp.domain.tld:25": ERR=Name or service not known
bsmtp: tools/bsmtp.cc:410-0 Retrying connection using "localhost".
bsmtp: tools/bsmtp.cc:436-0 Failed to connect to mailhost localhost

using the var without the port works.

I've done this test as if you bsmtp --help indicate this should be possible

-h,--mailhost <mailhost/IPv4_address:port>,<[mailhost/IPv6_address]:port>
    Use mailhost:port as the SMTP server.

bruno-at-bareos
bruno-at-bareos reviewed Apr 27, 2023
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos 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 for the moment, check my comment about the test with SMTPSERVER:PORT.

@alaaeddineelamri
Copy link
Contributor Author

alaaeddineelamri commented Apr 28, 2023

I've done this test as if you bsmtp --help indicate this should be possible

-h,--mailhost <mailhost/IPv4_address:port>,<[mailhost/IPv6_address]:port>
    Use mailhost:port as the SMTP server.

The help indicates using host:port within the options, but not as the environment variable. That could doable though, but should be documented where providing the environment variable is mentionned.

@bruno-at-bareos bruno-at-bareos self-requested a review May 2, 2023 08:43
bruno-at-bareos
bruno-at-bareos requested changes May 2, 2023
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Can we check if we can better handle the default?
Also bsmtp.cc need to have copyright updated.

@bruno-at-bareos bruno-at-bareos self-requested a review May 3, 2023 07:59
bruno-at-bareos
bruno-at-bareos requested changes May 3, 2023
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

--removed--

bruno-at-bareos
bruno-at-bareos requested changes May 3, 2023
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Good Work, I think we can squash the commit concerning bsmtp in one and the second concerning bextract and bls

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/s5465-bsmtp-mailhost-default branch from 9009f89 to e94a854 Compare May 3, 2023 08:36
bruno-at-bareos
bruno-at-bareos approved these changes May 3, 2023
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Thanks a lot!!!

Ready for merge

@alaaeddineelamri alaaeddineelamri mentioned this pull request May 3, 2023
Merged
11 tasks
@pstorz pstorz changed the title bsmtp: make mailhost not required and update docs bsmtp bls bextract: fixes for command line parsing May 11, 2023
Alaa Eddine Elamri and others added 3 commits May 11, 2023 12:37
@pstorz
bsmtp: make mailhost not required and update docs
7a718f2 
bsmtp: parse `SMTPSERVER` value


bsmtp: update default mailport handling


bsmtp: get environment variable earlier

renamed `cp` and `mail_port`
@pstorz
bls: bextract: fix -i not correctly processing
6cb9bee
@pstorz
Update CHANGELOG.md
48ab1d8
@pstorz pstorz force-pushed the dev/alaaeddineelamri/master/s5465-bsmtp-mailhost-default branch from 9ef9817 to 48ab1d8 Compare May 11, 2023 10:37
@pstorz pstorz merged commit b721365 into bareos:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bruno-at-bareos bruno-at-bareos bruno-at-bareos approved these changes

Assignees

@bruno-at-bareos bruno-at-bareos

Labels

requires backport to 22

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alaaeddineelamri @bruno-at-bareos @pstorz