refactor: Helper function _replace_by_env_in_file()
#3526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Minor change to add support for usage with `saslauthd.conf` which differs with `:` for it's key/value delimiter. Also adopts the`_escape_for_sed()` method, instead of the inline sed pattern (_which for some reason escaped `=` but not `(` + `)`, thus buggy if ever matching on input with those tokens_). `_escape_for_sed()` likewise wasn't escaping for `(` + `)` or even `|`, which are required for proper escape support if using `sed -E` / `sed -r` with this method. Additionally added escaping support for `&` replacement segment token, which seems valid. Increased verbosity to better grok pattern matching expression, clarified escaping concern with `sed` delimiter since the project is not consistent there.
No need to check for a non-empty value to prepend a space (_since an empty value is used via sed anyway?_). Can also be a bit DRY with the sed pattern, matching the key + delimiter (_and all white-space before/after the delimiter until the value_), then capture that for the replacement left-side value while only actually swapping the value for the ENV input value. Should be an improvement, unless there is a scenario that would differ between `` and ` ` as valid value assignments?
polarathene
added
area/scripts
kind/improvement
georglauterbach
approved these changes
Sep 5, 2023
georglauterbach
previously approved these changes
Sep 5, 2023
casperklein
reviewed
Sep 9, 2023
Both should be improvements for usage Co-authored-by: Casper <[email protected]>
github-actions
bot
added
the
meta/stale
polarathene
added
stale-bot/ignore
casperklein
approved these changes
Oct 7, 2023
georglauterbach
added
the
meta/feature freeze
georglauterbach
removed
the
meta/feature freeze
georglauterbach
added
the
meta/feature freeze
georglauterbach
removed
the
meta/feature freeze
|
This is marked for v14.0.0. Will it be addresses for v14.0.0, or should we immediately postpone to v14.1.0? |
Same response to the related LDAP PR, that PR needs to be merged before this one can be. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Some refactoring improvements and fixes to the escaping support:
_replace_by_env_in_file():saslauthd.confwhich differs with:for it's key/value delimiter._escape_for_sed()helper function, instead of the inline sed pattern (which for some reason escaped a=? but not actual regex tokens(+), thus buggy if ever matching on input with those tokens).sedexpression, no need to prepend white-space, and we can additionally preserve the white-space between key and value of the source input while being more DRY._escape_for_sed():(+)(or even|), which is required for proper escaping support when usingsed -E/sed -rwith this helper function. It does now.&replacement segment token, which seems ok to escape in the regexp segment too (even though it has no meaning there). Increased verbosity to better grok pattern matching expression (easier when broken down, vs all in a single line), clarified an escaping concern with thesedsegment delimiter (since the project is not consistent there).NOTE: Tests will fail, this PR can be rebased upon #3524 which for the
mail_with_ldap.batstest at least should pass then, other tests can be updated here after that.Type of change
Checklist: