Added 'workgroup' as optional argument to athena component #3254
Conversation
|
Hi @LeonardAukea. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @Jeffwan |
|
Thanks for the contribution! |
| ) | ||
| def query(client, query, database, output, workgroup=None): | ||
|
|
||
| if workgroup: |
There was a problem hiding this comment.
Is there more elegant way to handle optional arguments here? If we have extra configs, we will have lots of if else and nested if else logic here.
There was a problem hiding this comment.
Yeah, sorry about that @Jeffwan. The camelcase args are not super elegant either (boto3).
I can change it to:
arg_dict = dict(QueryString=query,
QueryExecutionContext={
'Database': database
},
ResultConfiguration={
'OutputLocation': output,
}
)
if workgroup:
arg_dict.update(dict(WorkGroup=workgroup))
response = client.start_query_execution(**arg_dict)Unless you have a better option?
There was a problem hiding this comment.
I accidentally added black formatting in the latest commit. Sorry in advance, if the changes are not as clear. But it's essentially the above. Except arg_dict is renamed to params
|
@Jeffwan If more AWS components are of interest to you guys. We could potentially add more. eg. S3 interactions. Since, we are writing them anyways. It's not too much of a hassle to make them a little neater and general enough. Are you planning to provide a set of "official" AWS reusable components? |
| QueryExecutionContext={"Database": database}, | ||
| ResultConfiguration={"OutputLocation": output,}, | ||
| ) | ||
| if workgroup: |
|
/ok-to-test |
That would be great. Could you create an issue and share the plans of components you like to add? Yes, managed services components are something we'd like to add and maintain. Current focus is more on the data plane and ML area. |
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mameshini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…3254) * added optional workgroup arg * Added a slighly more elegant way of handling query exec params * added args to yml and main() * added args to yml and main()
@Jeffwan Not sure if you want this here. But at least we needed this option. If you want me to commit more changes etc. I can do that too. I noticed that you had some todos.
This change is