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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works but seems backwards, it's easy to miss since the code is checking
process.env[ENV_VAR.EMAIL_NOTE]
in bothgetOptions
andgetEventArguments
. e.g. if a config file is introduced later, this needs to also check&& config_file.email_note === undefined
i think peter brought this up before, it might be better to have a default options object in
getOptions
. If there are any explicitly defined configs, overwrite the value with the highest priority config (command arguments > environment variables), or overwrite multiple times starting from lowest priorityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. will need check how to set default options object though because currently for cli options object is created with commander program, even setting env var is happening at that time for cli https://github.com/opensearch-project/reporting-cli/blob/main/src/arguments.js#L24 and want to keep main logic same for cli/lambda/package. getOptions is setting environment variable only for lambda & credentials as it has
:
. Anyway will add this also to refactor issue and try to prioritize.