Improve handling of negations in applications array
#9
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
While _negations in the Applications type was being properly initialised, it wasn't copied on merges. This simple fix merely extends the list, which should work just fine i.e. I don't think it makes sense to ensure uniqueness here. Closes: madduck#44 Signed-off-by: martin f. krafft <[email protected]>
We add logic in `Applications.append_if_new()` to only insert non-negated items if they aren't already on our internal negation list. Additionally, we drop items from the negation list, once we've applied the negation once either in `append_if_new()` or in `merge_unique()` so that patterns like adding, then removing and then adding an item again have the expected result of the item being present in the final list. This fixes the issue where it wasn't possible to preemptively remove an entry from the applications list in a multi-dimensional hierarchy, e.g. in a [Commodore] global defaults repository where we may want to exclude applications for a certain Kubernetes distribution regardless of the cloud on which a cluster with that distribution is running. [Commodore]: https://syn.tools/commodore
simu
force-pushed
the
feat/robust-applications-negation-handling
branch
from
8c7e8a2
to
44065c8
Compare
4 tasks
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.
This PR improves handling of negated entries for the
applicationsarray.The first commit is cherry-picked from madduck@9c34784. The second commit adds a check to ensure that we don't add values which have previously been negated to the contents of
applications. Additionally the second commit removes negations from the internal negations list after they've been processed once. This ensures that we get the intuitively correct behavior for sequences like adding an item, then removing it and then adding it again, which should result in the item being present in the final list.The two commits cumulatively address the issue where it wasn't possible to preemptively remove an entry from the applications list in a multi-dimensional hierarchy, e.g. in a Commodore global defaults repository where we may want to exclude applications for a certain Kubernetes distribution regardless of the cloud on which a cluster with that distribution is running.
Please note that this change potentially modifies the behavior of configurations which contain negated entries in the
applicationsarray.