Enables service installation with a name #2616

laardee · 2016-11-03T21:12:08Z

What did you implement:

This PR enables to rename the service when installing from GitHub, for example serverless install --name new-service-name --url https://github.com/SC5/serverless-messenger-boilerplate will install serverless-messenger-boilerplate to folder new-service-name and change service value in serverless.yml to new-service-name.

Closes #2626

How did you implement it:

Added new parameter name to install class and renameService helper function for renaming service in yaml file.

How can we verify it:

serverless install --name new-service-name --url https://github.com/SC5/serverless-messenger-boilerplate should install service to new-service-name folder and change service name to new-service-name.

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification config/commands/resources
Change ready for review message below

Is this ready for review?: YES

laardee · 2016-11-03T21:15:15Z

@pmuens, I noticed that you wrote the original download test, what kind of a test should I write for this? The current installation test just seems to mock download or something like that (the url is invalid).

pmuens · 2016-11-04T11:56:01Z

Hey @laardee. Great addition to the install command! I like it 👍 💯

Hmm. Couldn't you stub the download again and return something there (pretending the download succeeds). And after that test if the renameService function is called.
Furthermore you can unit test the renameService in isolation.

Maybe @eahefnawy and @flomotlik can add some thoughts here.

Today I stumbled upon an improvement for the proxyquire usage. I've implemented it here:
https://github.com/serverless/serverless/blob/master/lib/classes/Utils.test.js#L21-L29

Basically it should be moved in the beforeEach() so that you get a fresh stub every time.

Let me know if you need any help!

laardee · 2016-11-04T13:13:51Z

@pmuens thanks, I wrote unit test for renameService af53bcb, it creates serverless.yml and package.json files (only service and name keys), then it uses renameService to change those and verifies that.

I also moved downloadStub to beforeEach as you suggested and added one expect to check that the path that is created contains the new name for service df499e7.

pmuens · 2016-11-04T14:28:06Z

Hey @laardee thank you. Sounds good! 👍

Will review and take a deep dive ASAP.

laardee · 2016-11-05T07:36:03Z

I changed yml read/write in renameService function to use plain fs module with string replace to preserve comments in serverless.yml, js-yaml module doesn't seem to support comments nodeca/js-yaml#305.

pmuens

Hey @laardee thanks for your work on this one and the requested updates 💯 ! Great job 👍 Really like the functionality!

I added a few comments.

Pinging @eahefnawy and @flomotlik for feedback as well!

pmuens · 2016-11-07T09:36:10Z

docs/providers/aws/cli-reference/install.md

+```
+
+This example will download the .zip file of the `authentication` service from GitHub, create a new directory with the name `my-authentication` in the current working directory, unzips the files in this directory and renames service to `my-authentication` if `serverless.yml` exists in the service root.


"... and renames the service"

pmuens · 2016-11-07T09:37:48Z

lib/plugins/install/install.js

@@ -22,6 +24,11 @@ class Install {
            required: true,
            shortcut: 'u',
          },
+          name: {
+            usage: 'Name for the service',
+            required: false,


This could be left out as it means that same as require: false (AFAIK we don't follow this pattern in other plugins).

pmuens · 2016-11-07T09:40:28Z

lib/plugins/install/install.js

+          .then(contents => this.serverless.utils.writeFile(packageFile, contents)),
+      ])
+        .then(() => resolve(` as "${name}"`))
+        .catch((error) => {


This could be enhanced following this pattern we use throughout the codebase (

serverless/lib/plugins/aws/deploy/compile/events/apiGateway/lib/validate.js

Lines 43 to 50 in 5430fd8

const errorMessage = [

`Invalid http event in function "${functionName}"`,

' in serverless.yml.',

' If you define an http event, make sure you pass a valid value for it,',

' either as string syntax, or object syntax.',

' Please check the docs for more options.',

].join('');

throw new this.serverless.classes.Error(errorMessage);

).

Or you could let Serverless catch the error and remove the catch block altogether.

I refactored this to be synchronous and used this.serverless.classes.Error for throwing error. Error message could still be better.

pmuens · 2016-11-07T09:43:28Z

lib/plugins/install/tests/install.js

@@ -77,5 +129,18 @@ describe('Install', () => {
        expect(downloadStub.args[0][0]).to.equal(`${install.options.url}/archive/master.zip`);
      });
    });
+
+    it('should download and rename the service based on the GitHub URL', () => {


Can we enhance this test somehow that the call to the renaming method is tested as well?

@pmuens, so basically either serverless.yml should be created to the download folder beforehand or downloadStub should pass zip containing serverless.yml as a payload. The first one might not work, because file should be created to folder before running the install function, which checks if the folder exists and if so it won't install the package. I'll check if I can pass the zip as a payload.

Now the downloadStub mocks the zip loading and saves serverless.yml, then installation process renames the service value and finally the result is tested.

pmuens · 2016-11-07T09:45:21Z

lib/plugins/install/install.js

+        readFile(serviceFile, 'utf-8')
+          .then(contents =>
+            contents.replace(/service\s*:.+/gi, (match) => {
+              const fractions = match.split('#');


Are commented out parts in a serverless.yml file that common?

I don't know about others, but at least we have some comments on our workshop boilerplates and other similar project templates.

pmuens · 2016-11-07T09:47:14Z

lib/plugins/install/install.js

+              return fractions.join(' #');
+            }))
+          .then(contents => writeFile(serviceFile, contents)),
+        this.serverless.utils.readFile(packageFile)


Does a service always have a package.json file?

laardee · 2016-11-08T20:59:54Z

@pmuens, ready for review round two 😅

pmuens · 2016-11-09T13:42:25Z

Hey @laardee
Thank you very much for the updates to this PR! 👍
I followed this PR the other days and it looks really promising!

I take a deep dive later again because we're currently focused on 1.2 (https://github.com/serverless/serverless/milestone/16) to get this out of the door next week.

Again. Thanks for working on this really good addition! 👍 💃

laardee · 2016-11-26T17:32:00Z

@pmuens, have you already had time to look into this? I know this is not the first priority PR so I don't mind waiting and rebasing 😄

pmuens · 2016-11-27T11:42:23Z

@laardee thanks for getting back. 👍

Unfortunately I didn't have any time yet 😞 . But we'll plan the next milestones soon and will prioritize this PR soon. Sorry for the inconveniences.

laardee · 2016-12-10T23:15:15Z

@pmuens, I fixed the code to work with #2816. Unit tests are passing but the lint fails because of the metrics plugin. Is someone fixing that already?

…rless.yml

laardee · 2016-12-11T12:39:48Z

rebased, tests are now passing 👌

laardee · 2016-12-19T07:23:25Z

I wonder if 🎅 comes before next review round. 😀

pmuens

Hey @laardee sorry for the slow response / review on our end!

🎅 is finally here 😆 ! I looked over the code today and tested it. Works great! Good job 👍

I just pushed some minor changes on top of your PR. Basically the tests are now side-effect free and more predictable.

GTM from my side. Waiting for another Approval and then we can merge it! 👍

eahefnawy

Thanks @laardee & @pmuens for your work here. Works as expected!

laardee changed the title ~~Enables service installation with a new name~~ Enables service installation with a name Nov 4, 2016

laardee force-pushed the install-with-name branch from 49d99b7 to bd37e27 Compare November 4, 2016 11:20

laardee force-pushed the install-with-name branch from 4ea3510 to 80b1bb7 Compare November 5, 2016 13:03

pmuens suggested changes Nov 7, 2016

View reviewed changes

laardee force-pushed the install-with-name branch from cba0f00 to 165cd42 Compare November 8, 2016 20:12

laardee force-pushed the install-with-name branch from 5822407 to 001dc0f Compare November 14, 2016 13:24

pmuens added the status/2-code-review label Nov 20, 2016

laardee force-pushed the install-with-name branch 2 times, most recently from 40462ac to dcdbbda Compare November 26, 2016 14:18

laardee force-pushed the install-with-name branch from dcdbbda to 81a19eb Compare December 2, 2016 08:40

laardee force-pushed the install-with-name branch from 81a19eb to 24ccca2 Compare December 10, 2016 23:05

laardee added 9 commits December 11, 2016 14:33

enables service installation with name

27c7fbf

fixes lint issue

dc43e76

adds install with name documentation

4b64812

adds renaming of package.json name value

5397b68

adds renamed variable for skipping renaming

4d329ed

adds missing semicolon

c134760

adds unit test for install.renameService function

e51991b

moves donwloadStub and Install inside beforeEach

bceb6da

adds test for folder name

068059e

laardee added 9 commits December 11, 2016 14:33

changes yml service replacing to use fs to preserve comments in serve…

1850c18

…rless.yml

fixes lint issues

2d0bbfd

changes tests to return promise

9727195

changes renameService to ignore non-existing package.json

b7b6fa3

adds test for renaming service

246ca4e

adds creation of tmp folder that is used for storing generated files

13d8560

changes renameService to sync, serverless.classes.Error for error

31933a4

adds missing return

2cdfe72

fix functionality to work with serverless#2816

2829566

laardee force-pushed the install-with-name branch from 24ccca2 to 2829566 Compare December 11, 2016 12:34

Refactor and simplify tests

72103fa

pmuens approved these changes Dec 20, 2016

View reviewed changes

Refactor displaying of renaming on console

111e7ff

pmuens added this to the 1.5 milestone Dec 22, 2016

eahefnawy approved these changes Jan 2, 2017

View reviewed changes

eahefnawy merged commit 99f9073 into serverless:master Jan 2, 2017

laardee deleted the install-with-name branch January 2, 2017 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables service installation with a name #2616

Enables service installation with a name #2616

laardee commented Nov 3, 2016 •

edited

laardee commented Nov 3, 2016 •

edited

pmuens commented Nov 4, 2016

laardee commented Nov 4, 2016

pmuens commented Nov 4, 2016

laardee commented Nov 5, 2016

pmuens left a comment

pmuens Nov 7, 2016

laardee Nov 8, 2016

pmuens Nov 7, 2016

laardee Nov 8, 2016

pmuens Nov 7, 2016

pmuens Nov 7, 2016

laardee Nov 8, 2016

pmuens Nov 7, 2016

laardee Nov 8, 2016

laardee Nov 8, 2016

pmuens Nov 7, 2016

laardee Nov 8, 2016

pmuens Nov 7, 2016

laardee Nov 8, 2016

laardee commented Nov 8, 2016

pmuens commented Nov 9, 2016

laardee commented Nov 26, 2016

pmuens commented Nov 27, 2016 •

edited

laardee commented Dec 10, 2016

laardee commented Dec 11, 2016

laardee commented Dec 19, 2016

pmuens left a comment •

edited

eahefnawy left a comment

		```

		This example will download the .zip file of the `authentication` service from GitHub, create a new directory with the name `my-authentication` in the current working directory, unzips the files in this directory and renames service to `my-authentication` if `serverless.yml` exists in the service root.

	const errorMessage = [
	`Invalid http event in function "${functionName}"`,
	' in serverless.yml.',
	' If you define an http event, make sure you pass a valid value for it,',
	' either as string syntax, or object syntax.',
	' Please check the docs for more options.',
	].join('');
	throw new this.serverless.classes.Error(errorMessage);

Enables service installation with a name #2616

Enables service installation with a name #2616

Conversation

laardee commented Nov 3, 2016 • edited

What did you implement:

How did you implement it:

How can we verify it:

Todos:

laardee commented Nov 3, 2016 • edited

pmuens commented Nov 4, 2016

laardee commented Nov 4, 2016

pmuens commented Nov 4, 2016

laardee commented Nov 5, 2016

pmuens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laardee commented Nov 8, 2016

pmuens commented Nov 9, 2016

laardee commented Nov 26, 2016

pmuens commented Nov 27, 2016 • edited

laardee commented Dec 10, 2016

laardee commented Dec 11, 2016

laardee commented Dec 19, 2016

pmuens left a comment • edited

Choose a reason for hiding this comment

eahefnawy left a comment

Choose a reason for hiding this comment

laardee commented Nov 3, 2016 •

edited

laardee commented Nov 3, 2016 •

edited

pmuens commented Nov 27, 2016 •

edited

pmuens left a comment •

edited