Fix linode inventory filtering #4336
Merged
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
There was a regression introduced in the addition of caching. The `_consume_options` method was added and provided the `config_data` dictionary. This `pop`s every entry, resulting in an empty `config_data` dict, which was then reused and expected to be populated. After reviewing, `_consume_data` doesn't need to be called. Also, once the ``_read_config_data` method has been called, we no longer need the config_data dict, and can instead use the `get_option` method throughout. Once those were removed, the filtering function seemed a bit odd, since we were no longer using the file. I used that opportunity to move the filter calls into the populate function directly.
|
cc @Charliekenney23 @InTheCloudDan @LBGarber @decentral1se @displague @rmcintosh |
ansibullbot
added
bug
|
Realized I missed updating the tests and changelog entries. I will have those up shortly, and will mark this as a draft until it is ready for review. |
This removes tests that targeted some custom methods on configuration file handling. These are no longer necessary since they are now handled by the BaseInventoryPlugin `_read_config_data` method.
stvnjacobs
force-pushed
the
fix/linode-filter-options
branch
from
d0f0c56
to
5f0374f
Compare
ansibullbot
removed
WIP
|
Alright. Removed the tests that targeted some custom methods on configuration file handling which are no longer necessary and added the changelog fragment. |
felixfontein
reviewed
Mar 9, 2022
felixfontein
added
backport-4
check-before-release
This moves filters back into their own method, but now uses the get_option calls to pull filter configuration items.
stvnjacobs
force-pushed
the
fix/linode-filter-options
branch
from
609b844
to
855b9c9
Compare
ansibullbot
added
the
needs_revision
felixfontein
approved these changes
Mar 10, 2022
|
@wbh1 since you actually use this plugin (I hope :D ), could you take a look at this PR? Since you left a reaction I guess you already glanced at it (and apparently liked it), but some more explicit feedback would also be nice :) |
ansibullbot
removed
the
needs_revision
|
Ok, if nobody minds I'll merge by tomorrow morning. |
Can confirm that this works as expected / fixes the bug I introduced 😅 |
felixfontein
removed
the
check-before-release
Backport to stable-4: 💚 backport PR created✅ Backport PR branch: Backported as #4356 🤖 @patchback |
patchback bot
pushed a commit
that referenced
this pull request
Mar 14, 2022
* Fix linode inventory filtering There was a regression introduced in the addition of caching. The `_consume_options` method was added and provided the `config_data` dictionary. This `pop`s every entry, resulting in an empty `config_data` dict, which was then reused and expected to be populated. After reviewing, `_consume_data` doesn't need to be called. Also, once the ``_read_config_data` method has been called, we no longer need the config_data dict, and can instead use the `get_option` method throughout. Once those were removed, the filtering function seemed a bit odd, since we were no longer using the file. I used that opportunity to move the filter calls into the populate function directly. * Remove tests that target removed methods This removes tests that targeted some custom methods on configuration file handling. These are no longer necessary since they are now handled by the BaseInventoryPlugin `_read_config_data` method. * Add changelog entry for linode inventory bugfix * Revert filters back to their own method This moves filters back into their own method, but now uses the get_option calls to pull filter configuration items. (cherry picked from commit 386bb4b)
|
@stvnjacobs thanks for fixing this! |
felixfontein
pushed a commit
that referenced
this pull request
Mar 14, 2022
* Fix linode inventory filtering There was a regression introduced in the addition of caching. The `_consume_options` method was added and provided the `config_data` dictionary. This `pop`s every entry, resulting in an empty `config_data` dict, which was then reused and expected to be populated. After reviewing, `_consume_data` doesn't need to be called. Also, once the ``_read_config_data` method has been called, we no longer need the config_data dict, and can instead use the `get_option` method throughout. Once those were removed, the filtering function seemed a bit odd, since we were no longer using the file. I used that opportunity to move the filter calls into the populate function directly. * Remove tests that target removed methods This removes tests that targeted some custom methods on configuration file handling. These are no longer necessary since they are now handled by the BaseInventoryPlugin `_read_config_data` method. * Add changelog entry for linode inventory bugfix * Revert filters back to their own method This moves filters back into their own method, but now uses the get_option calls to pull filter configuration items. (cherry picked from commit 386bb4b) Co-authored-by: steven jacobs <stjacobs@fastmail.fm>
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.
SUMMARY
There was a regression introduced in the addition of caching. The
_consume_optionsmethod was added and provided theconfig_datadictionary. This
pops every entry, resulting in an emptyconfig_datadict, which was then reused and expected to be populated.
After reviewing,
_consume_datadoesn't need to be called. Also, oncethe
_read_config_datamethod has been called, we no longer need theconfig_datadict, and can instead use theget_optionmethodthroughout.
Once those were removed, the filtering function seemed a bit odd, since
we were no longer using the file. I used that opportunity to move the
filter calls into the populate function directly.
ISSUE TYPE
COMPONENT NAME
community.general.linode