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: support ssh format download template url #7588

Merged

Conversation

yugasun
Copy link
Contributor

@yugasun yugasun commented Apr 17, 2020

What did you implement

Support ssh format template url, like [email protected]/sample-service.git, actually, many company has their private gitlab, so I think supporting these format template urls is a must.

How can we verify it

I have add unit test for it:

it('should download the service based on a regular .git URL start with git@', () => {
  const url = '[email protected]/sample-service.git';

  return expect(downloadTemplateFromRepo(url)).to.be.fulfilled.then(() => {
    expect(spawnStub.calledOnce).to.equal(true);
    expect(downloadStub.calledOnce).to.equal(false);
    expect(spawnStub.args[0][0]).to.equal('git');
    expect(spawnStub.args[0][1][0]).to.equal('clone');
    expect(spawnStub.args[0][1][1]).to.equal(url);
  });
});

it('should download and rename the service based on a regular .git URL start with git@', () => {
  const url = '[email protected]/sample-service.git';
  const name = 'new-service-name';

  spawnStub.resolves({
    then: callback => {
      const slsYml = path.join(process.cwd(), 'new-service-name', 'serverless.yml');
      writeFileSync(slsYml, 'service: sample-service');
      callback();
    },
  });

  return expect(downloadTemplateFromRepo(url, name)).to.be.fulfilled.then(serviceName => {
    expect(spawnStub.calledOnce).to.equal(true);
    expect(downloadStub.calledOnce).to.equal(false);
    expect(spawnStub.args[0][0]).to.equal('git');
    expect(spawnStub.args[0][1][0]).to.equal('clone');
    expect(spawnStub.args[0][1][1]).to.equal(url);
    const yml = readFileSync(path.join(newServicePath, 'serverless.yml'));
    expect(yml.service).to.equal(name);
    expect(serviceName).to.equal('sample-service');
  });
});

Todos

Useful Scripts
  • npm run test:ci --> Run all validation checks on proposed changes
  • npm run lint:updated --> Lint all the updated files
  • npm run lint:fix --> Automatically fix lint problems (if possible)
  • npm run prettier-check:updated --> Check if updated files adhere to Prettier config
  • npm run prettify:updated --> Prettify all the updated files
  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

@hkbarton
Copy link
Contributor

@medikoo could you review this?

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@yugasun thanks for this update. Still I'm worried it won't work in most of the cases (it crashes for me locally)

Can you clarify why do we need that?

@@ -52,7 +52,7 @@ function validateUrl({ url, hostname, service, owner, repo }) {
* @param {String} url
*/
function isPlainGitURL(url) {
return url.startsWith('https') && url.endsWith('.git');
return (url.startsWith('https') || url.startsWith('git@')) && url.endsWith('.git');
Copy link
Contributor

@medikoo medikoo Apr 17, 2020

Choose a reason for hiding this comment

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

This url is then provided to download module.

Is it confirmed that it supports urls starting with git@ ?

I've just tried it locally, and it crashed for me with:

Error: Basic authentication must be done with the `auth` option` error

Copy link
Contributor Author

@yugasun yugasun Apr 17, 2020

Choose a reason for hiding this comment

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

@medikoo If you want to use this method, you should add your SSH key to your private gitlab setting. Only these who can use git clone command for these private repos. So you can use serverless create --template-url=git@xxx.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder is it good idea then to allow such urls. It requires an extra effort from user, which may not be obvious upfront, and error message is not meaningful.

Do https://.. urls not work with gitlab? Why do you feel it's important to support git@.. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my part, I have many private repos only can cloned by git@xxx format url, and I want to using this private project for serverless template. And if somebody want to use git@xxx url, they should know that they have ssh access right.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yugasun did you confirmed that this change works for you locally?

On my side it doesn't work (it crashes with Basic authentication must be done...), even though I can without issues clone repos via such url

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@medikoo Of course, this is a demo output:

# yugasun @ YUGASUN-MB0 in ~/Desktop/Playground [14:54:57] 
$ serverless create [email protected]:yugasun/fullstack-application-vue-demo.git
Serverless: Generating boilerplate...
Serverless: Downloading and installing "fullstack-application-vue-demo"...
Serverless: Successfully installed "fullstack-application-vue-demo" 

Before, you use this format url, you should ensure that you can git clone it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed I confirm it also works on my side

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @yugasun !

@medikoo medikoo merged commit d3bf39a into serverless:master Apr 20, 2020
@yugasun yugasun deleted the fix/support-ssh-format-git-template-url branch April 20, 2020 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants