-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP!: r/aws-athena-workgroup added #7995
WIP!: r/aws-athena-workgroup added #7995
Conversation
Rebase Master
Signed-off-by: Marco Tesch <mtesch@tecracer.de>
Added skeleton of resourceAwsAthenaWorkgroupUpdate Function Signed-off-by: Marco Tesch <mtesch@tecracer.de>
Added first TestCase, still working on create/update/read functionality. #7667 might become an issue as the SDK does not allow to have an ARN for the WorkGroup, which is needed to list-tags. But still working on it. |
@bflad : Got some Travis CI Problems with the govet/nillness checks you have fixed here how I might recover my branch from that? |
@marcotesch the fixes need to be reviewed and merged into master first then we can restart the TravisCI build. You could also pull in my commits, but that could be troublesome if we change the implementation during review. |
Signed-off-by: Marco Tesch <mtesch@tecracer.de>
Signed-off-by: Marco Tesch <mtesch@tecracer.de>
Signed-off-by: Marco Tesch <mtesch@tecracer.de>
Looks good now @bflad. I will keep working on this, probably tags will not be ready for the first release version and get added later on, if thats fine with you? |
Athena workgroups provide the ability to follow costs using tag allocation for specific processing, without the tags, workgroups are useless ( for my usage ) |
Added Update Functionality for the Parameter Signed-off-by: Marco Tesch <mtesch@tecracer.de>
Added Updated Test for WorkGroup w. BSCPQ Param. Signed-off-by: Marco Tesch <mtesch@tecracer.de>
@florianlocqueneux: For sure there are UseCases where Tags are needed. But lets look at the plain resource functionality, for that tags are not needed at all. So imho a working resource might be the first merge and the tag support for said resource might be a second seperate merge. This does not imply that I'm not trying to squeeze tag-support in the resource right from the get go but there might be some problems wich are not that easy to overcome. |
Signed-off-by: Marco Tesch <mtesch@tecracer.de>
This is now the first functioning version without Tags, so far. I will now add some documentation, but the main code is ready for an official review (@bflad) at this point in time. |
@marcotesch wondering if you could fix the merge conflicts thanks! |
Would it be possible to get this one going? Sorry, I can't tell if this is difficult or not. It would be extremely useful to have this capability. Thanks. |
would also love to see it merged :) |
At the moment, I unfortunately don't have the time to write the needed documentation, which leads to the problem that this might not be merged any time soon. But for now I resolved the existing merge conflict. |
Hi @marcotesch 👋 Would it be okay if the maintainers or another community member finished up this functionality in followup commits? |
Hi,
Yes it would be absolutely fine and appreciated, I’m sorry that I can’t finish it up on my own at the moment.
|
@marcotesch no worries, you did great work and many people will be happy when the functionality is made available! We hope that you can continue contributing in the future. 😄 I will post a followup PR either later today or tomorrow that combines this with #8136 |
Followup PR which includes all the commits here is available at: #9290 Please follow that pull request for further updates and thank you again for all the hard work, @marcotesch! |
Thanks for pushing on with this. Much appreciated. Looking forward to putting it to use soon. |
thx a lot! |
#9290 has been merged and will release with version 2.19.0 of the Terraform AWS Provider, likely later today. 👍 |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Fixes #7650, #7667
Changes proposed in this pull request:
This is now WIP, Test Output will be added when available
First test(s) are written and running: