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.

Sign up for GitHub

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

Jump to bottom

feat: add new env commands #290

Merged
merged 3 commits into from
Jul 2, 2021
Merged

feat: add new env commands #290

merged 3 commits into from
Jul 2, 2021

Conversation

dkundel
Copy link
Member

@dkundel dkundel commented Jun 25, 2021

This change introduces a new set of commands around retrieving and modifying environment variables
from already deployed environments.

re #249

Contributing to Twilio

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.

@dkundel
feat: add new env commands
44fbbbb 
This change introduces a new set of commands around retrieving and modifying environment variables
from already deployed environments.

re #249
philnash
philnash suggested changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@philnash philnash left a comment

Choose a reason for hiding this comment

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

Realise this is a draft, but I had a couple of suggestions, so I added them.

packages/plugin-serverless/src/commands/serverless/env/get.js Outdated Show resolved Hide resolved
packages/plugin-serverless/src/commands/serverless/env/get.js Outdated Show resolved Hide resolved
packages/serverless-api/src/client.ts Show resolved Hide resolved
@dkundel dkundel marked this pull request as ready for review July 1, 2021 23:27
@dkundel dkundel mentioned this pull request Jul 1, 2021
Open
@dkundel
Copy link
Member Author

dkundel commented Jul 1, 2021

@philnash I implemented the class factory you suggested. I only implemented it for the new commands but I created #294 and #295 to implement it in the other commands and further remove the amount of copy paste code inside twilio-run.

philnash
philnash reviewed Jul 2, 2021
View reviewed changes
packages/twilio-run/src/cli.ts
(yargs) => {
yargs.command(EnvCommands.GetCommand);
yargs.command(EnvCommands.ListCommand);
yargs.command(EnvCommands.UnsetCommand);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you missing SetCommand and ImportCommand here?

@philnash
Copy link
Contributor

philnash commented Jul 2, 2021

That feels like a beautiful reduction in copy/paste! Everything else looks good.

@philnash philnash self-requested a review July 2, 2021 01:36
@philnash
Copy link
Contributor

philnash commented Jul 2, 2021

Also, makes sense to reuse this code once this has been merged to reduce the copy/paste elsewhere. Nice work!

@dkundel dkundel merged commit 7d11a03 into main Jul 2, 2021
@dkundel dkundel deleted the features/env-command branch July 2, 2021 19:29
@philnash philnash mentioned this pull request Jul 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philnash philnash philnash approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dkundel @philnash