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

Fix service name in template install message #5839

Merged
merged 2 commits into from Feb 19, 2019

Conversation

Projects
2 participants
@Xenonym
Copy link
Contributor

Xenonym commented Feb 16, 2019

What did you implement:

Closes #5802.

How did you implement it:

I had downloadTemplateFromRepo() return the service name instead of the final directory name, so that create.js and install.js can correctly display the service name both when creating / installing a service by default and when a service name is explicitly specified.

How can we verify it:

With default service name

serverless install --url https://github.com/mnylen/serverless-python-fake-context-timeout-issue should output:

Serverless: Downloading and installing "serverless-python-fake-context-timeout-issue"...
Serverless: Successfully installed "serverless-python-fake-context-timeout-issue"

With user-specified service name

serverless install --url https://github.com/mnylen/serverless-python-fake-context-timeout-issue --name timeout should output:

Serverless: Downloading and installing "serverless-python-fake-context-timeout-issue"...
Serverless: Successfully installed "serverless-python-fake-context-timeout-issue" as "timeout"

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • 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

@pmuens pmuens self-assigned this Feb 17, 2019

@pmuens pmuens added the pr/in-review label Feb 17, 2019

@pmuens pmuens added this to In progress in Serverless via automation Feb 17, 2019

@pmuens
Copy link
Member

pmuens left a comment

Hey @Xenonym thank you very much for taking the initiative to fix this 💯 👍

I just checked out your branch and tested it locally. Unfortunately I still see the "as undefined" 🤔.

Here's what I did (following your PR description):

serverless install --url https://github.com/mnylen/serverless-python-fake-context-timeout-issue
Serverless: Downloading and installing "serverless-python-fake-context-timeout-issue"...
Serverless: Successfully installed "serverless-python-fake-context-timeout-issue" as "undefined"

Am I missing something?

Would be really great if we could add a regression test here to ensure that this won't happen again in the future.

Let me know if you need any help here and thanks again for working on this 👍

Serverless automation moved this from In progress to Needs review Feb 18, 2019

Fix service name in template install message
- was previously displaying "undefined"

@Xenonym Xenonym force-pushed the Xenonym:fix/template-install-name-undefined branch from f67ebb7 to 63b2d61 Feb 18, 2019

@Xenonym

This comment has been minimized.

Copy link
Contributor Author

Xenonym commented Feb 18, 2019

I just checked out your branch and tested it locally. Unfortunately I still see the "as undefined" 🤔.

@pmuens Oops, sorry about that! Turns out I accidentally took out a check for this.options.name's falsyness, it should work now that I have put it back.

Would be really great if we could add a regression test here to ensure that this won't happen again in the future.

Not too sure how I should go about doing this: install() needs to get the service name from createTemplateFromRepo() to print the message, and I am not sure how to mock that in the tests since install() just directly calls it.

Serverless automation moved this from Needs review to Reviewer approved Feb 19, 2019

@pmuens

pmuens approved these changes Feb 19, 2019

Copy link
Member

pmuens left a comment

Hey @Xenonym thank you very much for getting back and updating this PR 👍

I just reviewed it and it works great! I also looked into the tests and added some unit tests. I agree, that truly testing it is quite hard and requires lots of mocks which defeat the prupose of having a clear end-to-end tests. Nevertheless I added some (although heavily mocked).

I'll merge the PR once the build is green! :shipit:

@pmuens pmuens added this to the 1.38.0 milestone Feb 19, 2019

@pmuens pmuens merged commit b3d4337 into serverless:master Feb 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Feb 19, 2019

@Xenonym Xenonym deleted the Xenonym:fix/template-install-name-undefined branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.