Added a new env variable for crossing-region scenario (app migration)

[ymao2]

This PR is to address athena connection issue, for one of dashboard apps ( gold-scorecard-form (https://github.com/moj-analytical-services/gold-scorecard-form)

The platform we are going to use for all the AP dashboard apps is Cloud-platform (central moj infrastructure), their AWS resources are managed under different account and region they use is eu-west-2, but our athena service and all the datasets (buckets) we manage are under admin-data account with region eu-west-1, so there are involved some cross-accounts and cross-region issue.

Last week we managed to get the cross-account permission work, then found out the region on AWS_DEFAULT_REGION and AWS_REGION are linked to the underline cluster, we cannot overwrite it

This PR is to introduce an new alternative env var for Rdbtools to retrieve the default region of s3 buckets ( AWS_ATHENA_QUERY_REGION), it will give benefit of requiring only minor changes to other apps using this R pacakge

@mratford @pjrh-moj , what do you think? keen to hear your feedback, as we want to get the app change done asap

[julialawrence]

LGTM

[pjrh-moj]

The alternative is just to set the aws_region argument of connect_athena() - is there some reason you can't just do that?

Otherwise, I think that's fine. Can you put a note explaining this into the comment above get_region() and also the @param aws_region docstring for connect_athena()?

Thanks!

[ymao2]

The alternative is just to set the aws_region argument of connect_athena() - is there some reason you can't just do that?

Otherwise, I think that's fine. Can you put a note explaining this into the comment above get_region() and also the @param aws_region docstring for connect_athena()?

Thanks!

Thank you very much for reviewing PR 👍 .

Yes, I knew the region can be passed from the scripts/codes using this package, the only reason for raising this PR is that it require quite minor changes from app code side and hide such detail from them 😳 , no other strong reason. Introducing a new env var does make the package itself less universal, but since the PR had some code already specific to DP , m codes hopefully doesn't make it worse 😳

I will update the param and doc

[pjrh-moj]

I'm all for hiding details from users! Thanks for finding this issue and providing a fix!

[ymao2]

I added some notes in the readme and the function itself, have a look to see whether they make sense or not, thank you!

[ymao2]

@pjrh-moj and @mratford, I am going to merge this PR, would it be possible to make a new release after that ? we are trying to update one of dashboard apps for app migration, and we are blocking by this small change, once the new release is made, then I can update this app to use new version

thank you! 😳

[pjrh-moj]

released as v0.3.0 - hope the migration goes smoothly!