Skip to content
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

added support for chimesdkvoice voiceconnector - pull and event mode … #9398

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

shakiyal
Copy link

@shakiyal shakiyal commented Apr 2, 2024

…- tests added. Please review and merge my contribution. Thank you

@shakiyal shakiyal requested a review from kapilt as a code owner April 2, 2024 16:57
@shakiyal
Copy link
Author

shakiyal commented Apr 2, 2024

tests pass locally, failure appear to be in other code - dont understand, please help

@Johnlon
Copy link
Contributor

Johnlon commented Apr 2, 2024

Try ...

  1. comment out cfn_type and config_type
  2. add permission_prefix = 'chime'

Re 2) so it matches the code in https://awspolicygen.s3.amazonaws.com/js/policies.js - search for "chime"

return tagged_resources


@resources.register('chimesdkvoice-voiceconnector')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding resource name, chime-voice-connector .. sdk is generic in this context., and voice doesn't need a repeat.

Copy link
Contributor

@Johnlon Johnlon Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, not sure what you are proposing.
Can you clarify thanks.


The service api in boto is "chime-sdk-voice", so that seems a sensible prefix for this collection of resources. (The deprecated legacy API is "chime" but that's not the subject of this pr as far as I am aware). There is also the Aws offering ChimeSdkMeetings , these are Amazon's names for these things , IE "SDK" is actually in the name, which is unusual, but nonetheless a fact. So just using the aws given name seems most consistent.

Amongst the various resources Inside that chimesdkvoice offering we have resources named such as "phone number", "sip rules" as well as "voice profile" and "voice connector", so "voice connector" seems a logical and obvious name in this case.

And thus I guess the comitter ended up with ...

chime-sdk-voice.voiceconnector

Copy link
Contributor

@Johnlon Johnlon Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Whilst the boto API has "chime sdk voice" and "chime sdk meeting" the Aws console seems to have just "chime sdk" and it only has the voice resources.

The "chime sdk meetings" namespace and client is only accessible via the APIs .

So I dont know if all the different chime APIs and resources should be under merely "chimesdk" in the cloud custodian namespaces, or should we just make all the cc names map directly to Aws API names as this chime extension appears to be doing?

@kapilt
Copy link
Collaborator

kapilt commented Apr 3, 2024

@shakiyal the test failures are from meta tests that trying to validate metadata on the new resource. @Johnlon suggestions are spot on re fixes.

@shakiyal
Copy link
Author

shakiyal commented Apr 3, 2024

tests all pass now. @kapilt - Is a name change required? As per John's comment, the service is Chime SDK Voice and the resource type is Voice Connector - https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/chime-sdk-voice.html

@shakiyal
Copy link
Author

Hi @kapilt - can this branch be merged? Or does the resource name require updating?

@kapilt
Copy link
Collaborator

kapilt commented Apr 16, 2024

Hi @kapilt - can this branch be merged? Or does the resource name require updating?

just the resource name change and then I think its fine. we don't mirror the sdk name into the resource name if a shorter name suffices, chime-voice as a prefix suffices.

@shakiyal
Copy link
Author

@kapilt updates have been made to the name, let me know if this suffices

@shakiyal
Copy link
Author

shakiyal commented May 9, 2024

@kapilt - all updates are complete and should be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants