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

feat: Cli core upgrade #112

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

feat: Cli core upgrade #112

wants to merge 13 commits into from

Conversation

shrutiburman
Copy link

@shrutiburman shrutiburman commented Oct 12, 2022

Contributing to TwilioReasons to remove some e2e testcases:
Removed some tests because:

  • These testcases were deploying a new video app with actual api keys & secrets configured on circleci. It's unclear which account's creds are those. Also, second guessing the process of storing the secrets anywhere.
  • The existing testing suite of deploying apps yields a 404 error from serverless api. Failed Pipeline run

Current coverage report:
image

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@piyushtank
Copy link

We, SDK team attempted to review this PR but we do not have expertise on the codebase. So, we cannot give any meaningful feedbacks.

@@ -20,8 +20,8 @@
"@oclif/command": "^1.5.19",
"@oclif/config": "^1.14.0",
"@oclif/plugin-help": "^5.1.11",
"@twilio-labs/serverless-api": "^4.0.3",
"@twilio/cli-core": "^6.7.0",
"@twilio-labs/serverless-api": "^5.4.0",

Choose a reason for hiding this comment

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

@shrutiburman did you ensure this doesn't break any of the ahoy apps that rely on this plugin?

Copy link
Author

Choose a reason for hiding this comment

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

Wasn't aware of the ahoy apps. Have only tested the default cmds that the plugin has. Create, deploy and delete a video app. Can you help me with the ahoy apps implementation?

Copy link

@seancoleman2 seancoleman2 Oct 14, 2022

Choose a reason for hiding this comment

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

This plugin was created to to enable customers to build/deploy our Video reference apps. Here are instructions to deploy the React app: https://github.com/twilio/twilio-video-app-react#clone-the-repository

I would assume upgrading will be fine but just wanted to call it out.

Copy link
Author

Choose a reason for hiding this comment

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

Based on the readme file of the twilio-video-app-react, I did some basic acceptance testing. Gist here. It'd be great if you or someone from the video-app-react team could take a look at it.


const twilioClient = require('twilio')(process.env.TWILIO_API_KEY, process.env.TWILIO_API_SECRET, {

Choose a reason for hiding this comment

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

@shrutiburman are these account credentials configured by the customer when they deploy the React / iOS / Android app? Or are you saying these are associated with a Twilio account?

Copy link
Author

Choose a reason for hiding this comment

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

These account creds are configured on circle CI env and are referenced from there. These are associated with a Twilio account.

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