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

Add vonage video api support #845

Open
wants to merge 28 commits into
base: vonage
Choose a base branch
from

Conversation

haocheng2023
Copy link

No description provided.

Copy link
Collaborator

@jeffswartz jeffswartz left a comment

Choose a reason for hiding this comment

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

This PR appears to disable the use of OpenTok credentials and require Vonage credentials (if we target the master branch). Do we want to create a new vonage-application-support branch (or whatever we want to call it), and have this PR target that branch? Or do we want to change this PR so that it lets you use either OpenTok or Vonage credentials (based on the environment variables on the system)?

Copy link
Collaborator

@jeffswartz jeffswartz left a comment

Choose a reason for hiding this comment

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

This PR appears to disable the use of OpenTok credentials and require Vonage credentials (if we target the master branch). Do we want to create a new vonage-application-support branch (or whatever we want to call it), and have this PR target that branch? Or do we want to change this PR so that it lets you use either OpenTok or Vonage credentials (based on the environment variables on the system)?

@haocheng2023 haocheng2023 changed the base branch from master to vonage-application-support November 29, 2023 21:47
@jeffswartz jeffswartz changed the base branch from vonage-application-support to vonage November 29, 2023 22:27
@opentok opentok deleted a comment from haocheng2023 Nov 29, 2023
export TB_API_KEY=<key>
export TB_API_SECRET=<secret>
export VONAGE_APPLICATION_ID=<application-id>
export VONAGE_PRIVATE_KEY=<private-key>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code uses VONAGE_PRIVATE_KEY_PATH, right? I actually think VONAGE_PRIVATE_KEY is better, because this can be a path to a private key or the actual private key String, right? If so, please change throughout.

(error) => {
if (error) {
return this.logger.log('Get archives error:', error);
}
return false;
},
}
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lots of linting errors (here and in other files). Please run npm run lint.

Comment on lines 30 to 32
// E.OPENTOK_PRECALL_API_KEY = { envVar: 'TB_PRECALL_API_KEY', jsonPath: 'precallTest.apiKey' };

E.OPENTOK_PRECALL_API_SECRET = { envVar: 'TB_PRECALL_API_SECRET', jsonPath: 'precallTest.apiSecret' };
// E.OPENTOK_PRECALL_API_SECRET = { envVar: 'TB_PRECALL_API_SECRET', jsonPath: 'precallTest.apiSecret' };
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I guess we're using the same app ID and secret for the precall session. That makes sense. I think it was a mistake early to have the app use a separate API key and secret for the precall session. (It should use a separate session ID.)

@@ -141,7 +141,7 @@ function ServerMethods(aLogLevel, aModules) {

const credentials = new Auth({
applicationId,
privateKey: privateKeyPath,
privateKey: privateKey,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
privateKey: privateKey,
privateKey,

You can set the `VONAGE_APPLICATION_ID` and `VONAGE_PRIVATE_KEY` environment variables to
your Vonage application ID and private key. For example, the following shell commands export
these values for use by the app (replace `<application-id>` and `<private-key>` with your
Vonage application ID and private key):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Vonage application ID and private key):
Vonage application ID and either the private key or path to the private key):

For example, the following shell commands export these values for use by the app
(replace `<key>` and `<secret>` with your OpenTok API key and the corresponding API secret):
You can set the `VONAGE_APPLICATION_ID` and `VONAGE_PRIVATE_KEY` environment variables to
your Vonage application ID and private key. For example, the following shell commands export
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
your Vonage application ID and private key. For example, the following shell commands export
your Vonage application ID and private key (or path to the private key). For example, the following shell commands export

@joliveraortega
Copy link

@jeffswartz or @haocheng2023 Is anything here left?, can this be merged in?, will this get to master branch? Please, advise. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants