config.c: Make ast_variable_retrieve return last match. #245
Conversation
|
cherry-pick-to: none |
github-actions
bot
added
testing-in-progress
test-checks-passed
test-gates-failed
and removed
testing-in-progress
labels
There was a problem hiding this comment.
I'm not comfortable putting this into master at this time. This is a fundamental change in behavior which should go into a standard release first to gather feedback from users. While it is reasonable to believe that it shouldn't impact people, in practice this doesn't always happen.
Makes sense, but I thought breaking changes were always typically master only anyways? If it's going into a standard release already, why wouldn't it also go into master, since that's always the furthest ahead? |
|
Once 21 was branched breaking changes went off the table unless absolutely necessary for some reason. The cut off point is when it is branched, not when the first release candidate occurs. The master branch will next be an LTS. |
Okay, so just to confirm, like some other things at the moment, this will need to wait for ~a year before it can be merged, i.e. it cannot go into either 21 or master at this time? |
|
Correct. |
|
Unless it was done in an optional manner that defaults to maintaining existing behavior. |
Got it. Could I suggest a new tag for issues/PRs, if it's not too much trouble? Something like breaking-change or what not, just so we can easily identify and keep track of things that will need to wait until such a time as breaking changes can be introduced again, e.g. this one and #258 |
I'd rather not add unneeded complexity to work around a bug just to get it merged sooner. I'll wait until next year so a proper fix can go in, that's fine. |
jcolp
added
the
waiting-for-standard-release-development-cycle
|
Label created and added. |
gtjoseph
force-pushed
the
master
branch
7 times, most recently
from
b15287c to
1862a36
Compare
|
@InterLinked1 This is eligible to go into master now since 23 will be a standard release. You'll need to rebase it on current master and change the cherry-pick line to "none". |
gtjoseph
removed
the
waiting-for-standard-release-development-cycle
ast_variable_retrieve currently returns the first match for a variable, as opposed to the last one. This is problematic because modules that load config settings by explicitly calling ast_variable_retrieve on a variable name (as opposed to iterating through all the directives as specified) will end up taking the first specified value, such as the default value from the template rather than the actual effective value in an individual config section, leading to the wrong config. This fixes this by making ast_variable_retrieve return the last match, or the most recently overridden one, as the effective setting. This is similar to what the -1 index in the AST_CONFIG function does. There is another function, ast_variable_find_last_in_list, that does something similar. However, it's a slightly different API, and it sees virtually no usage in Asterisk. ast_variable_retrieve is what most things use so this is currently the relevant point of breakage. In practice, this is unlikely to cause any breakage, since there would be no logical reason to use an inherited value rather than an explicitly overridden value when loading a config. ASTERISK-30370 #close Resolves: asterisk#244 UpgradeNote: Config variables retrieved explicitly by name now return the most recently overriding value as opposed to the base value (e.g. from a template). This is equivalent to retrieving a config setting using the -1 index to the AST_CONFIG function. The major implication of this is that modules processing configs by explicitly retrieving variables by name will now get the effective value of a variable as overridden in a config rather than the first-set value (from a template), which is consistent with how other modules load config settings.
InterLinked1
force-pushed
the
master-issue-244
branch
from
1367485 to
5cdf85e
Compare
github-actions
bot
added
pr-submit-testing-in-progress
and removed
test-checks-passed
test-gates-failed
labels
github-actions
bot
added
pr-submit-tests-passed
and removed
pr-submit-testing-in-progress
labels
github-actions
bot
added
pre-merge-testing-in-progress
pre-merge-checks-passed
merge-in-progress
and removed
merge-approved
pre-merge-testing-in-progress
labels
asterisk-org-access-app
bot
merged commit c157ee6
into
asterisk:master
|
Successfully merged to branch master. |
ast_variable_retrieve currently returns the first match for a variable, as opposed to the last one. This is problematic because modules that load config settings by explicitly calling ast_variable_retrieve on a variable name (as opposed to iterating through all the directives as specified) will end up taking the first specified value, such as the default value from the template rather than the actual effective value in an individual config section, leading to the wrong config.
This fixes this by making ast_variable_retrieve return the last match, or the most recently overridden one, as the effective setting. This is similar to what the -1 index in the AST_CONFIG function does.
There is another function, ast_variable_find_last_in_list, that does something similar. However, it's a slightly different API, and it sees virtually no usage in Asterisk. ast_variable_retrieve is what most things use so this is currently the relevant point of breakage.
In practice, this is unlikely to cause any breakage, since there would be no logical reason to use an inherited value rather than an explicitly overridden value when loading a config.
ASTERISK-30370 #close
Imported from Gerrit: https://gerrit.asterisk.org/c/asterisk/+/19744
Resolves: #244
UpgradeNote: Config variables retrieved explicitly by name now return the most recently overriding value as opposed to the base value (e.g. from a template). This is equivalent to retrieving a config setting using the -1 index to the AST_CONFIG function. The major implication of this is that modules processing configs by explicitly retrieving variables by name will now get the effective value of a variable as overridden in a config rather than the first-set value (from a template), which is consistent with how other modules load config settings.