Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(cwl): Change implementation of LiveTailRegistry to standard map #5789

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

keeganirby
Copy link

Problem

The nestedMap implementation of the registry was forcing a defined Default item. This doesn't fit the usecase I am aiming to provide by this registry. There is no default object to be returned. If an item doesn't exist in the registry, we should handle an undefined object, and not a placeholder. In other words, there is no concept of a placeholder/default value for a LiveTailSession.

Additionally, set on the NestedMap would perform a deep merge. I am wanting to replace the object.

Originally I defined the default object to an exception, but the NestedMap set operation calls get. And if there isn't already a value in the map, it would throw the exception defined as the default object. This means we could never add any item to the map.

Solution

Swap the underlying implementation of the registry to a basic Map.
I am still defining a LiveTailSessionRegistry class so we can statically initialize a registry singleton, as well as leaving the possibility open to override or extend the Map class. Also so we can pass around the type LiveTailSessionRegistry instead of Map<Foo, Bar>.


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@keeganirby keeganirby requested a review from a team as a code owner October 15, 2024 21:36
Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clear explanation!

@justinmk3 justinmk3 merged commit 56b7efd into aws:feature/cwltail Oct 15, 2024
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants