Skip to content

refactor(api): extract file system logic from API layer #8

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

Merged
merged 1 commit into from
Jul 24, 2019
Merged

refactor(api): extract file system logic from API layer #8

merged 1 commit into from
Jul 24, 2019

Conversation

stefanjudis
Copy link
Contributor

Extract file system logic from APi layer. :)

fix #7

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.

@stefanjudis stefanjudis changed the title refactor(api): extract file system logic from API layer WIP refactor(api): extract file system logic from API layer Jul 23, 2019
@stefanjudis stefanjudis changed the title WIP refactor(api): extract file system logic from API layer refactor(api): extract file system logic from API layer Jul 23, 2019
@stefanjudis
Copy link
Contributor Author

@dkundel I'd be up for an opinion here. 🙈 It may be that i'm not seeing the whole picture. :)

@stefanjudis
Copy link
Contributor Author

stefanjudis commented Jul 23, 2019

Issues I ran into:

https://github.com/twilio-labs/serverless-api/blob/master/src/api/functions.ts#L92-L94

This part only works when you're dealing with the file system. With configurations that are passed into deployProject the friendly name shouldn't be bound to the function path. :) I ran into a case with deployProject where it wasn't updating a function because of this logic. So i started extracting the file system from the API layer.

https://github.com/twilio-labs/serverless-api/blob/master/src/api/functions.ts#L178-L185

IMO upload functions shouldn't deal with this. :) Additional logic in the API layer makes the functionality very hard to grasp when you're dealing with the methods programmatically.

The same things apply for the asset part. :)


Overall, the proposed change is that the API layer only deals with API calls – not much additional logic.
To build configuration from the file system getListOfFunctionsAndAssets now deals completely with file names and extensions to come up with the configuration. :)

/**
* Path for the serverless function
*/
path?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be mandatory. Every Function or Asset should have a path.

Copy link
Contributor

Choose a reason for hiding this comment

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

path was a the filesystem one before and name was the actual path. Why would you have name now? See comment above

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move the example into the comment here of how those paths would look like. I.e. starting with a /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made path mandatory and answered your comments below. 👇

#8 (comment)

@stefanjudis
Copy link
Contributor Author

@dkundel Thanks for looking into this. I addressed your comments and pushed some updates. :)

@stefanjudis stefanjudis merged commit 1dff153 into twilio-labs:master Jul 24, 2019
@stefanjudis stefanjudis deleted the extract-file-logic branch July 24, 2019 16:13
dkundel added a commit to twilio-labs/serverless-toolkit that referenced this pull request Jul 24, 2019
* fix: update code for new version of severless-api

re twilio-labs/serverless-api#8

* chore(npm): upgrade @twilio-labs/serverless-api

* fix(tests): fix integration tests

Remove the reliance on stacktraces in the error integration tests
dkundel added a commit to twilio-labs/serverless-toolkit that referenced this pull request Jul 26, 2019
* feat: introduce config file functionality (#15)

With this change we are starting to allow .twilio-functions for more than just storing some build outcomes. It supports grouping configs by environment, command or project or nested combinations of these.

Very extended config:

```json
{
	"startConfig": {
		"ngrok": "dom",
		"inspect": ""
	},
	"environments": {
		"dev": {
			"deployConfig": {
				"env": ".env"
			}
		},
		"prod": {
			"deployConfig": {
				"env": ".env.prod"
			}
		}
	},
	"projects": {
		"ACc2bdaa19578061b45a518axxxxxxxxxx": {
			"serviceSid": "ZSd6e3037c5710c8e8c979f5xxxxxxxxxx",
			"latestBuild": "ZBdf824389570e3ee2ebcec7xxxxxxxxxx",
			"deployConfig": {
				"environment": "prod"
			}
		}
	},
	"serviceSid": "ZSd6e3037c5710c8e8c979fxxxxxxxxxx",
	"latestBuild": "ZBdf824389570e3ee2ebcec7xxxxxxxxxx"
}
```

fix: #15

* feat: add config support for list,activate,deploy (#15)

This commit adds config file support for the list, activate and deploy
command.

It also deprecates the --functions-env flag

BREAKING CHANGE: Deprecating --functions-env as an option

fix: #15, fix: #27

* refactor(config): factor out common code for testability

* fix(config): write serviceSid to config

This creates compatibility of config files for twilio-run

* test(config): add basic test for list config

* feat(runtime): handle invalid account sid & new error page

This change will check for valid Account SIDs in local development mode and create an appropriate
error message. It also introduces a new Error response page

BREAKING CHANGE: Error page layout changed

fix #45

* chore(release): 2.0.0-beta.13

* fix: update code for new version of severless-api (#46)

* fix: update code for new version of severless-api

re twilio-labs/serverless-api#8

* chore(npm): upgrade @twilio-labs/serverless-api

* fix(tests): fix integration tests

Remove the reliance on stacktraces in the error integration tests

* chore(release): 2.0.0-rc.0

* chore(npm): upgrade serverless-api library

* chore(release): 2.0.0-rc.1

* feat(new): change from prompts to inquirer (#36)

Prompts has a bug for screens that are smaller than the content. This fix hasn't been launched yet
so we are switching to inquirer.

fixes #36

* fix(templates): switch template list endpoint to next branch

The templates are currently pulled from the next branch but the list was from master, causing some
templates to crash

* feat: introduce config file functionality (#15)

With this change we are starting to allow .twilio-functions for more than just storing some build
outcomes. It supprts grouping configs by environment, command or project or nested combinations of
these

fix: #15

* feat: add config support for list,activate,deploy (#15)

This commit adds config file support for the list, activate and deploy
command.

It also deprecates the --functions-env flag

BREAKING CHANGE: Deprecating --functions-env as an option

fix: #15, fix: #27

* refactor(config): factor out common code for testability

* fix(config): write serviceSid to config

This creates compatibility of config files for twilio-run

* test(config): add basic test for list config

* feat(config): accept external config options

* test(config): add more tests for configs

* fix(config): store under accountSid not API key
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.

Extract file system access from funtions like getOrCreateFunctionResources and getOrCreateAssetResources
2 participants