-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: master
Are you sure you want to change the base?
Conversation
chore: upgrade twilio-cli-core
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
const twilioClient = require('twilio')(process.env.TWILIO_API_KEY, process.env.TWILIO_API_SECRET, { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Contributing to TwilioReasons to remove some e2e testcases:
Removed some tests because:
Current coverage report: