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

Integrating Components #6350

Merged
merged 4 commits into from
Jul 18, 2019
Merged

Integrating Components #6350

merged 4 commits into from
Jul 18, 2019

Conversation

eahefnawy
Copy link
Contributor

@eahefnawy eahefnawy commented Jul 9, 2019

What did you implement:

Integrates Serverless Components with the Serverless Framework.

How did you implement it:

Integrated with Serverless Components by simply depending on the components cli as a dependency and using its methods (I've kept all the required logic over there to keep things separate). Since components require at least Node 8, we are only integrating components for Node 8+ users.

One conflicting issue I've found is: what happens when there's an empty directory? I know @medikoo is working on an interactive serverless command, but the components project also ships with such a functionality. I think what we'll need to do is add support for components templates in @medikoo implementation, although I wish we could separate concerns. For now, the components check just moves on if it's an empty directory so that @medikoo updates kick in.

How can we verify it:

  1. run serverless create -t aws-nodejs - this should create a framework project
  2. run serverless deploy to deploy the newly created project
  3. delete all files from cwd
  4. in the same cwd, create a components serverless.yml file. For a minimal example
myBucket:
  component: "@serverless/aws-s3"
  1. run serverless again in the same cwd. This should run components instead of the framework as it detected that it's a components project

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

  • Write tests and confirm existing functionality is not broken.
    Validate via npm test
  • Write documentation
  • Ensure there are no lint errors.
    Validate via npm run lint-updated
    Note: Some reported issues can be automatically fixed by running npm run lint:fix
  • Ensure introduced changes match Prettier formatting.
    Validate via npm run prettier-check-updated
    Note: All reported issues can be automatically fixed by running npm run prettify-updated
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@eahefnawy eahefnawy mentioned this pull request Jul 9, 2019
8 tasks
@eahefnawy eahefnawy requested review from medikoo and pmuens and removed request for medikoo July 9, 2019 12:09
bin/serverless.js Show resolved Hide resolved
bin/serverless.js Outdated Show resolved Hide resolved
@medikoo medikoo added this to the 1.48.0 milestone Jul 9, 2019
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

I just tested it and it works fine.

I got an error when running the bucket example (NoSuchBucket) but I guess that this is intentional.

Then only thing that confused me was that the componentsCli has the logic which determines whether the serverless.yml is a Framework file or a Components file. As far as I understood it, the cli project is Components related (at least that what the project description on GitHub says).

I'd propose to add the check for whether it's a Framework or Componets project in the bin/serverless.js file and then call the relevant Components CLI commands if a components project is detected.

Another way would be to remove the emphasis on Components from the CLI project and build it in a consumer-agnostic way. In that case we should rename the variables in the bin/serverless.js file as suggested below.

bin/serverless.js Outdated Show resolved Hide resolved
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Just tested it again and it works fine 👍 (couldn't reproduce the Bucket error).

Thanks for updating the code 👍

LGTM :shipit:

@eahefnawy eahefnawy merged commit e58f52f into master Jul 18, 2019
@eahefnawy eahefnawy deleted the components-integ branch July 18, 2019 10:55
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