Closed ymao2 closed 1 year ago
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!
The alternative is just to set the
aws_region
argument ofconnect_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 forconnect_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
I'm all for hiding details from users! Thanks for finding this issue and providing a fix!
I added some notes in the readme and the function itself, have a look to see whether they make sense or not, thank you!
@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! 😳
released as v0.3.0 - hope the migration goes smoothly!
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 regioneu-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
andAWS_REGION
are linked to the underline cluster, we cannot overwrite itThis 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