-
Notifications
You must be signed in to change notification settings - Fork 373
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
feat: implement create site from template feature #3948
Conversation
📊 Benchmark resultsComparing with 127a1c6 Package size: 368 MB⬇️ 0.00% decrease vs. 127a1c6
Legend
|
3022af0
to
e79b826
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing such an awesome feature 🐳 Overall a very nice done PR! Only some minor changes.
Can you please add a small unit test with ava
that mocks the api requests away but tests the flow of this command?
Even though it is beta but beta often moves to prod by just removing the word beta 😂
console.log( | ||
`Choose a unique site name (e.g. ${siteSuggestion}.netlify.app) or leave it blank for a random name. You can update the site name later.`, | ||
) | ||
const { name: nameInput } = await inquirer.prompt([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a possibility to skip that input if it is optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was mostly copied from the sites:create
command. I think in general i'd wanna give people the choice to pick a title or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @charliegerard and @lukasholzer, I added a few comments of my own too
throw new Error('Duplicate repo') | ||
} | ||
} else { | ||
site = await api.createSiteInTeam({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create the site, but does not set up the CI part. Meaning a commit to the repo won't trigger a build on Netlify.
See
cli/src/commands/sites/sites-create.js
Line 127 in 40b1cf9
if (options.withCi) { |
Thanks for the feedback @lukasholzer & @erezrokah! Working through it now and will ask for another review soon, thanks! |
00f1991
to
1204098
Compare
|
||
return name | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are only to be able to extract the piece of code that is reused in the sites-create-template.js
file to avoid duplication, the logic isn't changed
*/ | ||
|
||
test.skip('netlify sites:create-template', async (t) => { | ||
const siteTemplateQuestions = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukasholzer I tried to add some tests for this command but I've been struggling and could do with some guidance on how to mock the API calls to Github if you have some idea or if you know somewhere else in the code I should look at, please? 🙏 I tried using sinon
and also nock
but didn't manage to make it work 😞
For now I skipped these tests cause they ping the real Github API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @charliegerard I've added a commit showcasing how to mock the github api – so you should good to go writing the test.
The problem you faced was that it is impossible to mock something that is spawned in a child process -> what you do by calling execa
-> basically a wrapper over child_process
spawn
.
So the cool thing is that you can start up a command without invoking it as a child process and then you achieve ultimate power of having the same global context and therefore mocking things out.
The only importance is that you need to do the mocks before you require the actual code. As it needs to patch the functions first with the stubbed logic.
I hope you are good to go now :) otherwise don't hesitate to reach out to me maybe we can do a 🍐 on that if it helps you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the commit: aee0ff5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you so much! I'll have a look!
@lukasholzer & @erezrokah I think I implemented most of the feedback, the main thing missing is around the tests that I've been having some issues with. Let me know what you think! Also, i'm not sure i understand why this check fails 🤔. I also added the |
@lukasholzer I added some tests so this PR should be ready for review again 🙂 Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for adding the tests and improving our overall test coverage 💯 This deserves a big 🐬 for awesomeness! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks greats @charliegerard 🚀 That you for following up with everything.
Added a few non blocking comments.
c8607a8
@charliegerard I re-approved so you can add |
@erezrokah Thanks so much! Do I need to rename the branch or anything? Looking at the contributing file, it says to have a branch named |
That's for pre-releases. For official releases once you merge a PR, a release PR is generated. I took the liberty of merging an releasing this in #4207 |
Summary
There's a new feature in the UI to start a site from a template that I think should also be available via the CLI so this PR implements this feature 🙂
FYI, I re-used a lot of the code already written for the
sites:create
command.Related to #3947
PR test plan:
netlify sites:create-template
, you should be prompted to pick a template out of the ones currently available (Next.js blog theme, Next.js starter, Nuxt, Hugo).For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)