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

Support for Github Entreprise in sls create #6332

Merged
merged 6 commits into from Jul 8, 2019

Conversation

Projects
2 participants
@kalote
Copy link
Contributor

commented Jul 4, 2019

What did you implement:

Closes #5332

How did you implement it:

Check wether the provided repository url contains 'github' keyword. I originally intended to also check if the url was a valid github entreprise url (using the /api/v3 endpoint as a verification endpoint), but i had trouble having the test running fine after, so i stopped at "if url is not github.com, but contains github, then it's a github entreprise url".

How can we verify it:

Provided you have a github entreprise setup:

./bin/serverless create --template-url https://github.mydomain.com/serverless-template/tree/master/non-prod/aws-nodejs --path test

./bin/serverless create --template-url https://username:password@github.mydomain.com/serverless-template/tree/master/non-prod/aws-nodejs --path test

I tested with my own setup (I'm working in a company that has its own dedicated github entreprise running on premise), and it worked fine.

or run the npm run test command.

Note: There was an issue with the auth management in lib/utils/downloadTemplateFromRepo.js - parseGitHubURL function. When using the 'download' library (that relies on 'got' module), it refuses to handle basic authentication in the url directly and asked for an auth option (see line 205 of node_modules/got/index.js). I change the mechanism in lib/utils/downloadTemplateFromRepo.js and updated the test accordingly.

Todos:

  • 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

@kalote kalote force-pushed the kalote:master branch from 1d6e17a to a03c18d Jul 4, 2019

@kalote kalote force-pushed the kalote:master branch from a03c18d to c18cf13 Jul 4, 2019

@pmuens pmuens self-assigned this Jul 4, 2019

@pmuens pmuens added this to In progress in Serverless via automation Jul 4, 2019

@pmuens pmuens added this to the 1.47.0 milestone Jul 4, 2019

@pmuens pmuens added cat/templates and removed cat/dx labels Jul 4, 2019

@pmuens
Copy link
Member

left a comment

Hey @kalote thanks for working on this! 👍

This looks good so far. I just added a comment about a minor change with respect to how we handle GitHub / GitHub Enterprise detection (basically moving the logic to detect the difference in the function which parses the GitHub URL.

In addition to that it seems like the Travis build is failing.

Let me know what you think and let us know if you need any help here.

Show resolved Hide resolved lib/utils/downloadTemplateFromRepo.js Outdated
Show resolved Hide resolved lib/utils/downloadTemplateFromRepo.js Outdated

Serverless automation moved this from In progress to Needs review Jul 4, 2019

@kalote kalote force-pushed the kalote:master branch from 73f53f2 to 57c1cc3 Jul 5, 2019

@kalote

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Fix. For the travis build, i ran the prettify command. Hopefully Travis will work now.

Serverless automation moved this from Needs review to Reviewer approved Jul 8, 2019

@pmuens
Copy link
Member

left a comment

Great! Thanks for working on / updating this PR @kalote 👍

I just did a final review and tested it thoroughly and it works like a charm! Will merge once Travis is done...

@pmuens pmuens dismissed their stale review via c234227 Jul 8, 2019

Serverless automation moved this from Reviewer approved to Needs review Jul 8, 2019

@pmuens

pmuens approved these changes Jul 8, 2019

Serverless automation moved this from Needs review to Reviewer approved Jul 8, 2019

@pmuens pmuens merged commit 650d55d into serverless:master Jul 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (medikoo) No manifest changes detected

Serverless automation moved this from Reviewer approved to Done Jul 8, 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.