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(ses): getenv detects more errors #2690

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 13, 2025

Closes: #XXXX
Refs: #1710

Description

#1710 enhanced getEnvironmentOption with an optional third argument listing all the other allowed choices aside from the default choice. Previously, we had only manual code to detect and complain of unrecognized environment option values. #1710 provides a more declarative form, and reuses the checking-and-complaining logic. However, following #1710 we did not retire much of the previous ad-hoc manual code, nor did we make enough use of this declarative alternative. This PR fixes that.

Security Considerations

An advantage to the old ad-hoc manual style is that the enumeration of all possible alternatives was textually code to the dispatch on the alternative. This makes it easy to keep the two in sync. Thus, a comparative disadvantage of this PR is their separation, leading to maintenance hazards if one is updated but not the other. For example, if the declarative form adds another option that the dispatch code does not take into account, that other option might fall into the dispatch's fall-through case. This may or may not be good, depending on whether the fall through case is carefully chosen to also be appropriate for new not-previously-recognized options.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

The text of the error messages for unrecognized options will likely be different. But aside from that there should be no observable difference. Since we do not consider a change to the text of an error message a correctness concern, we consider this PR to effectively be a pure refactor.

Compatibility and Upgrade Considerations

Both before and after this PR, we generally reject unrecognized environment options settings. This makes it difficult to grow existing options with new settings that are ignored if seen by previous code. This hinders plausible development patterns. This PR by itself does not change that. But by centralizing the detect-and-complaint logic and making it more declarative, perhaps we become better set up to address this issue later.

@erights erights self-assigned this Jan 13, 2025
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 573a6d7 to 5a3139b Compare January 18, 2025 02:15
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 5a3139b to 3d29d44 Compare January 27, 2025 21:16
@erights erights marked this pull request as ready for review January 27, 2025 21:37
@erights erights requested review from kriskowal and mhofman January 27, 2025 21:37
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.

1 participant