Skip to content

fix: accounts.sh - Support first valid local account#4581

Merged
polarathene merged 10 commits intomasterfrom
fix/alias-dummy-multiple-aliased
Oct 8, 2025
Merged

fix: accounts.sh - Support first valid local account#4581
polarathene merged 10 commits intomasterfrom
fix/alias-dummy-multiple-aliased

Conversation

@polarathene
Copy link
Copy Markdown
Member

@polarathene polarathene commented Oct 2, 2025 •
edited
Loading

Description

Ran into a small bug during a reproduction example, the alias was not creating a dummy account. This provides a slight improvement / fix to resolve that, along with minor revisions to the helper accounts.sh.

  • Ensure catch-all alias entries (eg: @example.test) are also skipped. Adding a dummy account for catch-all will not match a quota-status query to Dovecot by Postfix for a recipient.
  • When there are multiple addresses provided, they will now be iterated through by the , delimiter, instead of as a single value that fails. This way the first valid aliased address to a local Dovecot mailbox account will now be used for the dummy alias account.
  • The common logic for extracting the quota user attribute is now split out to a common function call.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change that does improve existing functionality)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes
  • I have added information about changes made in this PR to CHANGELOG.md

@polarathene
fix: accounts.sh - Support first valid local account
48f737c 
- Ensure catch-all alias entries (eg: `@example.test`) are also skipped. Adding a dummy account for catch-all will not match a `quota-status` query to Dovecot by Postfix for a recipient.
- When there are multiple addresses provided, they will now be iterated through by the `,` delimiter, instead of as a single value that fails. This way the first valid aliased address to a local Dovecot mailbox account will now be used for the dummy alias account.
- The common logic for extracting the quota user attribute is now split out to a common function call.
@polarathene polarathene added this to the v16.0.0 milestone Oct 2, 2025
@polarathene polarathene self-assigned this Oct 2, 2025
polarathene
polarathene commented Oct 2, 2025
View reviewed changes
Comment thread target/scripts/helpers/accounts.sh Outdated
polarathene
polarathene commented Oct 2, 2025
View reviewed changes
Comment thread target/scripts/helpers/accounts.sh Outdated
@polarathene
Update target/scripts/helpers/accounts.sh
0c6cae2
@polarathene
Update account_management.bats
d7d5b5b 
The catch-all alias should not have been added as a Dovecot dummy account.
@polarathene

This comment was marked as resolved.

Sign in to view
@polarathene polarathene enabled auto-merge (squash) October 2, 2025 21:28
polarathene
polarathene commented Oct 2, 2025
View reviewed changes
Comment thread test/tests/parallel/set3/mta/account_management.bats
georglauterbach
georglauterbach reviewed Oct 4, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach 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, just some notes.

Comment thread target/scripts/helpers/accounts.sh
Comment thread target/scripts/helpers/accounts.sh
Comment thread target/scripts/helpers/accounts.sh
@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Oct 7, 2025 •
edited
Loading

Beyond potentially double checking the unset requirement, and potentially adding a test case to ensure the problem is caught should someone remove it and introduce a regression bug, and perhaps a more clearer comment for the reader as to why these lines require caution... I think the PR itself has nothing else blocking?

Concerns like these are also incentives towards migrating some of the scripts away from bash (which I'd look into if we didn't have so much of a backlog as-is elsewhere). I definitely would like to pursue migrating our setup CLI to Rust at some point (perhaps next year), or at the very least the accounts/alias management.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 8, 2025

Will take a look soon and merge if everything is fine

@polarathene polarathene merged commit e5185e0 into master Oct 8, 2025
2 checks passed
@polarathene polarathene deleted the fix/alias-dummy-multiple-aliased branch October 8, 2025 19:58
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 8, 2025 •
edited
Loading

Will take a look soon and merge if everything is fine

Didn't yet get around to it in time it seems; any pressure merging this?

@polarathene
Copy link
Copy Markdown
Member Author

polarathene commented Oct 8, 2025

Oh sorry I just pressed the update branch button, didn't notice the auto merge I had enabled 😓

No there was no pressure to merge it. If you have anything you wanted to go over let me know and I can setup another PR.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 8, 2025

No worries, I'lll go over it soon and if anything comes up, I'll let you know. I do not anticipate any issues, though.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Oct 16, 2025

Resolved all conversations, LGTM 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@georglauterbach georglauterbach georglauterbach left review comments

@casperklein casperklein casperklein approved these changes

Assignees

@polarathene polarathene

Labels

area/scripts service/postfix

Projects

None yet

Milestone

v16.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@polarathene @georglauterbach @casperklein