-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
INFRA-7967 - add resource tagging functions to boto_dynamodb exec module #51684
Conversation
76c91fd
to
1ddb638
Compare
@rallytime Is there anything needed on this, or it just still catchup from the holiday rush :) |
@dwoz or @garethgreenaway Can one of y'all review this? |
@tkwilliams @rallytime we are no longer taking changes against the develop branch, can either of you rebase against the master branch or perhaps there is another PR to reference and then close this one? If not we may close this as abandoned. |
@sagetherage It looks to me like the author could be waiting on someone from the Salt team to review this submission. No one responded for over a year since my request for someone on the Core team to take a look. That seems a little unfair to now consider this abandoned. |
@rallytime I cannot have this reviewed as-is and it will need intervention to get a review. I have attempted to contact @tkwilliams several times outside of this PR and had no response. |
So, a few things here:
1 I did this, and it took 5 minutes. I also added tests, which took a further 40 minutes. If I can do it, the core team certainly can. |
Hi @terminalmage @sagetherage and of course @rallytime -- HOWDY :) Many apologies, I'm no longer at the company where I wrote this (they were very kind in letting me devel LOTS of salt module code and contribute it back, but sadly they were bought, merged, and I became redundant in the face of the juggernaut that is Terraform - on AZURE no less :-/ ) I also find that GH whacked my salt project membership at some point - likely due to my laxness in setting up 2fa. Though I DID finally get off my butt and do it last month at least.... Anywho, I don't have any AWS access anymore, nor in truth any interest in dynamodb (boto or otherwise) - I only wrote this because we needed it at the time. So while it did in fact work GREAT when I PR'ed it (gosh, almost a year and a half ago), I'm not in a position to really do much with it. If someone wants to hack it back into shape that'd be awesome, but if it falls in the bucket due to bit-rot I won't lose any sleep either :) I have finally wangled my current boss into using salt-ssh so I may yet be able to contribute some more back to you folks, but we're in the process of moving all the legacy bare metal into GCS at the moment so I don't expect to be working on any boto code at all in the near-term. Havahappi! |
@tkwilliams The code was in decent shape and easily merged onto |
As always, M'sieur @terminalmage, you are a gentleman and a scholar :) |
De rien! |
@tkwilliams contact me directly if you want to discuss adding you back into the SaltStack GH org, it is complete up to you and I can help if that is helpful to you: sagetherage at saltstack dot com |
Note that the original module was written against boto2, which still has not added support for tagging dynamodb resources, so these new functions required adding boto3 imports in order to provide this functionality.