Skip to content

Conversation

@JeremieRodon
Copy link

For the AWS EMF and XRay exporters, changing their respective 'region' argument so that, if unset, it tries to use AWS_REGION and AWS_DEFAULT_REGION environment variables (in that order) before defaulting to us-east-1.

Rational: from an AWS user perspective, it is expected that the default behavior is to use whatever default region the user has set, if any.

All changes:

  • Implemented the Default trait on exporters::shared::aws::Region
  • Implemented the core::str::FromStr trait on exporters::shared::aws::Region so we can parse strings in a more controlled manner
  • Deferred the previous From<String> trait implementation to str::parse()
  • Removed the clap::value_enum flag on the XRay/EMF arguments because the clap::ValueEnum trait is not implemented on exporters::shared::aws::Region anyway
  • Updated the documentation of both configuration arg to reflect the change

…region' argument so that, if unset, it tries to use AWS_REGION and AWS_DEFAULT_REGION (in that order) before defaulting to us-east-1. Rationnal: from an AWS user perpective, it is expected that the default behavior is to use whatever default region the user has set, if any.

All changes:
- Implemented Default on the exporters::shared::aws::Region struct
- Implemented core::str::FromStr on the Region struct so we can parse strings in a more controlled maner
- Defered the previous From<String> impl to str::parse()
- Removed the clap::value_enum flag on the XRay/EMF arguments because the clap::ValueEnum trait is not implemented on Region anyway
- Updated the documentation of both configuration arg to reflect the change
@mheffner
Copy link
Contributor

Thanks @JeremieRodon , I'll take a look at this tomorrow.

Copy link
Contributor

@mheffner mheffner left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the PR!

@mheffner
Copy link
Contributor

I have a PR that should fix the workflows to run here: #256

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